[PATCH] D156453: [clang][Interp] Create only globals when initializing a global variable

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1213
   std::optional<PrimType> SubExprT = classify(SubExpr);
-  if (E->getStorageDuration() == SD_Static) {
+  bool IsStatic = E->getStorageDuration() == SD_Static;
+  if (GlobalDecl || IsStatic) {
----------------
aaron.ballman wrote:
> tbaeder wrote:
> > aaron.ballman wrote:
> > > Should we be looking at the TLS kind of the extended declaration? (`CheckCompleteVariableDeclaration()` is using `VarDecl::getTLSKind() == VarDecl::TLS_Static`)
> > > 
> > > Would something along these lines work instead?
> > > ```
> > > bool EmitGlobalTemp = E->getStorageDuration() == SD_Static;
> > > if (!EmitGlobalTemp) {
> > >   if (const LifetimeExtendedTemporaryDecl *LETD = E->getLifetimeExtendedTemporaryDecl()) {
> > >     if (const auto *VD = dyn_cast_if_present<VarDecl>(LETD->getExtendingDecl()) {
> > >       EmitGlobalTemp= VD->getTLSKind() == VarDecl::TLS_Static;
> > >     }
> > >   }
> > > }
> > > ```
> > That code definitely works for the current `AST/Interp/` tests, but we don't have tests for thread local stuff in there right now.
> Hmm, I think we'll need those tests: https://eel.is/c++draft/expr.const#5.2
> 
> That seems to be the only mention about thread local storage duration for constant expressions in C++, so it might make sense to tackle that as part of this change?
> 
> (I worry that we'll forget to come back to this detail later, basically. So either we should have failing test coverage showing we need a fix, an issue in GitHub so we know to come back to it, etc. or just do the work up front given that it's closely related.)
I've pushed https://github.com/llvm/llvm-project/commit/11f5e5eb90c883d4b9ddba318e8fc57914b22ef3

As you can see, the problem also exists for static variables. I think this needs additional checks at interpretation time, or even new opcodes to check the declaration when the initializer is computed. So I'd rather have this in a separate followup patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156453/new/

https://reviews.llvm.org/D156453