C++: Add implicit destructors for named variables to the IR #15506

rdmarsh2 · 2024-02-01T19:45:10Z

No description provided.

MathiasVP

Some initial comments. So far I've only looked at some of the .expected files. I think I'll delay looking at too many of the .qll changes until the conflicts with main has been resolved.

I think some of the consistency issues I've noted here would also be revealed by running the cpp/ql/test/library-tests/ir/ tests

MathiasVP · 2024-02-02T14:28:29Z

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

+#-----|   Goto -> Block 1
+#-----|   Goto -> Block 3


This looks wrong. It looks like the call to delete and the destructor call is both specified as the successor here.

Yeah - there's an underlying extractor issue here that's causing issues with delete whenever the object being deleted has a destructor.

I see. Do you mind creating an internal issue for this? And is this something we can work around in IR generation for now?

MathiasVP · 2024-02-02T14:28:50Z

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

+#-----|   Goto -> Block 1
+#-----|   Goto -> Block 4


Same here (but for delete[])

MathiasVP · 2024-02-02T14:35:35Z

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

+# 1244|     r1244_2(glval<String>)   = VariableAddress[c]              : 
+# 1244|     r1244_3(glval<unknown>)  = FunctionAddress[~String]        : 
+# 1244|     v1244_4(void)            = Call[~String]                   : func:r1244_3, this:r1244_2
+# 1244|     mu1244_5(unknown)        = ^CallSideEffect                 : ~m?
+# 1244|     v1244_6(void)            = ^IndirectReadSideEffect[-1]     : &:r1244_2, ~m?
+# 1244|     mu1244_7(String)         = ^IndirectMayWriteSideEffect[-1] : &:r1244_2
+# 1244|     r1244_8(glval<String>)   = VariableAddress[b]              : 
+# 1244|     r1244_9(glval<unknown>)  = FunctionAddress[~String]        : 
+# 1244|     v1244_10(void)           = Call[~String]                   : func:r1244_9, this:r1244_8
+# 1244|     mu1244_11(unknown)       = ^CallSideEffect                 : ~m?
+# 1244|     v1244_12(void)           = ^IndirectReadSideEffect[-1]     : &:r1244_8, ~m?
+# 1244|     mu1244_13(String)        = ^IndirectMayWriteSideEffect[-1] : &:r1244_8
+# 1244|     r1244_14(glval<String>)  = VariableAddress[a]              : 
+# 1244|     r1244_15(glval<unknown>) = FunctionAddress[~String]        : 
+# 1244|     v1244_16(void)           = Call[~String]                   : func:r1244_15, this:r1244_14
+# 1244|     mu1244_17(unknown)       = ^CallSideEffect                 : ~m?
+# 1244|     v1244_18(void)           = ^IndirectReadSideEffect[-1]     : &:r1244_14, ~m?
+# 1244|     mu1244_19(String)        = ^IndirectMayWriteSideEffect[-1] : &:r1244_14


I don't think this is correct. a, b and c are all static locals. So their destructors should not be called at the end of the scope. Ideally, we should have some kind of new IRFunction that calls these destructors at the end of main, but let's leave that aside for now.

As a start, I think we should simply not emit these destructors for now.

Oh, good point. I missed that those were static when I was looking over the results.

MathiasVP · 2024-02-02T14:43:16Z

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

+#-----|   Goto -> Block 1
+#-----|   Goto -> Block 4


Here's another case with strange control-flow. Not sure what's going on here.

MathiasVP · 2024-02-02T14:44:34Z

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

+# 1017|   Block 1
+# 1017|     r1017_3(glval<unknown>) = FunctionAddress[~String]        : 
+# 1017|     v1017_4(void)           = Call[~String]                   : func:r1017_3
+# 1017|     mu1017_5(unknown)       = ^CallSideEffect                 : ~m?
+# 1017|     v1017_6(void)           = ^IndirectReadSideEffect[-1]     : &:r1017_2, ~m?
+# 1017|     mu1017_7(String)        = ^IndirectMayWriteSideEffect[-1] : &:r1017_2
+
+# 1020|   Block 1
+# 1020|     r1020_1(glval<unknown>)   = FunctionAddress[~PolymorphicBase] : 
+# 1020|     v1020_2(void)             = Call[~PolymorphicBase]            : func:r1020_1
+# 1020|     mu1020_3(unknown)         = ^CallSideEffect                   : ~m?
+# 1020|     v1020_4(void)             = ^IndirectReadSideEffect[-1]       : &:r1020_7, ~m?
+# 1020|     mu1020_5(PolymorphicBase) = ^IndirectMayWriteSideEffect[-1]   : &:r1020_7


This also looks wrong: We have two Block 1s 🤔

MathiasVP · 2024-02-02T14:45:03Z

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

+# 2059|   Block 1
+# 2059|     r2059_4(glval<unknown>) = FunctionAddress[~Base2]         : 
+# 2059|     v2059_5(void)           = Call[~Base2]                    : func:r2059_4
+# 2059|     mu2059_6(unknown)       = ^CallSideEffect                 : ~m?
+# 2059|     v2059_7(void)           = ^IndirectReadSideEffect[-1]     : &:r2059_3, ~m?
+# 2059|     mu2059_8(Base2)         = ^IndirectMayWriteSideEffect[-1] : &:r2059_3
+
+# 2062|   Block 1
+# 2062|     r2062_1(glval<unknown>) = FunctionAddress[~Base2]         : 
+# 2062|     v2062_2(void)           = Call[~Base2]                    : func:r2062_1
+# 2062|     mu2062_3(unknown)       = ^CallSideEffect                 : ~m?
+# 2062|     v2062_4(void)           = ^IndirectReadSideEffect[-1]     : &:r2062_8, ~m?
+# 2062|     mu2062_5(Base2)         = ^IndirectMayWriteSideEffect[-1] : &:r2062_8
+
+# 2065|   Block 1
+# 2065|     r2065_1(glval<unknown>) = FunctionAddress[~Derived2]      : 
+# 2065|     v2065_2(void)           = Call[~Derived2]                 : func:r2065_1
+# 2065|     mu2065_3(unknown)       = ^CallSideEffect                 : ~m?
+# 2065|     v2065_4(void)           = ^IndirectReadSideEffect[-1]     : &:r2065_8, ~m?
+# 2065|     mu2065_5(Derived2)      = ^IndirectMayWriteSideEffect[-1] : &:r2065_8


This also looks strange.

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

+    result = TranslatedVariableInitialization.super.getChildInternal(id)
+  }
+
+  final override Instruction getChildSuccessorInternal(TranslatedElement elem, EdgeKind kind) {


cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

+    result = TranslatedVariableInitialization.super.getChildInternal(id)
+  }
+
+  final override Instruction getChildSuccessorInternal(TranslatedElement elem, EdgeKind kind) {


cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll

@@ -11,6 +11,7 @@
 private import TranslatedCondition
 private import TranslatedElement
 private import TranslatedExpr
+private import TranslatedCall


rdmarsh2 · 2024-02-02T19:58:58Z

Force-pushed to fix a merge conflict with #15461 and remove some commits from #15318

MathiasVP · 2024-02-05T10:48:06Z

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

+# 2170|     mu2170_8(Bool)          = ^IndirectMayWriteSideEffect[-1] : &:r2170_1
+
+# 2170|   (no string representation)
+# 2170|     CopyValue: (condition decl)
+# 2170|     ConditionalBranch: (condition decl)
+#-----|   False -> Block 3
+#-----|   True -> Block 2
+
+# 2170|   Block 1
+# 2170|     r2170_9(glval<Bool>)     = VariableAddress[B]              : 
+# 2170|     r2170_10(glval<unknown>) = FunctionAddress[operator bool]  : 
+# 2170|     r2170_11(bool)           = Call[operator bool]             : func:r2170_10, this:r2170_9
+# 2170|     mu2170_12(unknown)       = ^CallSideEffect                 : ~m?
+# 2170|     v2170_13(void)           = ^IndirectReadSideEffect[-1]     : &:r2170_9, ~m?
+# 2170|     mu2170_14(Bool)          = ^IndirectMayWriteSideEffect[-1] : &:r2170_9


Hm. Do you know what's going on here? It seems like:

Block 0 ends without any outgoing edges

There's a block named (no string representation) that looks super strange.

Block 1 also has no outgoing edges

Is this related to this PR? Or to variable declarations inside if expressions?

Looks like it's related to variable declarations inside if expressions, not to destructors specifically. It looks like it only fails if there's a constructor call - see the test at 1780 in ir.cpp

MathiasVP · 2024-02-05T11:02:37Z

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

@@ -11647,6 +11844,464 @@ ir.cpp:
 # 2109|     v2109_11(void)          = AliasedUse                     : ~m?
 # 2109|     v2109_12(void)          = ExitFunction                   : 

+# 2115| void TryCatchDestructors(bool)


This is a great test! It looks like we're currently only emitting destructor calls on the non-exceptional blocks. This makes a lot of sense since it's would be a fair bit of work to ensure that we generate a destructor call on the exceptional path so I think simply not emitting them is a good choice.

Am I correct in this observation?

It looks like the extractor doesn't provide any destructor calls on the throw or catch blocks. I'm not sure if that's because the frontend doesn't provide that info or just because the extractor doesn't dump them.

MathiasVP · 2024-02-05T11:06:58Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/InstructionTag.qll


 class InstructionTag extends TInstructionTag {
-  final string toString() { result = "Tag" }
+  final string toString() { result = getInstructionTagId(this) }


Good idea! One of the first things I do when I debug IR construction is also to replace "Tag" with this exact line. So I'm very happy with this change 👍

MathiasVP · 2024-02-05T11:07:48Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll

@@ -87,7 +93,8 @@ abstract class TranslatedCall extends TranslatedExpr {
    )
  }

-  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
+  override Instruction getInstructionSuccessorInternal(InstructionTag tag, EdgeKind kind) {
+    kind instanceof GotoEdge and


Is this line needed given that we're restricting the EdgeKind on the line below?

MathiasVP · 2024-02-05T11:18:03Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedDeclarationEntry.qll

@@ -152,7 +156,13 @@ class TranslatedStaticLocalVariableDeclarationEntry extends TranslatedDeclaratio
    kind instanceof GotoEdge
  }

-  final override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
+  final override Instruction getLastInstructionInternal() {


Since this predicate can return multiple results: Should it be called getALastInstructionInternal? I think we should also add QLDoc to both this predicate (in TranslatedElement.qll) and getLastChild. Specifically, I'd like to know what "last" actually means. Is it "the instruction/child to be executed by this TranslatedElement"? Or something else?

It's "an instruction/child for which the successor instruction is outside this TranslatedElement". I'll add a comment.

MathiasVP · 2024-02-05T11:36:32Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

-  abstract Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind);
+  abstract Instruction getInstructionSuccessorInternal(InstructionTag tag, EdgeKind kind);
+
+  Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {


Can this one be final? Also: QLDoc, please 😂

MathiasVP · 2024-02-05T11:37:27Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

-  abstract Instruction getChildSuccessor(TranslatedElement child, EdgeKind kind);
+  Instruction getChildSuccessorInternal(TranslatedElement child, EdgeKind kind) { none() }
+
+  Instruction getChildSuccessor(TranslatedElement child, EdgeKind kind) {


Can this be final?

MathiasVP · 2024-02-05T11:40:07Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

@@ -75,6 +76,28 @@ abstract class TranslatedExpr extends TranslatedElement {
    expr.isGLValueCategory()
  }

+  abstract TranslatedElement getChildInternal(int id);


QLDoc, please 🙏

MathiasVP · 2024-02-05T11:41:21Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

+    result = max(int childId | exists(this.getChildInternal(childId))) + 1
+    or
+    not exists(this.getChildInternal(_)) and result = 0


Should this predicate only have a result if it doesn't handle destructor calls explicitly itself?

MathiasVP · 2024-02-05T11:42:16Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

+    )
+  }
+
+  final override predicate hasImplicitDestructorCalls() {


Very nitpick: Should this be called hasAnImplicitDestructorCall? Predicate names are very rarely pluralized

github-actions bot added the C++ label Feb 1, 2024

MathiasVP reviewed Feb 2, 2024

View reviewed changes

rdmarsh2 added 7 commits February 2, 2024 17:05

C++: generate instructions for destructor calls in IR

47720e0

C++: Add getLastInstruction to IR generation

3a404ce

C++: Add implicit named destructosrs to the IR CFG

85d1d07

C++: custom destructor handling for for loops

820f4a5

C++: test for destructors in range-based for

4513fd1

C++: test for declarations in if statement

984c7ab

C++: Test for destructors in declaration as if condition

2d010f6

rdmarsh2 force-pushed the rdmarsh2/cpp/ir-synthetic-destructors branch from a341acf to 2d010f6 Compare February 2, 2024 17:40

github-advanced-security bot found potential problems Feb 2, 2024

View reviewed changes

MathiasVP reviewed Feb 5, 2024

View reviewed changes

rdmarsh2 added 2 commits February 5, 2024 18:41

C++: add test for constructors in C++17 decl-in-if

bbabf1d

C++: QLDoc and naming updates for implicit destructors in IR

8013c2a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++: Add implicit destructors for named variables to the IR #15506

C++: Add implicit destructors for named variables to the IR #15506

rdmarsh2 commented Feb 1, 2024

MathiasVP left a comment •

edited

MathiasVP Feb 2, 2024

rdmarsh2 Feb 2, 2024

MathiasVP Feb 5, 2024

MathiasVP Feb 2, 2024

MathiasVP Feb 2, 2024

rdmarsh2 Feb 2, 2024

MathiasVP Feb 2, 2024

MathiasVP Feb 2, 2024

MathiasVP Feb 2, 2024

rdmarsh2 commented Feb 2, 2024

MathiasVP Feb 5, 2024

rdmarsh2 Feb 5, 2024 •

edited

MathiasVP Feb 5, 2024

rdmarsh2 Feb 5, 2024

MathiasVP Feb 5, 2024

MathiasVP Feb 5, 2024

MathiasVP Feb 5, 2024

rdmarsh2 Feb 5, 2024

MathiasVP Feb 5, 2024

MathiasVP Feb 5, 2024

MathiasVP Feb 5, 2024

MathiasVP Feb 5, 2024

MathiasVP Feb 5, 2024

C++: Add implicit destructors for named variables to the IR #15506

Are you sure you want to change the base?

C++: Add implicit destructors for named variables to the IR #15506

Conversation

rdmarsh2 commented Feb 1, 2024

MathiasVP left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdmarsh2 commented Feb 2, 2024

Choose a reason for hiding this comment

rdmarsh2 Feb 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MathiasVP left a comment •

edited

rdmarsh2 Feb 5, 2024 •

edited