[PATCH] Do not split variables definitions into 2 DIEs.

[David Blaikie]

On Tue, Oct 21, 2014 at 3:23 PM, Frédéric Riss <friss at apple.com> wrote:

>
> On Oct 21, 2014, at 2:09 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, Oct 21, 2014 at 2:01 PM, Frédéric Riss <friss at apple.com> wrote:
>
>>
>> On Oct 20, 2014, at 2:06 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> I've checked this out on the GDB test suite & it seems fine.
>>
>> Looking at the code itself, I think it could be made a bit more
>> obvious/uniform to have VariableDIE always refer to the definition and have
>> just a single conditional to decide whether to attach the
>> DW_AT_specification or not.
>>
>> Actually this works pretty well & even adds another source fidelity
>> benefit, allowing the definition, while out-of-line of the class, to be
>> attributed to the scope in which the definition occurs (eg: namespace x {
>> struct foo { static int i; }; int foo::i; } - putting the definition in
>> namespace 'x' instead of just forcing it out to the global namespace) and
>> exposes a bug in Clang where the definition is scoped to the decl context
>> instead of the lexical context. (patch attached to show a slightly
>> short-sighted fix there)
>>
>>
>> Yeah, this looks an even nicer cleanup. I re-tested it just to be sure.
>> Can you just commit what’s in your branch, or do you want me to reroll your
>> reworked patch with a proper commit message?
>>
>
> It'll need the clang change first, or we'll end up putting the static
> variable's definition inside the class (DW_AT_class_type with both a
> DW_AT_member and a DW_AT_variable as children, both describing the static
> member variable, with the latter pointing to the former via
> DW_AT_specification) & I'll need to fix/work up test coverage for it (so if
> you want to do those things instead, I won't complain - but I can do them).
>
>
> Oh that’s what it does, sorry, I thought you were just talking about
> file/line attribution of the definition… Interestingly it doesn’t seem to
> bother lldb much, but it’s obviously too ugly to live! The file/line issue
> is of course related. I thought it was the backend grabbing the wrong
> information from the Decls, but it’s in fact the frontend passing the wrong
> information down.
>
> What was your thinking on the scoping issue in the frontend? It's
> certainly not the full solution - the full solution, imho would be to emit
> a declaration (potentially the DW_AT_member in the class) whenever the
> lexical context and the decl context differ. This would be the precise
> source fidelity. (& in the backend, do the right thing for fields that
> differ between the two - so we can describe the location of the definition
> in the .cpp file as different from the location of the declaration in the
> .h file, etc)
>
>
> Well, you have sent out the wrong patch for the frontend part, thus I
> misunderstood the exact issue you were trying to solve. If you send the
> right patch I can try to make sense of it and get an opinion on the matter.
>

Oh, sorry - when you said "wrong patch" I thought you meant "wrong
approach" rather than "some compeltely unrelated code"... I'm not sure
where the original patch went, but here's what it looks like:

diff --git lib/CodeGen/CGDebugInfo.cpp lib/CodeGen/CGDebugInfo.cpp
index 723da71..1fcf5ba 100644
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3177,8 +3177,9 @@ void
CGDebugInfo::EmitGlobalVariable(llvm::GlobalVariable *Var,
   if (LinkageName == DeclName)
     LinkageName = StringRef();

-  llvm::DIDescriptor DContext =
-    getContextDescriptor(dyn_cast<Decl>(D->getDeclContext()));
+  llvm::DIDescriptor DContext = getContextDescriptor(
+      dyn_cast<Decl>(D->isStaticDataMember() ? D->getLexicalDeclContext()
+                                             : D->getDeclContext()));

   // Attempt to store one global variable for the declaration - even if we
   // emit a lot of fields.


As I said, this is a bit of a hack to get the static data member stuff
right, but in the end we should probably emit decl+def for any global
variable who's decl context is different from its lexical context.


>
> Fred
>
>  - David
>
>
>> Thanks!
>>
>
> Thanks for your patience,
>
> - David
>
>
>> Fred
>>
>> If we did a few more smart things in the frontend, we could get better
>> fidelity for declarations/definitions of namespace/global variables -
>> removing the weird special case introduced with my patch. Probably the
>> right thing here is to optimize in the frontend, if the definition appears
>> in the decl context (eg: if the global variable's decl context is the same
>> as the lexical context) then just produce the definition. If the decl and
>> lexical context differ, produce both a declaration (in the decl context)
>> and a definition (in the lexical context) - this would be the best source
>> fidelity, but probably isn't too worthwhile.
>>
>> Oh, and if we wanted to be really good here, we could make sure we emit
>> any attributes onto the definition that differ from the declaration - note
>> GCC does the right thing here, putting DW_AT_decl_line/file on the
>> definitions if the line/file happens to be different from the declaration's
>> line/file. We don't do that - we just put nothing on the definition (& I
>> don't think we do this for function declarations/definitions either (again,
>> here, GCC gets this right)... could, but probably not important)
>>
>>
>>
>> On Thu, Oct 16, 2014 at 8:46 AM, Frederic Riss <friss at apple.com> wrote:
>>
>>> ping
>>>
>>> http://reviews.llvm.org/D5457
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
>> <static_var_def.diff><global_var.diff>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141021/7f70a995/attachment.html>