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++: Constant type-bounds in the new range analysis #13783

Merged
merged 4 commits into from
Aug 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -307,3 +307,8 @@ class SemConditionalExpr extends SemKnownExpr {
branch = false and result = falseResult
}
}

/** Holds if `upper = true` and `e <= bound` or `upper = false` and `e >= bound`. */
predicate semHasConstantBoundConstantSpecific(SemExpr e, float bound, boolean upper) {
Specific::hasConstantBoundConstantSpecific(e, bound, upper)
}
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,50 @@ module SemanticExprConfig {

/** Gets the expression associated with `instr`. */
SemExpr getSemanticExpr(IR::Instruction instr) { result = Equiv::getEquivalenceClass(instr) }

private predicate typeBounds(SemType t, float lb, float ub) {
exists(SemIntegerType integralType, float limit |
integralType = t and limit = 2.pow(8 * integralType.getByteSize())
|
if integralType instanceof SemBooleanType
then lb = 0 and ub = 1
jketema marked this conversation as resolved.
Show resolved Hide resolved
else
if integralType.isSigned()
then (
lb = -(limit / 2) and ub = (limit / 2) - 1
) else (
lb = 0 and ub = limit - 1
)
)
or
// This covers all floating point types. The range is (-Inf, +Inf).
t instanceof SemFloatingPointType and lb = -(1.0 / 0.0) and ub = 1.0 / 0.0
}

/**
* Holds if `upper = true` and `e <= bound` or `upper = false` and `e >= bound` based
* only on type information.
*/
predicate hasConstantBoundConstantSpecific(Expr e, float bound, boolean upper) {
exists(
SemType converted, SemType unconverted, float unconvertedLb, float convertedLb,
float unconvertedUb, float convertedUb
|
unconverted = getSemanticType(e.getUnconverted().getResultIRType()) and
converted = getSemanticType(e.getConverted().getResultIRType()) and
typeBounds(unconverted, unconvertedLb, unconvertedUb) and
typeBounds(converted, convertedLb, convertedUb) and
(
upper = true and
unconvertedUb < convertedUb and
bound = unconvertedUb
or
upper = false and
unconvertedLb > convertedLb and
bound = unconvertedLb
)
)
}
}

predicate getSemanticExpr = SemanticExprConfig::getSemanticExpr/1;
Expand All @@ -457,3 +501,5 @@ IRBound::Bound getCppBound(SemBound bound) { bound = result }
SemGuard getSemanticGuard(IRGuards::IRGuardCondition guard) { result = guard }

IRGuards::IRGuardCondition getCppGuard(SemGuard guard) { guard = result }

predicate hasConstantBoundConstantSpecific = SemanticExprConfig::hasConstantBoundConstantSpecific/3;
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,10 @@ module CppLangImplConstant implements LangSig<FloatDelta> {
/**
* Holds if `e >= bound` (if `upper = false`) or `e <= bound` (if `upper = true`).
*/
predicate hasConstantBound(SemExpr e, float bound, boolean upper) { none() }
predicate hasConstantBound(SemExpr e, float bound, boolean upper, SemReason reason) {
semHasConstantBoundConstantSpecific(e, bound, upper) and
reason instanceof SemTypeReason
Copy link
Contributor

Choose a reason for hiding this comment

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

I've trouble following this. Why os this restricted to SemTypeReason only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because semHasConstantBoundConstantSpecific only infers bounds based on type information. It's probably possible to push this condition into semHasConstantBoundConstantSpecific, but that then requires that semHasConstantBoundConstantSpecific knows about the SemReason type which it currently doesn't since that predicate is defined outside the CppLangImplConstant module (which instantiates those types).

Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

So zero bounds are not constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

semHasConstantBoundConstantSpecific just forwards to hasConstantBoundConstantSpecific (which is the predicate added here). The only thing that adds is the type bounds inferred from type conversions.

Copy link
Contributor

Choose a reason for hiding this comment

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

We briefly discussed this face-to-face. This is the correct thing to do here because as Mathias writes:

semHasConstantBoundConstantSpecific only infers bounds based on type information

We could extend this later and push down the reason argument, but that is not of immediate concern here.

}

/**
* Holds if `e >= bound + delta` (if `upper = false`) or `e <= bound + delta` (if `upper = true`).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ module CppLangImplRelative implements LangSig<FloatDelta> {
/**
* Holds if `e >= bound` (if `upper = false`) or `e <= bound` (if `upper = true`).
*/
predicate hasConstantBound(SemExpr e, float bound, boolean upper) { none() }
predicate hasConstantBound(SemExpr e, float bound, boolean upper, SemReason reason) { none() }

/**
* Holds if `e >= bound + delta` (if `upper = false`) or `e <= bound + delta` (if `upper = true`).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ signature module LangSig<DeltaSig D> {
/**
* Holds if `e >= bound` (if `upper = false`) or `e <= bound` (if `upper = true`).
*/
predicate hasConstantBound(SemExpr e, D::Delta bound, boolean upper);
predicate hasConstantBound(SemExpr e, D::Delta bound, boolean upper, SemReason reason);

/**
* Holds if `e >= bound + delta` (if `upper = false`) or `e <= bound + delta` (if `upper = true`).
Expand Down Expand Up @@ -920,14 +920,15 @@ module RangeStage<
* Holds if `e` has an upper (for `upper = true`) or lower
* (for `upper = false`) bound of `b`.
*/
private predicate baseBound(SemExpr e, D::Delta b, boolean upper) {
hasConstantBound(e, b, upper)
private predicate baseBound(SemExpr e, D::Delta b, boolean upper, SemReason reason) {
hasConstantBound(e, b, upper, reason)
or
upper = false and
b = D::fromInt(0) and
semPositive(e.(SemBitAndExpr).getAnOperand()) and
// REVIEW: We let the language opt out here to preserve original results.
not ignoreZeroLowerBound(e)
not ignoreZeroLowerBound(e) and
reason instanceof SemNoReason
}

/**
Expand Down Expand Up @@ -1055,11 +1056,10 @@ module RangeStage<
origdelta = delta and
reason instanceof SemNoReason
or
baseBound(e, delta, upper) and
baseBound(e, delta, upper, reason) and
b instanceof SemZeroBound and
fromBackEdge = false and
origdelta = delta and
reason instanceof SemNoReason
origdelta = delta
or
exists(SemSsaVariable v, SemSsaReadPositionBlock bb |
boundedSsa(v, bb, b, delta, upper, fromBackEdge, origdelta, reason) and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ private Instruction getABoundIn(SemBound b, IRFunction func) {
pragma[inline]
private predicate boundedImpl(Instruction i, Instruction b, int delta) {
exists(SemBound bound, IRFunction func |
semBounded(getSemanticExpr(i), bound, delta, true, _) and
semBounded(getSemanticExpr(i), bound, delta, true,
any(SemReason reason | not reason instanceof SemTypeReason)) and
b = getABoundIn(bound, func) and
i.getEnclosingIRFunction() = func
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ Instruction getABoundIn(SemBound b, IRFunction func) {
pragma[inline]
predicate boundedImpl(Instruction i, Instruction b, int delta) {
exists(SemBound bound, IRFunction func |
semBounded(getSemanticExpr(i), bound, delta, true, _) and
semBounded(getSemanticExpr(i), bound, delta, true,
any(SemReason reason | not reason instanceof SemTypeReason)) and
b = getABoundIn(bound, func) and
pragma[only_bind_out](i.getEnclosingIRFunction()) = func
)
Expand Down Expand Up @@ -93,7 +94,8 @@ predicate arrayTypeHasSizes(ArrayType arr, int baseTypeSize, int size) {
bindingset[pai]
pragma[inline_late]
predicate constantUpperBounded(PointerArithmeticInstruction pai, int delta) {
semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), delta, true, _)
semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), delta, true,
any(SemReason reason | not reason instanceof SemTypeReason))
}

bindingset[pai, size]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,18 +195,18 @@ int test13(char c, int i) {
int z = i+1; // $ overflow=+
range(z); // $ range="==InitializeParameter: i+1"
range(c + i + uc + x + y + z); // $ overflow=+- overflow=+ overflow=- MISSING: range=>=1
range((double)(c + i + uc + x + y + z)); // $ overflow=+ overflow=+- overflow=- MISSING: range=>=1
range((double)(c + i + uc + x + y + z)); // $ overflow=+ overflow=+- overflow=- range=<=4294967295 MISSING: range=>=1
return (double)(c + i + uc + x + y + z); // $ overflow=+- overflow=+ overflow=-
}

// Regression test for ODASA-6013.
int test14(int x) {
int x0 = (int)(char)x;
range(x0);
range(x0); // $ range=<=127 range=>=-128
int x1 = (int)(unsigned char)x;
range(x1);
range(x1); // $ range=<=255 range=>=0
int x2 = (int)(unsigned short)x;
range(x2);
range(x2); // $ range=<=65535 range=>=0
int x3 = (int)(unsigned int)x;
range(x3);
char c0 = x;
Expand Down Expand Up @@ -759,9 +759,9 @@ unsigned long mult_overflow() {
unsigned long mult_lower_bound(unsigned int ui, unsigned long ul) {
if (ui >= 10) {
range(ui); // $ range=>=10
range((unsigned long)ui); // $ range=>=10
unsigned long result = (unsigned long)ui * ui; // $ overflow=+
range(result); // $ MISSING: range=>=100
jketema marked this conversation as resolved.
Show resolved Hide resolved
range((unsigned long)ui); // $ range=>=10 range=<=4294967295
unsigned long result = (unsigned long)ui * ui; // no overflow
range(result); // $ range=>=100 range=<=18446744065119617024
return result; // BUG: upper bound should be >= 18446744065119617025
}
if (ul >= 10) {
Expand Down Expand Up @@ -888,7 +888,7 @@ void notequal_variations(short n, float f) {
}

if (n >= 5) {
if (2 * n - 10 == 0) { // $ overflow=+
if (2 * n - 10 == 0) { // no overflow
jketema marked this conversation as resolved.
Show resolved Hide resolved
range(n); // $ range=>=5 MISSING: range===5
return;
}
Expand Down Expand Up @@ -936,7 +936,7 @@ void two_bounds_from_one_test(short ss, unsigned short us) {
range(ss); // -32768 .. 32767
}

if (ss + 1 < sizeof(int)) { // $ overflow=+
if (ss + 1 < sizeof(int)) { // $ overflow=-
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 verified experimentally

range(ss); // -1 .. 2
}
}
Expand Down