[PATCH] Do not split variables definitions into 2 DIEs.
Frédéric Riss
friss at apple.com
Thu Oct 23 13:54:10 PDT 2014
> On 23 Oct 2014, at 12:23, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, Oct 22, 2014 at 8:31 PM, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>
>> On 21 Oct 2014, at 15:30, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>
>>
>>
>> On Tue, Oct 21, 2014 at 3:23 PM, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>>
>>> On Oct 21, 2014, at 2:09 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>>
>>>
>>>
>>> On Tue, Oct 21, 2014 at 2:01 PM, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>>>
>>>> On Oct 20, 2014, at 2:06 PM, David Blaikie <dblaikie at gmail.com <mailto: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.
>>
>
> OK, this works because for static members we get the declaration context from the type member which isn’t impacted by this change. I believe this is the best we can do with the current representation. This isn’t even too hacky, it accounts for a difference of representation between static members and other globals (but if committed, a comment would be in order).
>
> I feel it's a bit hacky because I don't think there's any reason our current IR representation wouldn't work fine for (non-member) global variable declarations, it's just that the frontend hasn't been taught to create global variable declarations. Once we have that, this code should do the obvious thing and just check for a difference between decl and lexical context, and it won't having anything in particular about static member variables at all, hopefully..
>
> Hmm, I'm a bit confused by GCC's output - I thought, at some point, I'd been able to tickle GCC into producing at least a DW_AT_line on the definition of a global variable (because the definition was on a different line from the declaration) but I can't seem to reproduce that behavior right now. It seems the definitions (for both static and non-member global variables) just contain DW_AT_specification, DW_AT_location, and DW_AT_linkage_name (well, it puts the linkage name on the definition for static data members, but it puts it on the declaration for non-member globals... *shrug*)
>
> So it seems we'll be about right - the source fidelity benefits of being able to describe a definition that appears in a lexical context that is different from its decl context certainly isn't valuable enough for me to lose sleep over/prioritize in any way...
>
>
>> 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.
>
> Either the frontend emits a decl/def pair when it is necessary, or it always provides both, and we let the backend decide when to split. The latter seems to be a (potentially big?) waste of space.
>
> Yeah, possible - I'm not too fussed either way. The check is probably cheaper/easier in the frontend, but it's simpler to have it in the backend. My guess would be that this isn't a huge waste of space, but we could check first if we wanted to go down that route.
>
>
> This patch originated from a series where I try to get the forward declarations right for imported entities (emitting fwd decls when necessary and merging them with the def when it occurs). It might be possible to leverage some of that to offer the fidelity you’re looking for: not merging decl and def if their context is different, and adding a getDecl() method returning the declaration to DIGlobalVariable (it could reuse the static member field in DIGlobalVariable, they are mutually exclusive).
>
> So all in all, I’d be in favour of your patch and try to revisit it once the forward declaration stuff is in.
>
> Yep, sounds good to me. I've left a fixme in to that effect (with the clang change in r220488).
>
> Main LLVM patch committed in r220497.
Thanks! I’ll try to rebase my forward decl patches in the next days.
Fred
> Thanks for your patience/discussion/patches/etc!
>
> - David
>
>
>
>
> Fred
>
>>
>> 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 <mailto:friss at apple.com>> wrote:
>>>> ping
>>>>
>>>> http://reviews.llvm.org/D5457 <http://reviews.llvm.org/D5457>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <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/20141023/922c90a5/attachment.html>
More information about the llvm-commits
mailing list