Skip to content

Commit a86a15d

Browse files
author
AndreiDiaconu1
committed
Fix problem with IsExpr
The translation of `IsExpr` created a sanity check to fail since it generated a Phi node that had only one source: if a variable was declared as part of the `IsExpr`, a conditional branch was generated, and the variable was defined only in the true successor; this has been changes so that the declaration happens before the conditional branch, and the variable is uninitialized (this removed the need for the `isInitializedByElement` predicate from `TranslatedDeclarationBase`, so that has been removed) and only the assignment happens in the true successor block (so now the two inputs of the Phi node are the result of the `Uninitialized` instruction and the `Store` instruction from the true successor block).
1 parent 17e6b80 commit a86a15d

File tree

6 files changed

+17
-36
lines changed

6 files changed

+17
-36
lines changed

csharp/ql/src/semmle/code/csharp/ir/IRConfiguration.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ class IRConfiguration extends TIRConfiguration {
1515
/**
1616
* Holds if IR should be created for callable `callable`. By default, holds for all callables.
1717
*/
18-
predicate shouldCreateIRForFunction(Callable callable) {
18+
predicate shouldCreateIRForFunction(Callable callable) {
1919
callable.getLocation().getFile().getExtension() = "cs"
2020
}
2121
}

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedDeclaration.qll

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,4 @@ class TranslatedLocalVariableDeclaration extends TranslatedLocalDeclaration,
7272
// then the simple variable initialization
7373
result = getTranslatedInitialization(var.getInitializer())
7474
}
75-
76-
override predicate isInitializedByElement() { expr.getParent() instanceof IsExpr }
7775
}

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedExpr.qll

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1792,27 +1792,27 @@ class TranslatedIsExpr extends TranslatedNonConstantExpr {
17921792
this.hasVar() and
17931793
tag = GeneratedBranchTag() and
17941794
(
1795-
tag = GeneratedBranchTag() and
17961795
kind instanceof TrueEdge and
1797-
result = getPatternVarDecl().getFirstInstruction()
1796+
result = this.getInstruction(InitializerStoreTag())
17981797
or
1799-
tag = GeneratedBranchTag() and
18001798
kind instanceof FalseEdge and
18011799
result = this.getParent().getChildSuccessor(this)
18021800
)
18031801
or
18041802
tag = GeneratedConstantTag() and
18051803
kind instanceof GotoEdge and
1806-
result = this.getInstruction(GeneratedNEQTag())
1804+
if this.hasVar()
1805+
then result = this.getPatternVarDecl().getFirstInstruction()
1806+
else result = this.getInstruction(GeneratedNEQTag())
18071807
}
18081808

18091809
override Instruction getChildSuccessor(TranslatedElement child) {
18101810
child = this.getIsExpr() and
18111811
result = this.getInstruction(ConvertTag())
18121812
or
18131813
this.hasVar() and
1814-
child = getPatternVarDecl() and
1815-
result = this.getInstruction(InitializerStoreTag())
1814+
child = this.getPatternVarDecl() and
1815+
result = this.getInstruction(GeneratedNEQTag())
18161816
}
18171817

18181818
override predicate hasInstruction(
@@ -1842,7 +1842,7 @@ class TranslatedIsExpr extends TranslatedNonConstantExpr {
18421842
this.hasVar() and
18431843
tag = GeneratedBranchTag() and
18441844
opcode instanceof Opcode::ConditionalBranch and
1845-
resultType = expr.getType() and
1845+
resultType instanceof VoidType and
18461846
isLValue = false
18471847
}
18481848

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/common/TranslatedDeclarationBase.qll

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,7 @@ abstract class LocalVariableDeclarationBase extends TranslatedElement {
3939
kind instanceof GotoEdge and
4040
if hasUninitializedInstruction()
4141
then result = getInstruction(InitializerStoreTag())
42-
else
43-
if isInitializedByElement()
44-
then
45-
// initialization is done by an element
46-
result = getParent().getChildSuccessor(this)
47-
else result = getInitialization().getFirstInstruction()
42+
else result = getInitialization().getFirstInstruction()
4843
)
4944
or
5045
hasUninitializedInstruction() and
@@ -75,11 +70,8 @@ abstract class LocalVariableDeclarationBase extends TranslatedElement {
7570
* desugaring process.
7671
*/
7772
predicate hasUninitializedInstruction() {
78-
(
79-
not exists(getInitialization()) or
80-
getInitialization() instanceof TranslatedListInitialization
81-
) and
82-
not isInitializedByElement()
73+
not exists(getInitialization()) or
74+
getInitialization() instanceof TranslatedListInitialization
8375
}
8476

8577
Instruction getVarAddress() { result = getInstruction(InitializerVariableAddressTag()) }
@@ -101,10 +93,4 @@ abstract class LocalVariableDeclarationBase extends TranslatedElement {
10193
* as a different step, but do it during the declaration.
10294
*/
10395
abstract TranslatedElement getInitialization();
104-
105-
/**
106-
* Holds if a declaration is not explicitly initialized,
107-
* but will be implicitly initialized by an element.
108-
*/
109-
abstract predicate isInitializedByElement();
11096
}

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/desugar/internal/TranslatedCompilerGeneratedDeclaration.qll

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,10 +72,6 @@ abstract class TranslatedCompilerGeneratedDeclaration extends LocalVariableDecla
7272
// element
7373
override LocalVariable getDeclVar() { none() }
7474

75-
// A compiler generated element always has an explicit
76-
// initialization
77-
override predicate isInitializedByElement() { none() }
78-
7975
override Type getVarType() { result = getIRVariable().getType() }
8076

8177
/**

csharp/ql/test/library-tests/ir/ir/raw_ir.expected

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -806,8 +806,10 @@ isexpr.cs:
806806
# 13| r0_13(Object) = Load : &:r0_12, ~mu0_2
807807
# 13| r0_14(Is_A) = CheckedConvertOrNull : r0_13
808808
# 13| r0_15(Is_A) = Constant[0] :
809-
# 13| r0_16(Boolean) = CompareNE : r0_14, r0_15
810-
# 13| r0_17(Boolean) = ConditionalBranch : r0_16
809+
# 13| r0_16(glval<Is_A>) = VariableAddress[tmp] :
810+
# 13| mu0_17(Is_A) = Uninitialized[tmp] : &:r0_16
811+
# 13| r0_18(Boolean) = CompareNE : r0_14, r0_15
812+
# 13| v0_19(Void) = ConditionalBranch : r0_18
811813
#-----| False -> Block 2
812814
#-----| True -> Block 3
813815

@@ -817,13 +819,12 @@ isexpr.cs:
817819
# 8| v1_2(Void) = ExitFunction :
818820

819821
# 13| Block 2
820-
# 13| v2_0(Void) = ConditionalBranch : r0_16
822+
# 13| v2_0(Void) = ConditionalBranch : r0_18
821823
#-----| False -> Block 5
822824
#-----| True -> Block 4
823825

824826
# 13| Block 3
825-
# 13| r3_0(glval<Is_A>) = VariableAddress[tmp] :
826-
# 13| mu3_1(Is_A) = Store : &:r3_0, r0_14
827+
# 13| mu3_0(Is_A) = Store : &:r0_16, r0_14
827828
#-----| Goto -> Block 2
828829

829830
# 15| Block 4

0 commit comments

Comments
 (0)