C++: Ensure that there are AST GuardConditions for || and &&

[MathiasVP]

This PR fixes two bugs in the AST-based GuardCondition library. First some background:

In C/C++ guard conditions come in two flavors: One that directly is built directly on the IR, and one that wraps the IR-based logic in an AST-based wrapper.

To encode short circuiting the IR desugars binary logical operators (i.e., || and &&) into individual checks. For example the IR for:

if(b1 && b2) {
 ...
}

is the same as the IR for

if(b1) {
  if(b2) {
    ...
  }
}

This means that there's no Instruction that holds the computes the b1 && b2 expression. That is, there is no instruction i for which i.getUnconvertedResultExpression() is the b1 && b2. As a result, there is no AST-based guard condition for b1 && b2.

This has confused people in the past, and because of that we added special cases in the AST-based wrapper to make sure these exist. Unfortunately, we implemented the logic around these all wrong 🙈. The second part of this PR fixes that logic!

The first commit fixes another AST-wrapper bug: The mapping from IRBlocks to AST BasicBlocks was incorrect when the IRBlock was the initial block of a function. This is because, in the AST-based BasicBlock library the function itself is the exit block of the function, but there are some Instructions whose AST component is the function itself. This resulted in bugs where the initial IRBlock of a function was incorrectly translated into the exit BasicBlock of a function.

Commit-by-commit review recommended. You can see the effects on our tests in each commit. They are a bit hard to interpret because the tests haven't been ported to use inline expectations yet. Maybe I should do that as a follow-up 🤔

DCA looks excellent. We get two new TPs.¹ cc @bdrodes because you complained about politely raised this issue.

Footnotes

1. Don't look to closely at the "related locations" in the alert comparison. It turns out this work helped discover a 6 year old bug in DCA. The results are actually TPs, but the locations of the related locations are incorrectly swapped in the generated report. ↩

[Copilot]

Pull request overview

This PR fixes two bugs in the AST-based GuardCondition library for C/C++. The first bug involved incorrect mapping from IR blocks to AST BasicBlocks when the IR block was the initial block of a function. The second bug involved incorrect logic for creating AST-based guard conditions for binary logical operators (|| and &&).

Changes:

• Fixed the mapping of IR blocks to AST BasicBlocks by excluding instructions generated by TranslatedFunction that map to the function itself
• Rewrote the control flow logic for binary logical operators to correctly use OR instead of AND when determining which blocks are controlled
• Added special handling for ChiInstructions that derive from excluded instructions

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll	Fixed two bugs: (1) Added exclusions for instructions from TranslatedFunction and related ChiInstructions in excludeAsControlledInstruction, (2) Rewrote valueControls and valueControlsEdge in GuardConditionFromBinaryLogicalOperator to use OR logic instead of AND, and added aliased imports for clarity
cpp/ql/test/library-tests/controlflow/guards/GuardsEnsure.expected	Updated test expectations: removed incorrect guards pointing to function declarations and added correct guards for && expressions controlling additional blocks
cpp/ql/test/library-tests/controlflow/guards/GuardsControl.expected	Updated test expectations: removed incorrect guards pointing to function declarations and added correct guards for && expressions
cpp/ql/test/library-tests/controlflow/guards-ir/tests.expected	Updated test expectations: removed incorrect guards and added correct guards for && expressions with line number references

---

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

[IdrissRio]

Looks good to me 👍
Thanks for the clear and thorough PR description. I also really appreciate the amount of comments in the code, it made the review much easier.

Overall, the changes make sense to me, and DCA looks good as well.