This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Support destructors
ClosedPublic

Authored by tbaeder on Oct 31 2022, 4:24 AM.

Details

Summary

Try to use the existing cleanup machinery to implement calling destructors.

Diff Detail

Event Timeline

tbaeder created this revision.Oct 31 2022, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 4:24 AM
tbaeder requested review of this revision.Oct 31 2022, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 4:24 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 471971.Oct 31 2022, 5:01 AM
shafik added inline comments.Dec 20 2022, 5:33 PM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
30

Why the change from LocalScope to VariableScope?

clang/lib/AST/Interp/ByteCodeExprGen.h
369
clang/test/AST/Interp/cxx20.cpp
279
tbaeder marked 2 inline comments as done.Dec 21 2022, 1:54 AM
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
30

I changed VariableScope to do the destructor handling, bu that's not needed for "temporaries in toplevel variable desclarations" (also, but unrelated, inherting from something called "local scope" but then explaining in a comment that it's only used for toplevel declarations is kinda weird).

tbaeder updated this revision to Diff 484500.Dec 21 2022, 1:55 AM
shafik accepted this revision.Dec 21 2022, 1:16 PM

LGTM

This revision is now accepted and ready to land.Dec 21 2022, 1:16 PM

Looks like I have to actually go up the class chain and emit all destructors manually.

tbaeder updated this revision to Diff 486525.Jan 5 2023, 4:42 AM
tbaeder marked an inline comment as done.
tbaeder set the repository for this revision to rG LLVM Github Monorepo.
aaron.ballman requested changes to this revision.Jan 18 2023, 6:14 AM
aaron.ballman added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
1513–1515

As a FIXME: you should also handle virtual destructors at some point, whenever you get around to handling virtual functions in general.

1528–1529

isTrivial() only works once the class has been fully built up by Sema IIRC; we should have a test case for that situation.

1530

This looks like it will destroy the array elements in order instead of in reverse order -- need test coverage for that. See https://eel.is/c++draft/class.dtor#13.sentence-5

1547

Won't this also destroy static data members? (Needs a test case for that.)

Also, what if the record is a union and not a structure? We only want to destroy the active member in that case, not all of the variant members, right? (Also needs a test case.)

See http://eel.is/c++draft/class.dtor#13

1571–1576

I don't think we handle virtual functions yet so I doubt we handle virtual bases, but that's something that might need a FIXME comment.

clang/lib/AST/Interp/ByteCodeExprGen.h
343

No need to add the virtual here as the override already signals that (can't override a non-virtual function).

This revision now requires changes to proceed.Jan 18 2023, 6:14 AM
tbaeder marked 3 inline comments as done.Jan 24 2023, 4:19 AM
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
1528–1529

Are you saying that isTrivial() cannot be used like this, or just that it can, but needs a test case to ensure that this is true?

Also, how would such a test case look like?

1530

Right, that makes a lot of sense, good catch.

1547

I saw

// A union destructor does not implicitly destroy its members.
if (RD->isUnion())
  return true;

in ExprConstant.cpp, but since we don't handle unions at all in the new interpreter right now, I didn't add anything for them. I added a comment.

tbaeder updated this revision to Diff 491709.Jan 24 2023, 4:19 AM
aaron.ballman added inline comments.Feb 2 2023, 10:48 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
1528–1529

Sema::DeclareImplicitDestructor() decides whether the destructor is trivial or not, and that is based on information that the class collects as the class is being declared. While the class is being parsed, the DeclarationData for the class is updated as we go and we use that to decide if we need the destructor, whether it's trivial, etc. So it's possible for us to have not seen a part of the class yet that would cause the special member function to be (non)trivial and so asking the method "are you trivial" may give a different answer depending on when the question is asked.

In terms of a test case, I think it would be trying to hit one of these cases http://eel.is/c++draft/class.mem#class.dtor-8 by using a constexpr function that needs to be evaluated before we get to something that causes the dtor to no longer be trivial.

tbaeder marked an inline comment as done.Feb 3 2023, 1:17 AM
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
1528–1529

Hm, I can't come up with a reproducer for this. The class of a member variable must be fully defined when the member is declared, so I can't forward-declare it and then introduce a non-trivial destructor later. And as soon as I add a destructor declaration (and try to define i later), the destructor is automatically not trivial anymore.

aaron.ballman accepted this revision.Feb 3 2023, 8:05 AM

LGTM assuming no surprises with the new test request.

clang/lib/AST/Interp/ByteCodeExprGen.cpp
1528–1529

Yeah, I'm struggling to make a test case as well, so let's move on.

clang/test/AST/Interp/cxx20.cpp
388

Another test case that I think would be interesting is with a static member that is not constexpr (to show it doesn't cause problems) and an out-of-line destructor just because it's a bit of an oddity: https://godbolt.org/z/YqrPMEEr4

This revision is now accepted and ready to land.Feb 3 2023, 8:05 AM
tbaeder updated this revision to Diff 494660.Feb 3 2023, 8:24 AM
tbaeder marked an inline comment as done.

No surprises I think.

This revision was landed with ongoing or failed builds.Mar 5 2023, 1:08 AM
This revision was automatically updated to reflect the committed changes.