C++: Measure bounds for Enum constants and reduce getBoundsLimit

[MathiasVP]

This PR fixes a SimpleRangeAnalysis performance problem we've seen at Microsoft.

There were two problems (both of which are fixed in this PR):

• We didn't produce type bounds for Enum types. This let to analyzableExpr not being satisfied for expressions of Enum types, which let to nrOfBoundsExpr not having a result for expressions Enum.
• Now that we compute the expected number of bounds, it turns out that the getBoundsLimit treshold was set way too high. Since this was just the arbitrary default value picked in C++: Range analysis measure bounds #20645 to fix the initial performance problem that PR was meant to solve, I think it's fair to reduce it to a more sane value now that we have yet another performance problem. Reducing it from 2^40 to 2^29 fixes this performance problem, and on anything greater we'll timeout even during codeql test run.

Commit-by-commit review recommended.

[Copilot]

Pull request overview

Improves C++ SimpleRangeAnalysis performance by ensuring enum-typed expressions get appropriate type bounds (so the bounds estimator stays functional) and by lowering the widening trigger threshold to avoid pathological blowups.

Changes:

• Add enum underlying-type handling to type-bound computation used by range analysis utilities.
• Reduce BoundsEstimate.getBoundsLimit from 2^40 to 2^29.
• Add a regression test (missing_bounds.cpp) and update rangeanalysis test expectations.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
cpp/ql/lib/semmle/code/cpp/rangeanalysis/RangeAnalysisUtils.qll	Extend type-bound logic to cover Enum types via an inferred/explicit underlying integral type.
cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll	Lower the bounds-estimate limit used to decide when to enable widening.
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/missing_bounds.cpp	New regression test exercising enum constants / flag-like operations for bounds estimation.
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/nrOfBounds.ql	Tighten functionality test to assert expected estimator behavior for the new regression input.
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/nrOfBounds.expected	Updated expected results for bounds-estimation test output.
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/lowerBound.expected	Updated expected lower-bound results for the new regression input.
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/upperBound.expected	Updated expected upper-bound results for the new regression input.

[paldepind]

I'm really happy to see that you where able to address a simple range analysis performance issue in a clean way without any ad hoc hacks, but simply by 1/ filling a general gap in the range analysis and 2/ lowering getBoundsLimit.

When I implemented the number of bounds estimate my hope was exactly that it would scale to future issues in this way.

I only have a few minor comments in regards to the tests:

• In the tests the change to typeBounds doesn't affect the lower and upper bounds we calculate. But doesn't the change also give improvements there?

  If I recall correctly for a function parameter we use the type to determine the bounds. So when a parameter has an enum type (like foo(MY_ENUM arg)) we might get better bounds now. Could we add a test showing that?

• The change to nonFunctionalNrOfBounds doesn't really fit with the name of that predicate.

• The file name missing_bounds.cpp is maybe a bit confusing after this PR, given that bounds are no longer unexpectedly missing?

To address the last two points I think it would be fine to just add the tests in the existing test.c file and not change nonFunctionalNrOfBounds but just go by the change to nrOfBounds.expected. Alternatively we could add a test_nr_of_bounds.c file and a new predicate missingNrOfBounds with its own annotation.

[MathiasVP]

In the tests the change to typeBounds doesn't affect the lower and upper bounds we calculate. But doesn't the change also give improvements there?

If I recall correctly for a function parameter we use the type to determine the bounds. So when a parameter has an enum type (like foo(MY_ENUM arg)) we might get better bounds now. Could we add a test showing that?

Fixed in bfbb2ee

[MathiasVP]

To address the last two points I think it would be fine to just add the tests in the existing test.c file and not change nonFunctionalNrOfBounds but just go by the change to nrOfBounds.expected. Alternatively we could add a test_nr_of_bounds.c file and a new predicate missingNrOfBounds with its own annotation.

I'd much rather do option 2. There are many constructs that still dont have a measured bound if we moved the test into test.c/test.cpp we'd get lots of inline expectations that would need fixing (i could annotate them all as // $ MISSING, but ... I just dont like the clutter this would introduce).

[paldepind]

I'd much rather do option 2. There are many constructs that still dont have a measured bound if we moved the test into test.c/test.cpp we'd get lots of inline expectations that would need fixing (i could annotate them all as // $ MISSING, but ... I just dont like the clutter this would introduce).

That's fine by me. But just to clarify, my suggestion in option 1 was to make no changes to nonFunctionalNrOfBounds so there'd be no change in inline expectations. I completely agree that having a missing bound expectation for everything in the existing tests would be way too much noise.

[MathiasVP]

Fixed the last one in 5ccd61a (hopefully). There are some small philosophical things one could discuss, but as this is just tests at this point I'd prefer to get this merged sooner rather than later to not risk delaying the next release 🤞 I hope hat's okay!

[paldepind]

Makes sense and sounds good to me 👍