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.

Sign up for GitHub

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

Jump to bottom

C++: Add another IR consistency query #18013

Merged
merged 4 commits into from
Nov 18, 2024
Merged

C++: Add another IR consistency query #18013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
493ea6d
C++: Add a new consistency check for branching on non-boolean values.
MathiasVP Nov 18, 2024
28391d1
C++: Add some more tests to expose consistency issues.
MathiasVP Nov 18, 2024
4a4552b
C++: Also update the consistency query itself.
MathiasVP Nov 18, 2024
ccca0b6
C++: Also update syntax-zoo tests.
MathiasVP Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/IRConsistency.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,4 +546,26 @@ module InstructionConsistency {
"' has no associated variable, in function '$@'." and
irFunc = getInstructionIRFunction(instr, irFuncText)
}

query predicate nonBooleanOperand(
Instruction instr, string message, OptionalIRFunction irFunc, string irFuncText
) {
exists(Instruction unary |
unary = instr.(LogicalNotInstruction).getUnary() and
not unary.getResultIRType() instanceof IRBooleanType and
irFunc = getInstructionIRFunction(instr, irFuncText) and
message =
"Logical Not instruction " + instr.toString() +
" with non-Boolean operand, in function '$@'."
)
Comment on lines +553 to +560
Copy link
Contributor

@jketema jketema Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure to what extent you want to change the IR behavior here, but this probably requires some care with cases like the following (for which you have already added a test):

int y = ...;
int x = !y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I intend to fix those cases as well 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

For !y, I'd suggest doing the same thing we do for C++, which is to first convert the int to bool via CompareNE.

Separate from the consistency issue, I wonder if translating !y directly as CompareEQ, instead of CompareNE, LogicalNot, would give better results somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate from the consistency issue, I wonder if translating !y directly as CompareEQ, instead of CompareNE, LogicalNot, would give better results somewhere.

The problem with this is the translation back to Exprs (which we need in particular when someone does dataflow to a condition of an if statement). In that case, the unconverted result expression should be the CompareNE, and the converted result expression should be the LogicalNot. But if we merge them together as a CompareEQ then the unconverted result expression of !y would be a CompareEQ which is a bit strange.

or
exists(Instruction cond |
cond = instr.(ConditionalBranchInstruction).getCondition() and
not cond.getResultIRType() instanceof IRBooleanType and
irFunc = getInstructionIRFunction(instr, irFuncText) and
message =
"Conditional branch instruction " + instr.toString() +
" with non-Boolean condition, in function '$@'."
)
}
}
22 changes: 22 additions & 0 deletions cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/IRConsistency.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,4 +546,26 @@ module InstructionConsistency {
"' has no associated variable, in function '$@'." and
irFunc = getInstructionIRFunction(instr, irFuncText)
}

query predicate nonBooleanOperand(
Instruction instr, string message, OptionalIRFunction irFunc, string irFuncText
) {
exists(Instruction unary |
unary = instr.(LogicalNotInstruction).getUnary() and
not unary.getResultIRType() instanceof IRBooleanType and
irFunc = getInstructionIRFunction(instr, irFuncText) and
message =
"Logical Not instruction " + instr.toString() +
" with non-Boolean operand, in function '$@'."
)
or
exists(Instruction cond |
cond = instr.(ConditionalBranchInstruction).getCondition() and
not cond.getResultIRType() instanceof IRBooleanType and
irFunc = getInstructionIRFunction(instr, irFuncText) and
message =
"Conditional branch instruction " + instr.toString() +
" with non-Boolean condition, in function '$@'."
)
}
}
22 changes: 22 additions & 0 deletions cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/IRConsistency.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,4 +546,26 @@ module InstructionConsistency {
"' has no associated variable, in function '$@'." and
irFunc = getInstructionIRFunction(instr, irFuncText)
}

query predicate nonBooleanOperand(
Instruction instr, string message, OptionalIRFunction irFunc, string irFuncText
) {
exists(Instruction unary |
unary = instr.(LogicalNotInstruction).getUnary() and
not unary.getResultIRType() instanceof IRBooleanType and
irFunc = getInstructionIRFunction(instr, irFuncText) and
message =
"Logical Not instruction " + instr.toString() +
" with non-Boolean operand, in function '$@'."
)
or
exists(Instruction cond |
cond = instr.(ConditionalBranchInstruction).getCondition() and
not cond.getResultIRType() instanceof IRBooleanType and
irFunc = getInstructionIRFunction(instr, irFuncText) and
message =
"Conditional branch instruction " + instr.toString() +
" with non-Boolean condition, in function '$@'."
)
}
}
3 changes: 2 additions & 1 deletion cpp/ql/src/Metrics/Internal/IRConsistency.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,5 @@ select count(Instruction i | IRConsistency::missingOperand(i, _, _, _) | i) as m
count(Instruction i | IRConsistency::nonUniqueEnclosingIRFunction(i, _, _, _) | i) as nonUniqueEnclosingIRFunction,
count(FieldAddressInstruction i | IRConsistency::fieldAddressOnNonPointer(i, _, _, _) | i) as fieldAddressOnNonPointer,
count(Instruction i | IRConsistency::thisArgumentIsNonPointer(i, _, _, _) | i) as thisArgumentIsNonPointer,
count(Instruction i | IRConsistency::nonUniqueIRVariable(i, _, _, _) | i) as nonUniqueIRVariable
count(Instruction i | IRConsistency::nonUniqueIRVariable(i, _, _, _) | i) as nonUniqueIRVariable,
count(Instruction i | IRConsistency::nonBooleanOperand(i, _, _, _) | i) as nonBooleanOperand
Loading