[PATCH] D11180: Fixed debug info generation for function static variables, typedef, and records

[David Blaikie via llvm-commits]

On Thu, Aug 27, 2015 at 10:39 PM, Amjad Aboud via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> aaboud added a comment.
>
> > Right, what I'm suggesting is that part could be done separately from
> the scope handling stuff.
>
>
> I understand the benefit of splitting code into as much patches as
> possible.
>
> > This change would ensure that the DIEs go in the right function (the
> abstract one or the concrete one) - which would fix bugs that occur when
> there is no concrete DIE, if I understand correctly?
>
>
> Actually, what we agreed on that the DIEs would go to abstract one if it
> exists, otherwise to the concrete. So if we have abstract and concrete the
> DIE will be only in the abstract.
> And this is what I implemented above.
>
> > For example, currently if you have a static local variable in a function
> that is inlined we produce an abstract definition, an inlined subroutine,
> but then we also produce this weird stub-concrete definition (a subprogram
> tag with /only/ an abstract_origin attribute, and the child DW_TAG_variable
> for the static variable). If we delay the addition of the local types and
> static variables and just put them in the abstract definition if there is
> one, we shouldn't end up producing the weird stub concrete definition
> (that's not really a concrete definition anyway).
>
>
> But the “currently” you are mentioning is not true anymore, once we
> changed the design to create the DIE without attaching it to a scope, the
> weird stub-concrete definition is not created anymore.
> So, unless we handle these DIEs at end of module processing and put them
> in the correct scope (abstract or concrete), we will end up having flying
> (unattached) DIEs.
>
> > So that would be a good, separable, incremental step. /then/ we can have
> a separate (3rd) patch or patches to try to put it in the right scopes?
>
>
> We improved the design, there is no reason to go one step back, just to
> commit two patches that none of them make sense as itself.
>

I think they do make sense by themselves - one of them fixes a smaller
existing bug, the next one improves things after we have a good foundation
for those improvements. Much like the series of patches I made last year to
improve inline debug information - a lot of it was figuring out where I
needed to go, then working backwards to figure out the steps that would
best get me there.

This isn't uncommon review feedback either - we often try to break up
larger changes to ease review and keep the commit history (& also the
revert history) simple  and easy to follow.

I'd be happy to work with you to help with this - I can work up some/all of
these patches & we can review them together if you prefer.

- Dave


>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D11180
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150827/71bc04df/attachment.html>