[PATCH] D89286: [DebugInfo] Check for templated static data member when adding constant to record static fields

Amy Huang via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 14 13:05:35 PDT 2020


akhuang added a subscriber: rsmith.
akhuang added inline comments.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1421-1425
+  else if (auto *TemplateDecl = Var->getInstantiatedFromStaticDataMember()) {
+    // Inline static data members might not have an initialization.
+    if (TemplateDecl->getInit())
+      Value = TemplateDecl->evaluateValue();
+  }
----------------
dblaikie wrote:
> I'd suspect this might not be the most general solution - because it's retrieving the value from the original template static member. Two issues come to mind:
> 1) If the initialization is dependent or otherwise not knowable at compile-time, does this code at least not crash (I assume "evaluateValue" returns nullptr if the value can't be evaluated for whatever reason)
> 2) If the initialization is dependent, it might still be knowable, eg:
> 
> ```
> template<typename T>
> struct static_decl_templ {
>   static constexpr int static_constexpr_decl_templ_var = sizeof(T);
> };
> ```
> 
> I'm guessing this patch as-is doesn't provide a value in this case, but it should be achievable, I would guess/hope/imagine - but don't really know exactly how to do that off-hand. (@zygoloid would know - but maybe a bit of looking around will find it if/before he has a chance to weigh in here)
> 
> 
@rsmith 

Actually, can we just instantiate the initializer for static constexpr data members? 

currently [[https://github.com/llvm/llvm-project/blob/683b308c07bf827255fe1403056413f790e03729/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L5062| it delays creating the initializer for inline static data members ]]; can we not do that if it's a constexpr?


================
Comment at: clang/test/CodeGenCXX/debug-info-static-member.cpp:140-144
+#if __cplusplus >= 201103L
+  static constexpr int static_constexpr_decl_templ_var = 8;
+#else
+  static const int static_constexpr_decl_templ_var = 8;
+#endif
----------------
dblaikie wrote:
> I guess the non-11 version of this variable is intended as a stub to make the test across different language levels simpler?
> 
> I think it might be better to make that more explicit - rather than having a variable with "constexpr" in the name that isn't actually constexpr. I'd say only add the variable under that macro condition - but change the test to use multiple --check-prefixes, and the check for this variable to use a CPP11 prefix or something like that.
> 
> Hmm, I'm slightly confused now that I Think about it - it looks like the macro is about testing if the version is >= 11. But the change also adds a test for C++17? Is C++17 likely to be any different than C++11 for any of this? (or C++20 for that matter) if it's not any different, I'd probably leave the 17 case out & expect the 11 case is sufficient coverage.
Yeah, it was intended as a stub, but I can remove that part.

I added c++17 because it appears that in c++17 (and for MS targets) we make constexpr data members implicitly inline, which is why the initializer is not being instantiated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89286



More information about the cfe-commits mailing list