C++: First-class destructors in AST #15318

rdmarsh2 · 2024-01-12T21:27:56Z

This PR adds a new getSyntheticDestructor predicate to Stmt and Expr, and adds destructors to the IR. Currently only the Stmt relation is populated by the extractor.

MathiasVP

I started reviewing this before I noticed it was a draft. Sorry! Feel free to ignore my comments if you're still working all these things out 😄

MathiasVP · 2024-01-16T22:43:48Z

cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll

+  /**
+   * Gets the `n`th compiler-generated destructor call that is performed after this expression, in
+   * order of destruction.
+   */


I think it would be good to have an example here (specifically, for when there are multiple destructors attached to an element).

I think this would still be good, but given that we don't actually generate any such destructor calls yet this can wait 🙈

MathiasVP · 2024-01-16T22:44:41Z

cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll

+   * Gets the `n`th compiler-generated destructor call that is performed after this expression, in
+   * order of destruction.
+   */
+  DestructorCall getSyntheticDestructor(int n) {


Could we avoid using the term "synthetic" in the public API? I don't actually know why the relational is using this term, because I don't think there's anything more "synthetic" about these destructor calls than other things in the AST

Maybe getImplicitDestructor instead?

Sounds good to me. Maybe even getImplicitDestructorCall since this is really the call to the destructor?

MathiasVP · 2024-01-16T22:48:00Z

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

  result = expr.getParentWithConversions()
  or
  result.(Destructor).getADestruction() = expr
+  or
+  result.(Expr).getASyntheticDestructor() = expr
+  or
+  result.(Stmt).getASyntheticDestructor() = expr


Is it ever the case that an expr has multiple parents with these new cases included?

MathiasVP · 2024-01-16T22:48:49Z

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

+    (
+    call instanceof ConstructorCall or
+    call instanceof DestructorCall
+    ) and 


The comment above here is no longer correct, right? i.e., this part:

// Don't bother with destructor calls for now, since we won't see very many of them in the IR
// until we start injecting implicit destructor calls.

MathiasVP

I think this all LGTM, @rdmarsh2! I think we can just take this out of draft and merge it (after accepting the test changes)?

MathiasVP · 2024-01-24T15:17:29Z

cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll

+  /**
+   * Gets the `n`th compiler-generated destructor call that is performed after this expression, in
+   * order of destruction.
+   */


I think this would still be good, but given that we don't actually generate any such destructor calls yet this can wait 🙈

rdmarsh2 · 2024-01-24T18:35:28Z

The failing test is in the internal repo, so I'll need to open a PR there and do a synchronized merge.

MathiasVP

LGTM! I don't think we should put any change notes here until we properly support destructor calls associated with expressions

jketema · 2024-01-24T19:38:23Z

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

 #  619|                 Type = [PointerType] const char *
 #  619|                 ValueCategory = prvalue
 #  620|     getStmt(4): [ReturnStmt] return ...
+#  620|       getImplicitDestructorCall(0): [DestructorCall] call to ~String


This can be done in a separate PR, but I wonder if we should print these in the order in which they should be called, which is from the highest to the lowest index:

codeql/cpp/ql/lib/semmlecode.cpp.dbscheme

Lines 1122 to 1127 in 32a2ea1

/**

* `destructor_call` destructs the `i`'th entity that should be

* destructed following `element`. Note that entities should be

* destructed in reverse construction order, so for a given `element`

* these should be called from highest to lowest `i`.

*/

They already are - I reversed the indexes from the dbscheme order in getImplicitDestructorCall, so index 0 should be the first to be destructed.

I see now. I was clearly not reading the code correctly 🤦 . Thanks for clarifying.

github-actions bot added the C++ label Jan 12, 2024

MathiasVP reviewed Jan 16, 2024

View reviewed changes

rdmarsh2 force-pushed the rdmarsh2/cpp/first-class-destructors branch from 99b4e63 to 278b22c Compare January 19, 2024 17:30

rdmarsh2 changed the title ~~C++: First-class destructors in AST and IR~~ C++: First-class destructors in AST Jan 23, 2024

MathiasVP approved these changes Jan 24, 2024

View reviewed changes

rdmarsh2 marked this pull request as ready for review January 24, 2024 18:35

rdmarsh2 requested a review from a team as a code owner January 24, 2024 18:35

rdmarsh2 added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Jan 24, 2024

rdmarsh2 added 5 commits January 24, 2024 18:52

C++: add getSyntheticDestructor to Expr and Stmt

1acc111

C++: add destructors in PrintAST

3a0290b

C++: switch to getImplicitDestructorCall

3970a9f

C++: more documentation for implict destructor calls

56c9566

C++: autoformat for first-class destructors

0bc0231

rdmarsh2 force-pushed the rdmarsh2/cpp/first-class-destructors branch from 64769a1 to 0bc0231 Compare January 24, 2024 18:53

MathiasVP approved these changes Jan 24, 2024

View reviewed changes

jketema reviewed Jan 24, 2024

View reviewed changes

rdmarsh2 merged commit 67a8639 into github:main Jan 24, 2024

MathiasVP mentioned this pull request Jan 29, 2024

C++: PrintAST support for destructor calls #15270

Closed

rdmarsh2 mentioned this pull request Feb 2, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

C++: First-class destructors in AST #15318

C++: First-class destructors in AST #15318

Uh oh!

rdmarsh2 commented Jan 12, 2024

Uh oh!

MathiasVP left a comment

Uh oh!

MathiasVP Jan 16, 2024

Uh oh!

MathiasVP Jan 24, 2024

Uh oh!

MathiasVP Jan 16, 2024

Uh oh!

rdmarsh2 Jan 22, 2024

Uh oh!

MathiasVP Jan 23, 2024

Uh oh!

MathiasVP Jan 16, 2024

Uh oh!

MathiasVP Jan 16, 2024

Uh oh!

MathiasVP left a comment •

edited

Loading

Uh oh!

MathiasVP Jan 24, 2024

Uh oh!

rdmarsh2 commented Jan 24, 2024

Uh oh!

MathiasVP left a comment

Uh oh!

jketema Jan 24, 2024

Uh oh!

rdmarsh2 Jan 24, 2024 •

edited

Loading

Uh oh!

jketema Jan 24, 2024

Uh oh!

Uh oh!

	/**
	* `destructor_call` destructs the `i`'th entity that should be
	* destructed following `element`. Note that entities should be
	* destructed in reverse construction order, so for a given `element`
	* these should be called from highest to lowest `i`.
	*/

C++: First-class destructors in AST #15318

C++: First-class destructors in AST #15318

Uh oh!

Conversation

rdmarsh2 commented Jan 12, 2024

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MathiasVP left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdmarsh2 commented Jan 24, 2024

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdmarsh2 Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MathiasVP left a comment •

edited

Loading

rdmarsh2 Jan 24, 2024 •

edited

Loading