On Wed, Nov 01, 2023 at 08:37:51PM -0700, Andrii Nakryiko wrote:
> When performing 32-bit conditional operation operating on lower 32 bits
> of a full 64-bit register, register full value isn't changed. We just
> potentially gain new knowledge about that register's lower 32 bits.
>
> Unfortunately, __reg_combine_{32,64}_into_{64,32} logic that
> reg_set_min_max() performs as a last step, can lose information in some
> cases due to __mark_reg64_unbounded() and __reg_assign_32_into_64().
> That's bad and completely unnecessary. Especially __reg_assign_32_into_64()
> looks completely out of place here, because we are not performing
> zero-extending subregister assignment during conditional jump.
>
> So this patch replaced __reg_combine_* with just a normal
> reg_bounds_sync() which will do a proper job of deriving u64/s64 bounds
> from u32/s32, and vice versa (among all other combinations).
>
> __reg_combine_64_into_32() is also used in one more place,
> coerce_reg_to_size(), while handling 1- and 2-byte register loads.
> Looking into this, it seems like besides marking subregister as
> unbounded before performing reg_bounds_sync(), we were also performing
> deduction of smin32/smax32 and umin32/umax32 bounds from respective
> smin/smax and umin/umax bounds. It's now redundant as reg_bounds_sync()
> performs all the same logic more generically (e.g., without unnecessary
> assumption that upper 32 bits of full register should be zero).
>
> Long story short, we remove __reg_combine_64_into_32() completely, and
> coerce_reg_to_size() now only does resetting subreg to unbounded and then
> performing reg_bounds_sync() to recover as much information as possible
> from 64-bit umin/umax and smin/smax bounds, set explicitly in
> coerce_reg_to_size() earlier.
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
LGTM. Seeing __mark_reg{64,32}_unbounded() removed had me spooked
quite a bit though :)
Acked-by: Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx>
