On Fri, Oct 21, 2022 at 10:56 AM Dave Thaler <dthaler@xxxxxxxxxxxxx> wrote: > > > On Wed, Oct 19, 2022 at 4:35 PM Stanislav Fomichev <sdf@xxxxxxxxxx> > > wrote: > > > On Wed, Oct 19, 2022 at 2:06 PM Dave Thaler <dthaler@xxxxxxxxxxxxx> > > wrote: > > > > > > > > sdf@xxxxxxxxxx wrote: > > > > > > ``BPF_ADD | BPF_X | BPF_ALU`` means:: > > > > > > > > > > > - dst_reg = (u32) dst_reg + (u32) src_reg; > > > > > > + dst = (u32) (dst + src) > > > > > > > > > > IIUC, by going from (u32) + (u32) to (u32)(), we want to signal > > > > > that the value will just wrap around? > > > > > > > > Right. In particular the old line could be confusing if one > > > > misinterpreted it as saying that the addition could overflow into a > > > > higher bit. The new line is intended to be unambiguous that the upper 32 > > bits are 0. > > > > > > > > > But isn't it more confusing now because it's unclear what the sign > > > > > of the dst/src is (s32 vs u32)? > > > > > > > > As stated the upper 32 bits have to be 0, just as any other u32 assignment. > > > > > > Do we mention somewhere above/below that the operands are unsigned? > > > IOW, what prevents me from reading this new format as follows? > > > > > > dst = (u32) ((s32)dst + (s32)src) > > > > The doc mentions it, but I completely agree with you. > > The original line was better. > > Dave, please undo this part. > > Nothing prevents you from reading the new format as > dst = (u32) ((s32)dst + (s32)src) > because that implementation wouldn't be wrong. Assuming the underlying cpu architecture is using 2s complement to represent negative numbers, right? (which is probably a safe one nowadays) I don't know whether that's something that's spelled out in a generic BPF architecture doc. > Below is why, please point out any logic errors if you see any. > > Mathematically, all of the following have identical results: > dst = (u32) ((s32)dst + (s32)src) > dst = (u32) ((u32)dst + (u32)src) > dst = (u32) ((s32)dst + (u32)src) > dst = (u32) ((u32)dst + (s32)src) > > u32 and s32, once you allow overflow/underflow to wrap within 32 bits, are > mathematical rings (see https://en.wikipedia.org/wiki/Ring_(mathematics) ) > meaning they're a circular space where X, X + 2^32, and X - 2^32 are equal. > So (s32)src == (u32)src when the most significant bit is clear, and > (s32)src == (u32)src - 2^32 when the most significant bit is set. > > So the sign of the addition operands does not matter here. > What matters is whether you do addition where the result can be > more than 32 bits or not, which is what the new line makes unambiguous > and the old line did not. > > Specifically, nothing prevented mis-interpreting the old line as > > u64 temp = (u32)dst; > temp += (u32)src; > dst = temp; > > which would give the wrong answer since the upper 32-bits might be non-zero. > > u64 temp = (s32)dst; > temp += (s32)src; > dst = (u32)temp; > > Would however give the correct answer, same as > > u64 temp = (u32)dst; > temp += (u32)src; > dst = (u32)temp; > > As such, I maintain the old line was bad and the new line is still good. [..] > If you like I can explicitly say > dst = (u32) ((u32)dst + (u32)src) > but as noted above the operand sign is irrelevant once you cast the result. That might be a good compromise. A bit verbose, but solves both "what happens to the overflow" and "what's the operand sign" questions.
