On Tue, Jul 23, 2024 at 02:36:18PM GMT, Shung-Hsi Yu wrote:
[...]
> > +1
> > Pls document the logic in the code.
> > commit log is good, but good chunk of it probably should be copied
> > as a comment.
> >
> > I've applied the rest of the patches and removed 'test 3' selftest.
> > Pls respin this patch and a test.
> > More than one test would be nice too.
>
> Ack. Will send send another series that:
>
> 1. update current patch
> - add code comment explanation how signed ranges are deduced in
> scalar*_min_max_and()
> - revert 229d6db14942 "selftests/bpf: Workaround strict bpf_lsm return
> value check."
> 2. reintroduce Xu Kuohai's "test 3" into verifier_lsm.c
> 3. add a few tests for BPF_AND's signed range deduction
> - should it be added to verifier_bounds*.c or verifier_and.c?
>
> I think former, because if we later add signed range deduction for
> BPF_OR as well...
I was curious whether there would be imminent need for signed range
deduction for BPF_OR, though looks like there is _not_.
Looking at DAGCombiner::SimplifySelectCC() it does not do the
bitwise-OR variant of what we've encountered[1,2], that is
fold (select_cc seteq (and x, y), 0, A, -1) -> (or (sra (shl x)) A)
In other words, transforming the following theoretial C code that
returns -EACCES when certain bit is unset, and -1 when certain bit is
set
if (fmode & FMODE_WRITE)
return -1;
return -EACCESS;
into the following instructions
r0 <<= 62
r0 s>>= 63 /* set => r0 = -1, unset => r0 = 0 */
r0 |= -13 /* set => r0 = (-1 | -13) = -1, unset => r0 = (0 | -13) = -13 = -EACCESS */
exit /* returns either -1 or -EACCESS */
So signed ranged deduction with BPF_OR is probably just a nice-to-have
for now.
1: https://github.com/llvm/llvm-project/blob/2b78303/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L27657-L27684
2: neither was the setne version transformed, i.e.
fold (select_cc setne (and x, y), 0, A, 0) -> (and (sra (shl x)) A)
> then test for signed range deducation of both
> BPF_AND and BPF_OR can live in the same file, which would be nice
> as signed range deduction of the two are somewhat symmetric
