Skip to content
Merged
26 changes: 26 additions & 0 deletions cpp/ql/src/semmle/code/cpp/controlflow/IRGuards.qll
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,32 @@ class IRGuardCondition extends Instruction {
ne.controls(controlled, testIsTrue.booleanNot()))
}

/**
* Holds if `branch` jumps directly to `succ` when this condition is `testIsTrue`.
*
* This predicate is intended to help with situations in which an inference can only be made
* based on an edge between a block with multiple successors and a block with multiple
* predecessors. For example, in the following situation, an inference can be made about the
* value of `x` at the end of the `if` statement, but there is no block which is controlled by
* the `if` statement when `x >= y`.
* ```
* if (x < y) {
* x = y;
* }
* return x;
* ```
*/
predicate hasBranchEdge(ConditionalBranchInstruction branch, IRBlock succ, boolean testIsTrue) {
branch.getCondition() = this and
(
testIsTrue = true and
succ.getFirstInstruction() = branch.getTrueSuccessor()
or
testIsTrue = false and
succ.getFirstInstruction() = branch.getFalseSuccessor()
)
}

/** Holds if (determined by this guard) `left < right + k` evaluates to `isLessThan` if this expression evaluates to `testIsTrue`. */
cached predicate comparesLt(Operand left, Operand right, int k, boolean isLessThan, boolean testIsTrue) {
compares_lt(this, left, right, k, isLessThan, testIsTrue)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ module InstructionSanity {
query predicate instructionWithoutUniqueBlock(Instruction instr, int blockCount) {
blockCount = count(instr.getBlock()) and
blockCount != 1
}
}
}

/**
Expand Down Expand Up @@ -750,6 +750,15 @@ class BinaryInstruction extends Instruction {
final Instruction getRightOperand() {
result = getAnOperand().(RightOperand).getDefinitionInstruction()
}

/**
* Holds if this instruction's operands are `op1` and `op2`, in either order.
*/
final predicate hasOperands(Operand op1, Operand op2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

QLDoc, please.

op1 = getAnOperand().(LeftOperand) and op2 = getAnOperand().(RightOperand)
or
op1 = getAnOperand().(RightOperand) and op2 = getAnOperand().(LeftOperand)
}
}

class AddInstruction extends BinaryInstruction {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ class Operand extends TOperand {
result = "Operand"
}

Location getLocation() {
result = getInstruction().getLocation()
}

/**
* Gets the `Instruction` that consumes this operand.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ class ValueNumber extends TValueNumber {
instr order by instr.getBlock().getDisplayIndex(), instr.getDisplayIndexInBlock()
)
}

final Operand getAUse() {
this = valueNumber(result.getDefinitionInstruction())
}
}

/**
Expand All @@ -107,6 +111,7 @@ private class CongruentCopyInstruction extends CopyInstruction {
def = this.getSourceValue() and
(
def.getResultMemoryAccess() instanceof IndirectMemoryAccess or
def.getResultMemoryAccess() instanceof PhiMemoryAccess or
not def.hasMemoryResult()
)
)
Expand Down Expand Up @@ -211,14 +216,21 @@ private predicate uniqueValueNumber(Instruction instr, FunctionIR funcIR) {
/**
* Gets the value number assigned to `instr`, if any. Returns at most one result.
*/
ValueNumber valueNumber(Instruction instr) {
cached ValueNumber valueNumber(Instruction instr) {
result = nonUniqueValueNumber(instr) or
exists(FunctionIR funcIR |
uniqueValueNumber(instr, funcIR) and
result = TUniqueValueNumber(funcIR, instr)
)
}

/**
* Gets the value number assigned to `instr`, if any. Returns at most one result.
*/
ValueNumber valueNumberOfOperand(Operand op) {
result = valueNumber(op.getDefinitionInstruction())
}

/**
* Gets the value number assigned to `instr`, if any, unless that instruction is assigned a unique
* value number.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ module InstructionSanity {
query predicate instructionWithoutUniqueBlock(Instruction instr, int blockCount) {
blockCount = count(instr.getBlock()) and
blockCount != 1
}
}
}

/**
Expand Down Expand Up @@ -750,6 +750,12 @@ class BinaryInstruction extends Instruction {
final Instruction getRightOperand() {
result = getAnOperand().(RightOperand).getDefinitionInstruction()
}

final predicate hasOperands(Operand op1, Operand op2) {
op1 = getAnOperand().(LeftOperand) and op2 = getAnOperand().(RightOperand)
or
op1 = getAnOperand().(RightOperand) and op2 = getAnOperand().(LeftOperand)
}
}

class AddInstruction extends BinaryInstruction {
Expand Down
4 changes: 4 additions & 0 deletions cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ class Operand extends TOperand {
result = "Operand"
}

Location getLocation() {
result = getInstruction().getLocation()
}

/**
* Gets the `Instruction` that consumes this operand.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ private class CongruentCopyInstruction extends CopyInstruction {
def = this.getSourceValue() and
(
def.getResultMemoryAccess() instanceof IndirectMemoryAccess or
def.getResultMemoryAccess() instanceof PhiMemoryAccess or
not def.hasMemoryResult()
)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ module InstructionSanity {
query predicate instructionWithoutUniqueBlock(Instruction instr, int blockCount) {
blockCount = count(instr.getBlock()) and
blockCount != 1
}
}
}

/**
Expand Down Expand Up @@ -750,6 +750,12 @@ class BinaryInstruction extends Instruction {
final Instruction getRightOperand() {
result = getAnOperand().(RightOperand).getDefinitionInstruction()
}

final predicate hasOperands(Operand op1, Operand op2) {
op1 = getAnOperand().(LeftOperand) and op2 = getAnOperand().(RightOperand)
or
op1 = getAnOperand().(RightOperand) and op2 = getAnOperand().(LeftOperand)
}
}

class AddInstruction extends BinaryInstruction {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ class Operand extends TOperand {
result = "Operand"
}

Location getLocation() {
result = getInstruction().getLocation()
}

/**
* Gets the `Instruction` that consumes this operand.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ private class CongruentCopyInstruction extends CopyInstruction {
def = this.getSourceValue() and
(
def.getResultMemoryAccess() instanceof IndirectMemoryAccess or
def.getResultMemoryAccess() instanceof PhiMemoryAccess or
not def.hasMemoryResult()
)
)
Expand Down
80 changes: 80 additions & 0 deletions cpp/ql/src/semmle/code/cpp/rangeanalysis/Bound.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import cpp
private import semmle.code.cpp.ir.IR
private import semmle.code.cpp.ir.ValueNumbering

private newtype TBound =
TBoundZero() or
TBoundValueNumber(ValueNumber vn) {
exists(Instruction i |
vn.getAnInstruction() = i and
(
i.getResultType() instanceof IntegralType or
i.getResultType() instanceof PointerType
) and
not vn.getAnInstruction() instanceof ConstantInstruction
|
i instanceof PhiInstruction
or
i instanceof InitializeParameterInstruction
or
i instanceof CallInstruction
or
i instanceof VariableAddressInstruction
or
i instanceof FieldAddressInstruction
or
i.(LoadInstruction).getSourceAddress() instanceof VariableAddressInstruction
or
i.(LoadInstruction).getSourceAddress() instanceof FieldAddressInstruction
or
i.getAUse() instanceof ArgumentOperand
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If TBoundOperand should correspond to Java's TBoundExpr, then this includes too much. But at the time of writing this I haven't yet seen how you deal with the SsaReadPosition difference, so maybe that will explain it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had thought that bounding relative to Operand might give a tighter bound than Instruction in some cases involving guard conditions; I'm no longer convinced that that's true.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also looks like TBoundInstruction includes too much if it's trying to correspond to TBoundSsa - it should likely exclude temporary variables that are introduced by the IR construction. Note, that the overall algorithm works by constructing a graph (that's all the *step* predicates), which should be fast as its size is likely linear in the size of the program, and the recursive calculation in the graph essentially calculates the transitive closure in the graph restricted to certain starting points, and the Bound class is exactly those starting points. So Bound should be as small as possible to maintain good performance, while including all the interesting nodes in the graph that one might want as a bound for something else, and all phi nodes as they are needed as bounds for themselves to get any sort of relevant bounds in loops.

Copy link
Contributor Author

@rdmarsh2 rdmarsh2 Dec 12, 2018

Choose a reason for hiding this comment

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

Right. I think the set of interesting bounds in C++ is going to be bigger than in Java, but I haven't narrowed it down fully. Also, @dave-bartolomeo suggested that TBoundInstruction might be better as TBoundValueNumber. We'd probably need to go through ValueNumber anyway to match up cases where the allocation is done in the same function. I'm not sure how well it will work for cases where the bound is passed into the function as a field of an inparam.

Copy link
Contributor

Choose a reason for hiding this comment

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

I absolutely don't think value numbers in general should be part of Bound - that's going to be too big without any benefits. E.g. bounding anything by the value number of x+1 is never going to be useful, as it's always better to bound directly by x.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, sorry, just now read the commit with the code change above to use value numbers. When restricted to atomic things, then it looks like it might be fine. I'm slightly concerned by the i.getAUse() instanceof ArgumentOperand disjunct - doesn't that open the door for valuenumbers of things like x+1?

Copy link
Contributor Author

@rdmarsh2 rdmarsh2 Dec 13, 2018

Choose a reason for hiding this comment

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

Yes; Consider the following code:

int *foo(int x) {
  int *arr = new int[x*2];
  for(int i = 0; i < x*2; i++) {
    arr[i] = i; // we need to be able to relate i to the size of arr, which is x*2
  }
  return arr;
}

I think we will need to be able to have bounds relative to arbitrary ValueNumbers that are used as arguments to allocating functions, and given that C++ allows custom allocators, I don't think we'll be able to do that without allowing any ValueNumber used as an argument to be a bound.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I can see why it can be useful. Then let me just say that the size of the Bound type and whether a smaller set of valuenumbers can be used here is something to potentially revisit if performance becomes a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm starting on performance testing; it looks like the main stage of range analysis is about 50% slower with the i.getAUse() instanceof ArgumentOperand disjunct than without.


/**
* A bound that may be inferred for an expression plus/minus an integer delta.
*/
abstract class Bound extends TBound {
abstract string toString();

/** Gets an expression that equals this bound plus `delta`. */
abstract Instruction getInstruction(int delta);

/** Gets an expression that equals this bound. */
Instruction getInstruction() { result = getInstruction(0) }

abstract Location getLocation();
}

/**
* The bound that corresponds to the integer 0. This is used to represent all
* integer bounds as bounds are always accompanied by an added integer delta.
*/
class ZeroBound extends Bound, TBoundZero {
override string toString() { result = "0" }

override Instruction getInstruction(int delta) { result.(ConstantValueInstruction).getValue().toInt() = delta }

override Location getLocation() {
result instanceof UnknownDefaultLocation
}
}
/**
* A bound corresponding to the value of an `Instruction`.
*/
class ValueNumberBound extends Bound, TBoundValueNumber {
ValueNumber vn;

ValueNumberBound() {
this = TBoundValueNumber(vn)
}

/** Gets the SSA variable that equals this bound. */
override Instruction getInstruction(int delta) { this = TBoundValueNumber(valueNumber(result)) and delta = 0}

override string toString() { result = vn.getExampleInstruction().toString() }

override Location getLocation() {
result = vn.getLocation()
}
}
Loading