[LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication

[David Blaikie]

On Wed, Oct 16, 2013 at 1:21 PM, Manman Ren <manman.ren at gmail.com> wrote:

>
> There are a few places where we break the assumption:
> 1> formal_parameter constructed in DwarfDebug when adding attribute type
>      we call SPCU->addType(Arg, ATy), where Arg does not belong to SPCU.
> 2> inlined_subroutine constructed in DwarfDebug when adding attribute
> abstract_origin
>      The inlined_subroutine does not belong to the CU we call addDIEEntry
> on.
>      We create the children of inlined_subroutine and call addDIEEntry
> before we add it to an owner.
> 3> ...
>
> When building xalan with "lto -g", I saw 3158 violations of the
> assumption. Should we try to fix all these violations to make the
> assumption true?
>

As you've said, this assumption (that DIEs are always constructed by the CU
that will own them) isn't necessary for this patch - it will be necessary
for cu-at-a-time emission. Perhaps we should discuss it in a separate
thread rather than trying to make a stronger assumption than is needed for
this patch.

Of course without your patch the assumption is trivially correct (well,
given your point (1) above, perhaps it wasn't - maybe we do do things out
of order and from outside the CU) and we might want to say that it's so
important that your patch shouldn't violate it - but I'm not sure that's
necessary (Eric?). I haven't given it a great deal of thought.


> As I stated earlier, this assumption, in my opinion, is not really
> general, and if we only make this assumption to remove a worklist, it may
> not worth it.
>

There is still no discussion about "removing the worklist" - the only
discussion is about whether we need to /add/ the worklist. It's up to the
commiter to justify the addition of code, not for the community to justify
its removal. (I'd say this is /almost/ even true of committed code - if
something isn't tested, I'm likely to remove it and see what breaks - if
someone cared about it, they should've provided tests for it).


> How can developers make sure this assumption is not later violated?
>

We can add the kind of assertions we've discussed in this thread, keeping a
mapping every time we rely on the assumption and checking the mapping
whenever we resolve the parent chain. I don't think this is necessary, but
I wouldn't block such a change either.


> If we are going to make assumptions,
>

We are always going to make assumptions - well, we're always going to have
invariants in our design where any violation of those invariants is a bug.


> I will suggest making general assumptions such as
> 1> always use a CU to construct a DIE
> 2> Before constructing a DIE, figure out the owner CU first by chasing the
> context chain, then call construction on the owner CU.
> 3> Before calling addDIEEntry in DwarfDebug, find the owner CU first, then
> call addDIEEntry on the owner CU.
>

I'm not sure how valuable this would be - perhaps highly, perhaps not. It'd
require more work for your patch, though.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131016/636e81ab/attachment.html>