On Tue, Jul 23, 2024 at 12:07 AM Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> wrote: > > 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. Yeah. Let's not complicate the verifier until really necessary. But I wonder whether we should override shouldFoldSelectWithSingleBitTest() in the backend to suppress this optimization. I guess not, since removal of a branch is a good thing.
