EssentialType: Implement correct essential types for bitwise binary operators &, ^ and |.
#787
Conversation
They shouldn't have the standard type if either both the operands are signed or they are both unsigned.
| if exists(getValue()) | ||
| then result = stlr(this) | ||
| else ( | ||
| if leftEssentialType.getSize() > rightEssentialType.getSize() |
There was a problem hiding this comment.
It seems we should have a predicate Type maxRank(Type a, Type b) to simplify this repetition
| leftEssentialTypeCategory = getEssentialTypeCategory(leftEssentialType) and | ||
| rightEssentialTypeCategory = getEssentialTypeCategory(rightEssentialType) | ||
| | | ||
| if |
There was a problem hiding this comment.
Looks like this is now the third occurrence of this pattern, may be worth creating some kind of predicate to deduplicate this/reduce the bloat. Perhaps something like coalesce<stlr>(this, leftEssentialType, rightEssentialType, EssentiallySignedType()), even just bothSigned(leftEssentialType, rightEssentialType) could reduce the boilerplate. And it might be worthy of a bigger hammer such as Type standardCombine(Expr e, Expr operandA, Expr operandB)
| s32 ^ s16; // Essentially signed, int | ||
| s16 ^ s32; // Essentially signed, int | ||
|
|
||
| u32 & s32; // Essentially signed, int |
There was a problem hiding this comment.
These are essentially unsigned ints in the .expected file, which I think is correct based on c11 spec
if the operand that has unsigned integer type has rank greater or
equal to the rank of the type of the other operand, then the operand with
signed integer type is converted to the type of the operand with unsigned
integer type.
| class EssentialBinaryBitwiseExpr extends EssentialExpr, BinaryBitwiseOperation { | ||
| EssentialBinaryBitwiseExpr() { | ||
| not this instanceof LShiftExpr and | ||
| not this instanceof RShiftExpr |
There was a problem hiding this comment.
Any concern here about future updates to the characteristic predicate of EssentialShiftOperationExpr that aren't matched here (and vice versa)? In theory we could not find certain expressions to analyze, or worse, return two results from the overriden member predicates.
It may be better to entangle them here, eg, not this instanceof EssentialBinaryBitwiseOperationExpr
Description
Fixes #786.
Implements correct behaviour for binary bitwise operators.
Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
RULE-10-1RULE-10-3RULE-10-4RULE-10-5RULE-10-6RULE-10-7RULE-10-8RULE-12-2Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
Query development review checklist
For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:
Author
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Reviewer
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.