On Wed, Aug 26, 2020 at 3:06 PM John Fastabend <john.fastabend@xxxxxxxxx> wrote: > > It is a hold-out from when we went from having a 32-bit var-off > and a 64-bit var-off. I'll send a patch its clumsy and not needed > for sure. please follow up with such patches. > The other subtle piece here we should clean up. Its possible > to have a const in the subreg but a non-const in the wider > 64-bit reg. In this case we skip marking the 32-bit subreg > as known and rely on the 64-bit case to handle it. But, we > may if the 64-bit reg is not const fall through and update > the 64-bit bounds. Then later we call __update_reg32_bounds() > and this will use the var_off, previously updated. The > 32-bit bounds are then updated using this var_off so they > are correct even if less precise than we might expect. I > believe xor is correct here as well. makes sense. I think it's correct now, but I agree that cleaning this up would be good as well. > I need to send another patch with a comment for the BTF_ID > types. I'll add some test cases for this 64-bit non-const and > subreg const so we don't break it later. I'm on the fence > if we should tighten the bounds there as well. I'll see if > it helps readability to do explicit 32-bit const handling > there. I had it in one of the early series with the 32-bit > bounds handling, but dropped for what we have now. Not following. Why is this related to btf_id ? > LGTM, but I see a couple follow up patches with tests, comments, > and dropping the duplicate ALU op I'll try to do those Friday, unless > someone else does them first. yes. please :) I've pushed this set to bpf-next in the meantime. Thanks everyone!
