Constexpr variable with non-trivial constexpr destructor can generate global/exit-time destructor warnings

[pkasting]

struct S {
  constexpr ~S() {}  // Intentionally not =default so as to be non-trivial
};
constexpr S s;

Compiling with -std=c++20 -Wglobal-constructors -Wexit-time-destructors triggers both those warnings (see https://godbolt.org/z/9xbKK4Gr1). This seems overly pessimistic; per https://en.cppreference.com/w/cpp/language/constexpr, "If a variable has constant destruction, there is no need to generate machine code in order to call destructor for it, even if its destructor is not trivial.", and indeed there seems to be no generated destructor call here despite the warnings. So I think the warnings are being emitted erroneously.

[tbaederr]

Both of these are emitted in Sema::FinalizeVarWithDestructor() in lib/Sema/SemaDeclCXX.cpp.

If this can simply be ignored for constexpr variables, this should be easy to fix.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Assign the issue to you.
2. Fix the issue locally.
3. Run the test suite locally.
   3.1) Remember that the subdirectories under test/ create fine-grained testing targets, so you can
   e.g. use make check-clang-ast to only run Clang's AST tests.
4. Create a git commit
5. Run git clang-format HEAD~1 to format your changes.
6. Submit the patch to Phabricator.
   6.1) Detailed instructions can be found here

For more instructions on how to submit a patch to LLVM, see our documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment on this Github issue.

@llvm/issue-subscribers-good-first-issue

[danakj]

I believe that sentence is talking about inside try blocks, since exceptions can not be thrown in constexpr expressions. It's part of this statement:

Even though try blocks and inline assembly are allowed in constexpr functions, throwing exceptions or executing the assembly is still disallowed in a constant expression.

So that if a constexpr function is evaluated in a constant expression, the destructors do not need to be generated at throw.

Constexpr objects are destroyed and can have non-trivial destructors since C++20, they need to run generally.

[danakj]

Chatted offline with pkasting. Here's the spec from c++20:

7 An object a is said to have constant destruction if:
(7.1) — it is not of class type nor (possibly multi-dimensional) array thereof, or
(7.2) — it is of class type or (possibly multi-dimensional) array thereof, that class type has a constexpr destructor,
and for a hypothetical expression E whose only effect is to destroy a, E would be a core constant
expression if the lifetime of a and its non-mutable subobjects (but not its mutable subobjects) were
considered to start within E.

Here the S has constant destruction as its destructor does not do anything to make it not evaluate as a core constant expression.

Then, as the variable was initialized at compile time, it does not need to be destroyed at runtime. I agree with Peter's analysis and these checks are incorrect. No machine code should be generated for the destructor.

In fact, it would seem that to do otherwise can introduce bugs. If the destructor has if (std::is_constant_evaluated()) which evals to false when the destructor was run, it may try to clean things up that do not match with the constructor which was run in a constant expression and thus would have seen if (std::is_constant_evaluated()) eval to true.

[shafik]

I am looking at the check in Sema::FinalizeVarWithDestructor and it looks like:

if (!VD->hasGlobalStorage()) return;

  // Emit warning for non-trivial dtor in global scope (a real global,
  // class-static, function-static).
  Diag(VD->getLocation(), diag::warn_exit_time_destructor);

  // TODO: this should be re-enabled for static locals by !CXAAtExit
  if (!VD->isStaticLocal())
    Diag(VD->getLocation(), diag::warn_global_destructor);

should be moved into the block before:

// If the destructor is constexpr, check whether the variable has constant
  // destruction now.
  if (Destructor->isConstexpr()) {

that block was added in 2b4fa53 by @zygoloid it feels like this was overlooked in that change.

[zygoloid]

Yeah, I think this was overlooked. There are three distinct but not entirely separate concerns here:

1. If a variable has a constexpr destructor, we try to evaluate its destruction ahead of time, and if we succeed then we won't emit any code to actually destroy it. Note that this happens regardless of whether the variable is constexpr.
2. If a variable is declared constexpr, then it must have constant destruction.
3. If a global variable does not have constant destruction, then we should emit a warning.

It seems to me that the checking for (3) is missing the "does not have constant destruction" part. This should do the trick:

-  if (!VD->hasGlobalStorage()) return;
+  if (!VD->hasGlobalStorage() || !VD->needsDestruction(getContext())) return;

[pkasting]

https://reviews.llvm.org/D149713