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

[bot+bpf-ci]

> 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.


---
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