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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
