> 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.
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.
Let me know if I'm missing anything.
Dave
