Skip to content

Conversation

@esteffin
Copy link
Contributor

@esteffin esteffin commented Nov 7, 2025

Reverts #20645

Reverting as this causes stable timeout regressions in certain projects (of various sizes).

@esteffin esteffin requested a review from a team as a code owner November 7, 2025 18:00
Copilot AI review requested due to automatic review settings November 7, 2025 18:00
@esteffin esteffin added C++ no-change-note-required This PR does not need a change note labels Nov 7, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request reverts a performance optimization from the C++ range analysis library by removing the bounds estimation mechanism that was designed to prevent combinatorial explosions in range analysis.

Key Changes

  • Removed the BoundsEstimate module that estimated the number of bounds to prevent performance issues
  • Removed helper predicates widenLowerBound() and widenUpperBound()
  • Inlined widening logic directly at call sites, now only applying widening for recursive expressions (not for expressions with "too many bounds")
  • Removed test functions repeated_if_statements(), conditional_nested_guards(), and many_conditional_assignments() that were designed to stress-test the bounds estimation
  • Removed the nrOfBounds.ql test query
  • Reverted changelog entries claiming performance improvements

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
SimpleRangeAnalysis.qll Removed bounds estimation module, inlined widening logic with syntax errors in two aggregations
test.c Removed 214 lines of test code for combinatorial explosion scenarios
nrOfBounds.ql Deleted test query for bounds estimation
upperBound.expected, lowerBound.expected, ternaryUpper.expected, ternaryLower.expected Updated test expectations after removing test code
CHANGELOG.md, change-notes/released/6.0.1.md Reverted performance improvement claims

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

result =
min(float widenUB |
widenUB = wideningUpperBounds(getVariableRangeType(v)) and
not widenUB < truncatedUB
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The widening logic has been changed from widenUB >= truncatedUB (in the removed helper function) to not widenUB < truncatedUB. While these conditions are logically equivalent for comparable numbers, consider using the original condition widenUB >= truncatedUB for consistency with the semantic intent and to avoid potential issues with NaN values or floating-point edge cases.

Suggested change
not widenUB < truncatedUB
widenUB >= truncatedUB

Copilot uses AI. Check for mistakes.
result =
max(float widenLB |
widenLB = wideningLowerBounds(expr.getUnspecifiedType()) and
not widenLB > newLB
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aggregation is missing the result expression. This should be max(float widenLB | ... | widenLB) to match the pattern used elsewhere in the codebase (e.g., lines 1805-1810). Without the result part, the aggregation may not compile or behave correctly.

Suggested change
not widenLB > newLB
not widenLB > newLB
| widenLB

Copilot uses AI. Check for mistakes.
min(float widenUB |
widenUB = wideningUpperBounds(expr.getUnspecifiedType()) and
not widenUB < newUB
)
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aggregation is missing the result expression. This should be min(float widenUB | ... | widenUB) to match the pattern used elsewhere in the codebase (e.g., lines 1835-1840). Without the result part, the aggregation may not compile or behave correctly.

Suggested change
)
| widenUB)

Copilot uses AI. Check for mistakes.
result =
max(float widenLB |
widenLB = wideningLowerBounds(getVariableRangeType(v)) and
not widenLB > truncatedLB
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The widening logic has been changed from widenLB <= truncatedLB (in the removed helper function) to not widenLB > truncatedLB. While these conditions are logically equivalent for comparable numbers, consider using the original condition widenLB <= truncatedLB for consistency with the semantic intent and to avoid potential issues with NaN values or floating-point edge cases.

Suggested change
not widenLB > truncatedLB
widenLB <= truncatedLB

Copilot uses AI. Check for mistakes.
@esteffin esteffin added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Nov 7, 2025
@jketema
Copy link
Contributor

jketema commented Nov 7, 2025

The proper way to do this is a merge back from the release branch, which Michael will take care of.

@mbg
Copy link
Member

mbg commented Nov 7, 2025

Already done 👍🏻

@jketema jketema closed this Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR documentation no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants