Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++: Support IRGuards with no implicit boolean conversion #16364

Merged
merged 6 commits into from
Apr 30, 2024

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Apr 30, 2024

When this is compiled as C code:

char* p = ...;
if(p) { ... } 

there's no implicit conversion to boolean, and thus we don't generate a CompareInstruction that the IRGuards library rely on.

This PR adds a case to IRGuards so that we can also infer guards when no CompareInstructions are present in the condition

@MathiasVP MathiasVP requested a review from a team as a code owner April 30, 2024 11:08
@github-actions github-actions bot added the C++ label Apr 30, 2024
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Apr 30, 2024
@MathiasVP MathiasVP requested a review from geoffw0 April 30, 2024 13:00
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

Two small comments.

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Outdated Show resolved Hide resolved
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM, and DCA looks clean.

// value.(BooleanValue).getValue() = true
// ```
// but all we know is that the value is non-zero in the true branch.
// So we can only conclude something in the false branch.
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the consequences if we included common values for true (namely 1 and -1)? Would any results be wrong, or just incomplete?

Copy link
Contributor Author

@MathiasVP MathiasVP Apr 30, 2024

Choose a reason for hiding this comment

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

Results would be wrong. For example, consider this example:

void foo(char* p) {
  if(p) { // assume we uncommented the above so that we inferred that `p == 1` is implied by this
    if(p == 1) { // I know this is a weird comparison to do on a pointer, but oh well.
       // We would conclude that this branch was always taken
    }
  }
}

@MathiasVP MathiasVP merged commit b86aeb6 into github:main Apr 30, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ 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.

3 participants