Re: [PATCH v2 bpf-next 11/18] bpf: Follow PHIs in bpf_scan_ir

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/11/09 05:42PM, bot+bpf-ci@kernel.org wrote:
> > bpf: Follow PHIs in bpf_scan_ir
> 
> TL;DR: Nits only: in bpf_scan_ir.cpp, isErrnoRange() duplicates
> errnoConstant()'s errno check, so the range is defined twice; also a comment
> style nit.
> 
> > diff --git a/scripts/llvm-plugins/bpf_scan_ir.cpp b/scripts/llvm-plugins/bpf_scan_ir.cpp
> > --- a/scripts/llvm-plugins/bpf_scan_ir.cpp
> > +++ b/scripts/llvm-plugins/bpf_scan_ir.cpp
> 
> [ ... ]
> 
> > @@ -34,9 +36,40 @@ namespace {
> >  
> >  static constexpr int MaxErrno = 4095;
> >  
> > +// is Range in [-4095, 0) ?
> > +static bool isErrnoRange(const ConstantRange &Range)
> > +{
> > +	unsigned Width = Range.getBitWidth();
> > +
> > +	if (Width != 32 && Width != 64)
> > +		return false;
> > +
> > +	auto L = APInt(Width, 0) - APInt(Width, MaxErrno);
> > +	auto R = APInt(Width, 0);
> > +	return !Range.isEmptySet() && ConstantRange(L, R).contains(Range);
> > +}
> > +
> 
> this isn't a bug, but could singleErrno() and errnoConstant() share one
> check?
> 
> isErrnoRange() has a single caller, singleErrno(), which only passes it a
> range that getSingleElement() already showed to hold exactly one value.
> For that input the width check, the isEmptySet() test and the wrapped
> ConstantRange containment do the same thing as errnoConstant() below
> (width of 32 or 64, then N >= -MaxErrno && N < 0), so the errno range is
> now defined in two places, and the !Range.isEmptySet() test can never
> fail.
> 
> For example, errnoConstant() could call
> singleErrno(ConstantRange(C->getValue()), N), or singleErrno() could test
> the sign-extended element against MaxErrno directly.
> 
> Also, the "// is Range in [-4095, 0) ?" comment is the only function-level
> comment in the file using // rather than /* */, and it hardcodes -4095
> where the code uses MaxErrno.

Yes, thanks, will fix

> 
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42662799554