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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Is this the change we were discussing the other week semi-related to preserved enums? It sounded to me like the conclusion was somewhat to go ahead without adding all these things to the constant list and see how it goes? Is this case different from the enum case in some way?



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4385
+    // Use the global scope for static members.
+    DContext = getContextDescriptor(
+       cast<Decl>(CGM.getContext().getTranslationUnitDecl()), TheCU);
----------------
rnk wrote:
> 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.
My expectation is that this characterization is probably a pragmatic one of "it's what GCC does/we're not sure if GDB could cope with it otherwise" (we don't always adhere strictly to what GCC does - for instance GCC always puts function definitions at the CU scope, and if the function was defined in a namespace it puts a declaration in the namespace and references that from the global scoped definition - Clang goes the other way and puts the definition in the namespace scope (even if the function was declared there but defined from outside the namespace))

If that's the case (worth testing) then arguably the frontend should emit the more accurate metadata and the DWARF backend could fudge this - or like some of the other stuff we do we could emit different metadata in the frontend depending on the DWARF v CodeView split.


On further experimentation, I'm somewhat confused what this change is about. In ToT, this code:

  struct foo {
    static const int bar = 3;
  };
  int f() {
    return foo::bar;
  }

Becomes DWARF that has a DICompositeType with elements pointing to a DIDerivedType:DW_TAG_member with the extraData of i32 3.

And that becomes this DWARF:

  TAG_structure_type
    AT_name "foo"
    TAG_member
      AT_name "foo"
      declaration true
      const_value 3

which seems accurate to me - this isn't a variable definition, it's a declaration with a value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62167