[PATCH] D62167: CodeView - add static data members to global variable debug info.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added subscribers: manmanren, probinson.
rnk added inline comments.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4385
+    // Use the global scope for static members.
+    DContext = getContextDescriptor(
+       cast<Decl>(CGM.getContext().getTranslationUnitDecl()), TheCU);
----------------
akhuang wrote:
> akhuang wrote:
> > dblaikie wrote:
> > > akhuang wrote:
> > > > @dblaikie I'm using the global scope here because if the class type is used as the scope it runs into the [[ https://github.com/llvm/llvm-project/blob/a2ee80b084e5c0b20364ed4379384706f5e059b1/llvm/lib/IR/DIBuilder.cpp#L630 | assert message ]] `Context of a global variable should not be a type with identifier`. Is there a reason for the assert? 
> > > I think it's generally how LLVM handles definitions for the most part - putting them at the global scope.
> > > 
> > > Though I'm confused by this patch - it has a source change in clang, but a test in LLVM. Generally there should be testing to cover where the source change is, I think?
> > yep, there should be a test for this - added now. 
> So for the global scope, it seems like [[ https://github.com/llvm-mirror/clang/blob/900624ef605b60c613342dac071228539a402ce9/lib/CodeGen/CGDebugInfo.cpp#L3274 | elsewhere ]], when creating the debug info for a static data member global variable, the scope is set to be the global scope. But it would be helpful for codeview debug info if the scope remained as the class type, partially so we can use the class type information in the variable name that we emit.
> 
> Does it seem reasonable to remove the assert for global variable scope so that the scope can be the class here?
I think the assertion in question is this one here in DIBuilder:
https://github.com/llvm/llvm-project/blob/master/llvm/lib/IR/DIBuilder.cpp#L634

It looks like @manmanren added this in 2014:
https://github.com/llvm/llvm-project/commit/bfd2b829d912ecd89f73ae32d4c683856dd677a3

@dblaikie do you know if this check is still necessary? It seems like the DWARF clang generages today corresponds to C++ written this way:
```
struct Foo { static const int Test2; };
const int Foo::Test2 = 4;
```
When oftentimes the source looks more like this:
```
struct Foo { static const int Test2 = 4; };
```
I saw a conversation between you and @probinson fixing clang irgen to avoid this assert sometime back, so I figured you might be a good point of reference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62167