On Mon, Aug 18, 2025 at 07:44:05PM +0200, Paul Chaignon wrote: > On Thu, Aug 14, 2025 at 08:55:22PM +0800, Shung-Hsi Yu wrote: > > On Wed, Aug 13, 2025 at 05:34:08PM +0200, Paul Chaignon wrote: [...] > > > @@ -15891,6 +15891,8 @@ static int is_scalar_branch_taken(struct bpf_reg_state *reg1, struct bpf_reg_sta > > > return 0; > > > if (smin1 > smax2 || smax1 < smin2) > > > return 0; > > > + if (!tnum_agree(t1, t2)) > > > + return 0; > > > > Could we reuse tnum_xor() here instead? > > > > If xor of two register cannot be 0, then the two can never hold the same > > value. Also we can use the tnum_xor() result in place of tnum_is_const() > > checks. > > > > case BPF_JEQ: > > t = tnum_xor(t1, t2); > > if (!t.mask) /* Equvalent of tnum_is_const(t1) && tnum_is_const(t2) */ > > return t.value == 0; > > if (umin1 > umax2 || umax1 < umin2) > > return 0; > > if (smin1 > smax2 || smax1 < smin2) > > return 0; > > if (!t.value) /* Equvalent of !tnum_agree(t1, t2) */ > > return 0; > > ... [...] > > Looks slighly less readable though. > > Nice find on the xor-based equivalent version! I lean a bit toward > keeping the slightly more readable version though. The xor version is > probably a bit more efficient, but I'm unsure that matters much for this > piece of code, whereas I think readability matters a lot here. True, I probably had jump a bit too quickly to optimization. Thinking about this over again adding a new helper does make more sense (someone probably will try to add it in the future anyway), so let's take the readability preference. You can add Acked-by: Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> Maybe add the check right after 'tnum_is_const(t1) && tnum_is_const(t2)' check, and before 'umin/umax/smin/smax' check though? Bunching tnum usage together for aesthetic. > ... That > said, if others prefer the xor version, I don't mind much :) FWIW I'd ideally would like tnum_intersect to return 'false' if no intersection can be found (similar to check_add_overflow), then we can use it here. And forcing check to always be done should help avoid running into some of the register bound violations. But such change felt too intrusive for the purpose of this patchset, maybe for a future refactor. __must_check bool tnum_intersect(struct tnum a, struct tnum b, struct tnum *out)
