Skip to content

C++: Better value numbering support for loading fields in IR #2772

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

Merged
merged 8 commits into from
Feb 6, 2020
Merged

C++: Better value numbering support for loading fields in IR #2772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
687dcb7
C++: Add testcase demonstrating unexpectly different value numbers
MathiasVP Feb 5, 2020
54f0b4a
C++: Add more support for load instructions
MathiasVP Feb 5, 2020
5e5bd92
C++: Accept output
MathiasVP Feb 5, 2020
4f27750
C++/C#: Sync identical files
MathiasVP Feb 5, 2020
cfcf087
C++: Add comment explaining buggy value number
MathiasVP Feb 5, 2020
ba395cf
C++: Update test annotations and accept output
MathiasVP Feb 5, 2020
527181b
C++: Rename CongruentCopyInstructionTotal to LoadTotalOverlapInstruct…
MathiasVP Feb 6, 2020
aaa6233
C++/C#: Sync identical files
MathiasVP Feb 6, 2020
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
31 changes: 30 additions & 1 deletion cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ newtype TValueNumber =
) {
inheritanceConversionValueNumber(_, irFunc, opcode, baseClass, derivedClass, operand)
} or
TLoadTotalOverlapValueNumber(
IRFunction irFunc, IRType type, ValueNumber memOperand, ValueNumber operand
) {
loadTotalOverlapValueNumber(_, irFunc, type, memOperand, operand)
} or
TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }

/**
Expand Down Expand Up @@ -101,12 +106,18 @@ class ValueNumber extends TValueNumber {
* The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
* congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
*/
private class CongruentCopyInstruction extends CopyInstruction {
class CongruentCopyInstruction extends CopyInstruction {
CongruentCopyInstruction() {
this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
}
}

class LoadTotalOverlapInstruction extends LoadInstruction {
LoadTotalOverlapInstruction() {
this.getSourceValueOperand().getDefinitionOverlap() instanceof MustTotallyOverlap
}
}

/**
* Holds if this library knows how to assign a value number to the specified instruction, other than
* a `unique` value number that is never shared by multiple instructions.
Expand All @@ -131,6 +142,8 @@ private predicate numberableInstruction(Instruction instr) {
instr instanceof PointerArithmeticInstruction
or
instr instanceof CongruentCopyInstruction
or
instr instanceof LoadTotalOverlapInstruction
}

private predicate variableAddressValueNumber(
Expand Down Expand Up @@ -205,6 +218,7 @@ private predicate unaryValueNumber(
instr.getEnclosingIRFunction() = irFunc and
not instr instanceof InheritanceConversionInstruction and
not instr instanceof CopyInstruction and
not instr instanceof FieldAddressInstruction and
instr.getOpcode() = opcode and
instr.getResultIRType() = type and
valueNumber(instr.getUnary()) = operand
Expand All @@ -221,6 +235,16 @@ private predicate inheritanceConversionValueNumber(
valueNumber(instr.getUnary()) = operand
}

private predicate loadTotalOverlapValueNumber(
LoadTotalOverlapInstruction instr, IRFunction irFunc, IRType type, ValueNumber memOperand,
ValueNumber operand
) {
instr.getEnclosingIRFunction() = irFunc and
instr.getResultIRType() = type and
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there test cases where the type is required to avoid conflating values? If so, I'd be surprised if those cases can't be tweaked so the type is the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Off the top of my head:

double foo(void* p, bool b) {
  if (b) {
    return *(int*)p;
  }
  else {
    return *(float*)p;
  }
}

I'm not sure we'd treat the results of the two casts as congruent today, but we probably should. However, we would not want the results of the two indirect loads to be congruent, even though their address operands were congruent.

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 included the type mainly because all the other constructors did so. I will take a further look tomorrow and see if it actually makes a difference

valueNumber(instr.getAnOperand().(MemoryOperand).getAnyDef()) = memOperand and
valueNumberOfOperand(instr.getAnOperand().(AddressOperand)) = operand
Copy link
Contributor

Choose a reason for hiding this comment

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

What CopyInstruction types actually have an AddressOperand? I believe this PR is intended for LoadInstructions, but the implementation includes all CopyInstructions.

  • The CopyValueInstruction doesn't have an AddressOperand, so it'll be missing from this predicate even though it's present in numberableInstruction, which means it'll have no value number at all, not even a unique one. I'm guessing that's not a problem in practice since CopyValueInstructions read from register operands rather than memory operands, and so they must always exactly overlap.
  • The StoreInstruction has an AddressOperand, but it's used for a different purpose. It doesn't have a MemoryOperand, though, so it'll be missing from this predicate. That's okay in practice for the same reasons as CopyValueInstruction.
  • The QLDoc on LoadOperand say that this operand type is also used on ReturnValueInstruction and ThrowValueInstruction. Is there something to be done for those instructions too? I don't think there is, because those instructions don't return anything. Their loads are always exact, so they won't be relevant in this case. I wonder why those instructions have a load built into them instead of using a separate LoadInstruction.
  • A ReadSideEffectInstruction is not a CopyInstruction, but I think it behaves like a LoadInstruction: it has an address and a memory operand, and that memory operand can be (is always?) inexact. It seems to me like we'd want value numbers for the indirections that are accessed by these read-side-effect instructions, but I don't see how it's possible. First, a ReadSideEffectInstruction does not return a result; second, we can't tell without analysing or modelling the callee what portion of the data will be loaded.

Okay, most of this discussion was just me thinking out loud and confirming that for the way the IR is generated today we're fine. But I think the right thing to do, for future robustness, is to restrict CongruentCopyInstructionTotal to LoadInstructions only and rename it accordingly.

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've restricted it to extend LoadInstruction only now, and renamed it CongruentCopyInstructionTotal for lack of a better name. The test results are identical.

}

/**
* Holds if `instr` should be assigned a unique value number because this library does not know how
* to determine if two instances of that instruction are equivalent.
Expand Down Expand Up @@ -313,6 +337,11 @@ private ValueNumber nonUniqueValueNumber(Instruction instr) {
TPointerArithmeticValueNumber(irFunc, opcode, type, elementSize, leftOperand, rightOperand)
)
or
exists(IRType type, ValueNumber memOperand, ValueNumber operand |
loadTotalOverlapValueNumber(instr, irFunc, type, memOperand, operand) and
result = TLoadTotalOverlapValueNumber(irFunc, type, memOperand, operand)
)
or
// The value number of a copy is just the value number of its source value.
result = valueNumber(instr.(CongruentCopyInstruction).getSourceValue())
)
Expand Down
31 changes: 30 additions & 1 deletion cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ newtype TValueNumber =
) {
inheritanceConversionValueNumber(_, irFunc, opcode, baseClass, derivedClass, operand)
} or
TLoadTotalOverlapValueNumber(
IRFunction irFunc, IRType type, ValueNumber memOperand, ValueNumber operand
) {
loadTotalOverlapValueNumber(_, irFunc, type, memOperand, operand)
} or
TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }

/**
Expand Down Expand Up @@ -101,12 +106,18 @@ class ValueNumber extends TValueNumber {
* The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
* congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
*/
private class CongruentCopyInstruction extends CopyInstruction {
class CongruentCopyInstruction extends CopyInstruction {
CongruentCopyInstruction() {
this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
}
}

class LoadTotalOverlapInstruction extends LoadInstruction {
LoadTotalOverlapInstruction() {
this.getSourceValueOperand().getDefinitionOverlap() instanceof MustTotallyOverlap
}
}

/**
* Holds if this library knows how to assign a value number to the specified instruction, other than
* a `unique` value number that is never shared by multiple instructions.
Expand All @@ -131,6 +142,8 @@ private predicate numberableInstruction(Instruction instr) {
instr instanceof PointerArithmeticInstruction
or
instr instanceof CongruentCopyInstruction
or
instr instanceof LoadTotalOverlapInstruction
}

private predicate variableAddressValueNumber(
Expand Down Expand Up @@ -205,6 +218,7 @@ private predicate unaryValueNumber(
instr.getEnclosingIRFunction() = irFunc and
not instr instanceof InheritanceConversionInstruction and
not instr instanceof CopyInstruction and
not instr instanceof FieldAddressInstruction and
instr.getOpcode() = opcode and
instr.getResultIRType() = type and
valueNumber(instr.getUnary()) = operand
Expand All @@ -221,6 +235,16 @@ private predicate inheritanceConversionValueNumber(
valueNumber(instr.getUnary()) = operand
}

private predicate loadTotalOverlapValueNumber(
LoadTotalOverlapInstruction instr, IRFunction irFunc, IRType type, ValueNumber memOperand,
ValueNumber operand
) {
instr.getEnclosingIRFunction() = irFunc and
instr.getResultIRType() = type and
valueNumber(instr.getAnOperand().(MemoryOperand).getAnyDef()) = memOperand and
valueNumberOfOperand(instr.getAnOperand().(AddressOperand)) = operand
}

/**
* Holds if `instr` should be assigned a unique value number because this library does not know how
* to determine if two instances of that instruction are equivalent.
Expand Down Expand Up @@ -313,6 +337,11 @@ private ValueNumber nonUniqueValueNumber(Instruction instr) {
TPointerArithmeticValueNumber(irFunc, opcode, type, elementSize, leftOperand, rightOperand)
)
or
exists(IRType type, ValueNumber memOperand, ValueNumber operand |
loadTotalOverlapValueNumber(instr, irFunc, type, memOperand, operand) and
result = TLoadTotalOverlapValueNumber(irFunc, type, memOperand, operand)
)
or
// The value number of a copy is just the value number of its source value.
result = valueNumber(instr.(CongruentCopyInstruction).getSourceValue())
)
Expand Down
31 changes: 30 additions & 1 deletion cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ newtype TValueNumber =
) {
inheritanceConversionValueNumber(_, irFunc, opcode, baseClass, derivedClass, operand)
} or
TLoadTotalOverlapValueNumber(
IRFunction irFunc, IRType type, ValueNumber memOperand, ValueNumber operand
) {
loadTotalOverlapValueNumber(_, irFunc, type, memOperand, operand)
} or
TUniqueValueNumber(IRFunction irFunc, Instruction instr) { uniqueValueNumber(instr, irFunc) }

/**
Expand Down Expand Up @@ -101,12 +106,18 @@ class ValueNumber extends TValueNumber {
* The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
* congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
*/
private class CongruentCopyInstruction extends CopyInstruction {
class CongruentCopyInstruction extends CopyInstruction {
CongruentCopyInstruction() {
this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
}
}

class LoadTotalOverlapInstruction extends LoadInstruction {
LoadTotalOverlapInstruction() {
this.getSourceValueOperand().getDefinitionOverlap() instanceof MustTotallyOverlap
}
}

/**
* Holds if this library knows how to assign a value number to the specified instruction, other than
* a `unique` value number that is never shared by multiple instructions.
Expand All @@ -131,6 +142,8 @@ private predicate numberableInstruction(Instruction instr) {
instr instanceof PointerArithmeticInstruction
or
instr instanceof CongruentCopyInstruction
or
instr instanceof LoadTotalOverlapInstruction
}

private predicate variableAddressValueNumber(
Expand Down Expand Up @@ -205,6 +218,7 @@ private predicate unaryValueNumber(
instr.getEnclosingIRFunction() = irFunc and
not instr instanceof InheritanceConversionInstruction and
not instr instanceof CopyInstruction and
not instr instanceof FieldAddressInstruction and
instr.getOpcode() = opcode and
instr.getResultIRType() = type and
valueNumber(instr.getUnary()) = operand
Expand All @@ -221,6 +235,16 @@ private predicate inheritanceConversionValueNumber(
valueNumber(instr.getUnary()) = operand
}

private predicate loadTotalOverlapValueNumber(
LoadTotalOverlapInstruction instr, IRFunction irFunc, IRType type, ValueNumber memOperand,
ValueNumber operand
) {
instr.getEnclosingIRFunction() = irFunc and
instr.getResultIRType() = type and
valueNumber(instr.getAnOperand().(MemoryOperand).getAnyDef()) = memOperand and
valueNumberOfOperand(instr.getAnOperand().(AddressOperand)) = operand
}

/**
* Holds if `instr` should be assigned a unique value number because this library does not know how
* to determine if two instances of that instruction are equivalent.
Expand Down Expand Up @@ -313,6 +337,11 @@ private ValueNumber nonUniqueValueNumber(Instruction instr) {
TPointerArithmeticValueNumber(irFunc, opcode, type, elementSize, leftOperand, rightOperand)
)
or
exists(IRType type, ValueNumber memOperand, ValueNumber operand |
loadTotalOverlapValueNumber(instr, irFunc, type, memOperand, operand) and
result = TLoadTotalOverlapValueNumber(irFunc, type, memOperand, operand)
)
or
// The value number of a copy is just the value number of its source value.
result = valueNumber(instr.(CongruentCopyInstruction).getSourceValue())
)
Expand Down
8 changes: 8 additions & 0 deletions cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/GlobalValueNumbering.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,11 @@
| test.cpp:105:11:105:12 | (Base *)... | 105:c11-c12 106:c14-c35 107:c11-c12 |
| test.cpp:105:11:105:12 | pd | 105:c11-c12 106:c33-c34 |
| test.cpp:105:15:105:15 | b | 105:c15-c15 107:c15-c15 109:c10-c10 |
| test.cpp:125:11:125:12 | pa | 125:c11-c12 126:c11-c12 128:c3-c4 129:c11-c12 |
| test.cpp:125:15:125:15 | x | 125:c15-c15 126:c15-c15 128:c7-c7 |
| test.cpp:136:11:136:18 | global_a | 136:c11-c18 137:c11-c18 139:c3-c10 |
| test.cpp:136:21:136:21 | x | 136:c21-c21 137:c21-c21 139:c13-c13 |
| test.cpp:144:11:144:12 | pa | 144:c11-c12 145:c11-c12 147:c3-c4 149:c11-c12 |
| test.cpp:145:15:145:15 | y | 145:c15-c15 147:c7-c7 |
| test.cpp:153:11:153:18 | global_a | 153:c11-c18 154:c11-c18 156:c3-c10 |
| test.cpp:153:21:153:21 | x | 153:c21-c21 154:c21-c21 |
Loading