llvm dev - Oct 2013 - [LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication

On Wed, Oct 16, 2013 at 11:54 AM, Manman Ren <manman.ren at gmail.com>
wrote:
>
>
>
> On Wed, Oct 16, 2013 at 11:10 AM, David Blaikie <dblaikie at
gmail.com>wrote:
>
>>
>>
>>
>> On Wed, Oct 16, 2013 at 10:54 AM, Manman Ren <manman.ren at
gmail.com>wrote:
>>
>>>
>>>
>>>
>>> On Tue, Oct 15, 2013 at 5:59 PM, David Blaikie <dblaikie at
gmail.com>wrote:
>>>
>>>>
>>>>   In beginModule, we construct the CUs, but not all the DIEs
that
>>>>>>> belong to the CU.
>>>>>>> In endFunction, we started to construct the scope
DIEs. So in some
>>>>>>> sense, we are adding things to prior CUs.
>>>>>>>
>>>>>>> Looking at void CompileUnit::addDIEEntry(DIE *Die,
uint16_t
>>>>>>> Attribute, DIE *Entry), we can possibly have 3 CUs,
this CU,
>>>>>>> the CU Die belongs to (i.e. the CU the parent chain
points to), the
>>>>>>> CU Entry belongs to.
>>>>>>>
>>>>>>
>>>>>> When does "this CU" differ from both the Die
and the Entry?
>>>>>>
>>>>>
>>>>> The signature of addDIEEntry didn't say that "this
CU" should be the
>>>>> same as either Die's CU or Entry's CU.
>>>>>
>>>>
>>>> No, but the code prior to your patch that's the only really
sane thing
>>>> for it to do, right? Why would one CompileUnit be dealing with
some other
>>>> CU's DIEs?
>>>>
>>>> This is why we should carefully think about the new semantics
we're
>>>> introducing by doing cross-CU DIE references.
>>>>
>>>>
>>>>>  If you think it should not happen, we should put a comment
in so
>>>>> other developers will not violate it.
>>>>> But that is irrelevant if we are going to make a single
assumption:
>>>>>   "in addDIEEntry, if a DIE is without a parent, it
belongs to the CU
>>>>> we call addDIEEntry on".
>>>>>
>>>>
>>>> I'm not entirely sure that will be true. In the case of
implicit
>>>> special members, such as the copy ctor, what could happen is
that a later
>>>> CU might want to add a member function to an earlier CU. That
member
>>>> function DIE will be being constructed from within the latter
CompileUnit,
>>>> but its parent will be the former CU. Walking the parent chain
will find
>>>> the correct (older CU) because we build the context for a
member function
>>>> and attach the member function to that context before we build
any of its
>>>> parameters, template arguments, etc.
>>>>
>>>> Exactly, so it is possible that a later CU can construct a DIE
that
>>> belongs to another CU.
>>>
>>> I was rephrasing the statement in your earlier email and trying to
be
>>> sure what assumption we are going to make.
>>>   The claim Eric and I are making/discussed is that we should
assume
>>> any DIE without a parent is in the current CU and/or that it
possibly
>>> should never come up (I haven't thought through the codepaths
carefully
>>> here - if we add CUs to their parent before creating their members,
I think
>>> it should never come up).
>>>
>>> I know that the assumption may not be true with the current code
base,
>>> but we need to either
>>> 1> keep the worklist or
>>> 2> remove the worklist and modify the code base to make the
assumption
>>> true, right?
>>>
>>
>> I'm OK assuming it to be true until we show it to not be true and
>> treating those cases as bugs and fixing them on an as-found basis.
>>
>
> Okay, I attached a patch removing the worklist. Please review,
>
Will do.

>
>
>> I'm not OK adding the worklist without knowing and testing the
cases it
>> addresses.
>>
>>
>>>
>>> If we are going to make the assumption true, any suggestion on how
to
>>> fix the above cases?
>>>
>>
>> I don't know if the above cases are problems. Have you written and
>> verified any of the test cases I've described?
>>
>>
>>> One possibility is to find the correct CU in DwarfDebug, and call
>>> getOrCreate on the correct CU.
>>> In your above example, if a later CU wants to add a member function
to
>>> an earlier CU, we find the earlier CU in DwarfDebug by going
through the
>>> context chain of the member function, and then call getOrCreate on
the
>>> earlier CU.
>>>
>>> So we can try to make the statement "a CU only constructs a
DIE that
>>> belongs to the CU",
>>>
>>
>> I don't believe so - not based on your current implementation.
We'd need
>> to think more carefully about mismatches between type definitions
(implicit
>> special members, member template specializations, nested classes -
these
>> three things can create differences between definitions) and resolving
the
>> metadata so those mismatches don't exist.
>>
>> That means when we merge the tables of type metadata string identifier
to
>> MDNode from each CU is that we may need to modify the type metadata
nodes
>> to merge the child lists from all those CUs.
>>
>
> We don't do that.
>
Yes - see the next paragraph - /if/ we did that, then the "we only produce
DIEs from the CU that contains them" could be true. It won't be for
now,
with your change, unless we do that or something else.

> If we did that, perhaps we wouldn't have to retroactively add any DIEs
to
>> previous CUs, in which case your statement might become true.
>>
>
> Adding DIE to a previous CU happens with the current code base, but we can
> still try to implement "a CU only constructs a DIE that belongs to the
CU",
> with that, we can release all DIEs constructed by the CU when we are done
> emitting the CU.
> This is actually related to emit-one-CU-at-a-time, not so much on this
> patch.
>
That's correct - you were asking whether we could rely on the assumption
that "a CU only constructs a DIE that belongs to that CU" and I said
we
can't at the moment because of this situation.

When we move to emit-one-CU-at-a-time, we'll need to figure out how to
address this issue.

>
>
>>
>> For now, it isn't, so far as I can tell. (again, I'm giving you
some
>> pointers about how I believe some of these things work, but my
intention is
>> that these should give you ideas about what you need to test, ways you
>> might be able to find worklist cases, etc - this testing needs to be
done
>> to understand how your patch will interact with these systems)
>>
>>
>>> that will help emit-one-CU-at-a-time?
>>>
>>
>> If we do one-CU-at-a-time emission we're not going to be able to
make
>> debug info as compact as we can with your current change, not without
more
>> work, at least.
>>
>> Taking the implicit special member example, CU1 would have the
definition
>> of some type 'foo' and its normal members. CU2 has a
declaration of 'foo'
>> (or perhaps another definition, depending on whether it's a
polymorphic
>> type, etc) and the implicit special member definition (such as the copy
>> ctor). When we do the right type referencing and uniquing at the
metadata
>> level what's going to happen (I think) is we'll have one
definition of
>> 'foo', but its list of members will be that of the definition
(what happens
>> if we have more than one definition and they differ because they have
>> different lists of members? I guess we'll produce two definitions
of 'foo',
>> one in each CU that has the definitions because they'll be
different
>> metadata nodes).
>>
>
> We will have two DIEs corresponding to the two definitions.
>
Unless one is a declaration and the other'

> But when we actually refer to the definition via DIRef, we resolve to one
> of the two definitions.
> The member DIEs will be added to the single class that we resolve the
> DIRef to.
>
> Manman
>
>
>
>>  Since we didn't merge the member lists of the two
>> definitions/declarations of 'foo', the child list will be
incomplete, but
>> the context links in the members will all point to the definition -
magic.
>>
>> So when we visit the first definition, we'll build some of the
members,
>> but we won't add the other members until we're emitting the
other CU (I
>> hope?). If we've already emitted the previous CU we can't go
and add new
>> members to its elements - they're already written to disk and
we've thrown
>> out the DIEs (we'd probably just record a collection section
offsets for
>> specific DIEs).
>>
>>
>>> We also need to replace all "new DIE" in DwarfDebug with
CU::getOrCreate,
>>> then every DIE will be constructed by a CU and belong to the same
CU.
>>> When we are done emitting one CU, we can release all DIEs belonging
to the
>>> CU (maybe keep a minimal DIE if it is cross-CU referenced).
>>>
>>> This does not sound like a small project, I am hoping to have the
type
>>> uniquing patches in first, since I have an internal milestone for
that.
>>>
>> I will be working on the emit-one-Cu-at-a-time later and removing the
>>> worklist then.
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> When Die or CU does not have an owner, what
assumption should we make
>>>>>>> to decide between ref_addr and ref4 inside
addDIEEntry?
>>>>>>>
>>>>>>
>>>>>> The claim Eric and I are making/discussed is that we
should assume
>>>>>> any DIE without a parent is in the current CU and/or
that it possibly
>>>>>> should never come up (I haven't thought through the
codepaths carefully
>>>>>> here - if we add CUs to their parent before creating
their members, I think
>>>>>> it should never come up).
>>>>>>
>>>>>
>>>>> Does "current CU" mean the CU we are calling
addDIEEntry on?
>>>>>
>>>>
>>>> Yes, I think that's a reasonable interpretation.
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> If we assume that if Die or Entry does not have an
owner, it should
>>>>>>> belong to this CU, it means:
>>>>>>> 1> Die or Entry should be constructed by this
CU.
>>>>>>> 2> DIEs that are constructed by CU A should
belong to CU A.
>>>>>>> 3> About DIEs that are constructed directly in
DwarfDebug, they
>>>>>>> should be added to a CU before calling addDIEEntry.
>>>>>>>      Another option is to wrap all DIE
constructions in a CU, which
>>>>>>> sounds reasonable for emit-a-CU-at-a-time.
>>>>>>>
>>>>>>
>>>>>> I'm not really sure what you mean by "wrap all
DIE constructions in a
>>>>>> CU". Even with your change the DIEs are
constructed by CUs and just added
>>>>>> to the appropriate cache (either CU-local, or
cross-CU), they're never
>>>>>> constructed outside of a CU, are they?
>>>>>>
>>>>> I am referring to the "new DIE" in
DwarfDebug.cpp, and saying that we
>>>>> should instead call getOrCreate function on a CU.
>>>>>
>>>>
>>>> Perhaps I'll need to go back and check your patch, but
generally, I
>>>> tend to think we should be calling into a CU to build anything
ratehr than
>>>> doing it 'raw' in DwarfDebug.cpp.
>>>>
>>>
>>> It is not related to my patch, we are directly constructing DIEs in
>>> DwarfDebug in the current code base.
>>>
>>> Thanks,
>>> Manman
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> How much effort is required to make sure these
assumptions are true?
>>>>>>> We also should have the corresponding assertions to
make sure the
>>>>>>> assumptions are not violated.
>>>>>>>
>>>>>>
>>>>>> Personally, I'm OK saying "this is the
intended design, anything else
>>>>>> is a bug" and leaving it at that. If we want to
add in the necessary
>>>>>> infrastructure to assert this is the case, I'm OK
with that too.
>>>>>>
>>>>>
>>>>> Okay, let's go with the assumption that "in
addDIEEntry, if a DIE is
>>>>> without a parent, it belongs to the CU we can addDIEEntry
on".
>>>>>
>>>>
>>>> Above counter-example I believe to be true. You should verify
that and
>>>> other such examples, please.
>>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> To implement the assertions, we need to keep a list
of all DIEs that
>>>>>>> are constructed by a CU.
>>>>>>>
>>>>>>
>>>>>> It shouldn't be hard - we'd add a map of
assumptions "we found this
>>>>>> DIE without a parent and assumed it was in this
CU" then whenever we add a
>>>>>> DIE to a parent we would check that map and remove the
entry (assert-fail
>>>>>> if there was a mismatch between assumption and
reality).
>>>>>>
>>>>>
>>>>> Yes that will verify the single assumption. So in
addDIEEntry, if a
>>>>> DIE (either Die or Entry) has no parent, add it to a map,
and in
>>>>> DIE::addChild, verify the assumption is true and remove it
from the map.
>>>>>
>>>>>
>>>>>>
>>>>>>> With the ConstructedDIEs, we can verify 1> in
addDIEEntry, 2> in
>>>>>>> finalize 3> by wrapping all DIE constructions in
a CU.
>>>>>>>
>>>>>>> I am wondering whether we should make the type
uniquing patches
>>>>>>> depend on the invariants/assumptions.
>>>>>>>
>>>>>>
>>>>>> If the alternative is introducing defensive and
unjustified/untested
>>>>>> complexity, yes - we must. We cannot tolerate
introducing unjustified
>>>>>> complexity into the codebase - it's the only way we
can strive to move the
>>>>>> code in a maintainable direction (I contend that
it's not maintainable at
>>>>>> the moment and it isn't because of changes like
this - adding more is
>>>>>> untenable, removing them is admirable, but hard and
something we'll be
>>>>>> working on for a long time to come).
>>>>>>
>>>>>
>>>>> I agree that the backend is not that clean and we should
try to clean
>>>>> it up. But that single assumption seems not really generic.
Instead I think
>>>>> "DIEs that are constructed by CU A should belong to CU
A" is more generic
>>>>> and it may help us in emit-one-CU-at-a-time.
>>>>>
>>>>> Manman
>>>>>
>>>>>
>>>>>>
>>>>>> - David
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Manman
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> The CU constructing a DIE will add the DIE
to the context owner,
>>>>>>>>> which may not belong to the CU itself.
>>>>>>>>>
>>>>>>>>
>>>>>>>> That's the question, isn't it? When is
it ever /not/ the current CU
>>>>>>>> under construction?
>>>>>>>>
>>>>>>>>
>>>>>>>>>  In the case that a DIE is added to an
owner in DwarfDebug, I
>>>>>>>>> don't think we should try to enforce
that DwarfDebug will add the DIE to
>>>>>>>>> the CU that constructed the DIE earlier.
>>>>>>>>>
>>>>>>>>
>>>>>>>> My claim is that this is already an invariant.
That the code you
>>>>>>>> are adding is never needed and thus is
additional, unjustified/unused
>>>>>>>> complexity that comes at a cost to all future
developers/development. This
>>>>>>>> codebase needs /much/ less of this than it
already has, let alone adding
>>>>>>>> more of it. Though I wouldn't mind an
assertion, I suspect it'll be more
>>>>>>>> code than is really worth it to keep track of
this information/state.
>>>>>>>>
>>>>>>>> I think if we more to CU-at-a-time DWARF
emission it'll be more
>>>>>>>> obvious that this invariant is true and cannot
be violated.
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>  If we can demonstrate a case where
this isn't true, then we
>>>>>>>>>> should work to address that problem -
until we demonstrate that, we should
>>>>>>>>>> not (though we might want to search for
such cases - but without type units
>>>>>>>>>> I can't imagine how they could
occur - we build the DIE tree for one CU at
>>>>>>>>>> a time, at no point do we have DIEs
under construction for multiple CUs).
>>>>>>>>>>
>>>>>>>>>> So if we want to build a reference to a
DIE, if the DIE is not in
>>>>>>>>>> another CU we should use ref4. (then
the only other case is that it's
>>>>>>>>>> either in this CU or it's in no
known CU at all - in which case it's under
>>>>>>>>>> construction and, without evidence to
the contrary, will be added to the
>>>>>>>>>> current CU when it's done).
>>>>>>>>>>
>>>>>>>>>> About the only assertion we could add
would involve keeping a
>>>>>>>>>> side-table of "assumptions"
("we expect this DIE will be added to this CU")
>>>>>>>>>> and check that those assumptions are
fulfilled at some point.
>>>>>>>>>>
>>>>>>>>> In my opinion, if we can't verify the
assumption with a reasonable
>>>>>>>>> amount of effort, then we should not make
the assumption.
>>>>>>>>>
>>>>>>>>
>>>>>>>> This leads to unbounded, unjustified complexity
that makes the
>>>>>>>> codebase impossible to maintain. It just cannot
be an acceptable method of
>>>>>>>> development.
>>>>>>>>
>>>>>>>> - David
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Manman
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> For that case, we can't assume
ref4 should be used. I don't
>>>>>>>>>>> think we can enforce that all DIEs
must be added to a parent before calling
>>>>>>>>>>> addDIEEntry.
>>>>>>>>>>>
>>>>>>>>>>> For the specific testing case, when
constructing children of a
>>>>>>>>>>> scope DIE, we call addDIEEntry on
the children, after that, we add the
>>>>>>>>>>> children to the scope DIE.
>>>>>>>>>>> cat foo.cpp
>>>>>>>>>>>
>>>>>>>>>>> #include "a.hpp"
>>>>>>>>>>> void f(int a) {
>>>>>>>>>>>   Base t;
>>>>>>>>>>> }
>>>>>>>>>>> cat bar.cpp
>>>>>>>>>>>
>>>>>>>>>>> #include "a.hpp"
>>>>>>>>>>> void f(int);
>>>>>>>>>>> void g(int a) {
>>>>>>>>>>>   Base t;
>>>>>>>>>>> }
>>>>>>>>>>> int main() {
>>>>>>>>>>>   f(0);
>>>>>>>>>>>   g(1);
>>>>>>>>>>>   return 0;
>>>>>>>>>>> }
>>>>>>>>>>> cat a.hpp
>>>>>>>>>>> struct Base {
>>>>>>>>>>>   int a;
>>>>>>>>>>> };
>>>>>>>>>>>
>>>>>>>>>>> Let me know if I should investigate
further.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Manman
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Let me know which one you
prefer.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Do you have any comments on
the ref_addr patch (the 1st patch
>>>>>>>>>>>>> of the two)?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Nothing much - I wouldn't
mind Eric taking a look (& would
>>>>>>>>>>>> rather you not commit this
until you're committing the second patch, since
>>>>>>>>>>>> it's otherwise
untested/unjustified code) on the label/offset related stuff
>>>>>>>>>>>> since I'm less familiar
with that.
>>>>>>>>>>>>
>>>>>>>>>>>> There's one or two cases of
{} on single-statement blocks you
>>>>>>>>>>>> could fix up.
>>>>>>>>>>>>
>>>>>>>>>>>> "DebugInfoOffset"
(both the member and the functions to set/get
>>>>>>>>>>>> it) might be more meaningfully
named "SectionOffset" - but I'm not sure.
>>>>>>>>>>>> Eric? Other names
(DebugInfoSectionOffset?)?
>>>>>>>>>>>>
>>>>>>>>>>>> - David
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131016/33b96240/attachment.html>

On Wed, Oct 16, 2013 at 12:38 PM, David Blaikie <dblaikie at gmail.com>
wrote:
>
>
>
> On Wed, Oct 16, 2013 at 11:54 AM, Manman Ren <manman.ren at
gmail.com> wrote:
>
>>
>>
>>
>> On Wed, Oct 16, 2013 at 11:10 AM, David Blaikie <dblaikie at
gmail.com>wrote:
>>
>>>
>>>
>>>
>>> On Wed, Oct 16, 2013 at 10:54 AM, Manman Ren <manman.ren at
gmail.com>wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Tue, Oct 15, 2013 at 5:59 PM, David Blaikie <dblaikie at
gmail.com>wrote:
>>>>
>>>>>
>>>>>   In beginModule, we construct the CUs, but not all the
DIEs that
>>>>>>>> belong to the CU.
>>>>>>>> In endFunction, we started to construct the
scope DIEs. So in some
>>>>>>>> sense, we are adding things to prior CUs.
>>>>>>>>
>>>>>>>> Looking at void CompileUnit::addDIEEntry(DIE
*Die, uint16_t
>>>>>>>> Attribute, DIE *Entry), we can possibly have 3
CUs, this CU,
>>>>>>>> the CU Die belongs to (i.e. the CU the parent
chain points to), the
>>>>>>>> CU Entry belongs to.
>>>>>>>>
>>>>>>>
>>>>>>> When does "this CU" differ from both the
Die and the Entry?
>>>>>>>
>>>>>>
>>>>>> The signature of addDIEEntry didn't say that
"this CU" should be the
>>>>>> same as either Die's CU or Entry's CU.
>>>>>>
>>>>>
>>>>> No, but the code prior to your patch that's the only
really sane thing
>>>>> for it to do, right? Why would one CompileUnit be dealing
with some other
>>>>> CU's DIEs?
>>>>>
>>>>> This is why we should carefully think about the new
semantics we're
>>>>> introducing by doing cross-CU DIE references.
>>>>>
>>>>>
>>>>>>  If you think it should not happen, we should put a
comment in so
>>>>>> other developers will not violate it.
>>>>>> But that is irrelevant if we are going to make a single
assumption:
>>>>>>   "in addDIEEntry, if a DIE is without a parent,
it belongs to the CU
>>>>>> we call addDIEEntry on".
>>>>>>
>>>>>
>>>>> I'm not entirely sure that will be true. In the case of
implicit
>>>>> special members, such as the copy ctor, what could happen
is that a later
>>>>> CU might want to add a member function to an earlier CU.
That member
>>>>> function DIE will be being constructed from within the
latter CompileUnit,
>>>>> but its parent will be the former CU. Walking the parent
chain will find
>>>>> the correct (older CU) because we build the context for a
member function
>>>>> and attach the member function to that context before we
build any of its
>>>>> parameters, template arguments, etc.
>>>>>
>>>>> Exactly, so it is possible that a later CU can construct a
DIE that
>>>> belongs to another CU.
>>>>
>>>> I was rephrasing the statement in your earlier email and trying
to be
>>>> sure what assumption we are going to make.
>>>>   The claim Eric and I are making/discussed is that we should
assume
>>>> any DIE without a parent is in the current CU and/or that it
possibly
>>>> should never come up (I haven't thought through the
codepaths carefully
>>>> here - if we add CUs to their parent before creating their
members, I think
>>>> it should never come up).
>>>>
>>>> I know that the assumption may not be true with the current
code base,
>>>> but we need to either
>>>> 1> keep the worklist or
>>>> 2> remove the worklist and modify the code base to make the
assumption
>>>> true, right?
>>>>
>>>
>>> I'm OK assuming it to be true until we show it to not be true
and
>>> treating those cases as bugs and fixing them on an as-found basis.
>>>
>>
>> Okay, I attached a patch removing the worklist. Please review,
>>
>
> Will do.
>
>
>>
>>
>>> I'm not OK adding the worklist without knowing and testing the
cases it
>>> addresses.
>>>
>>>
>>>>
>>>> If we are going to make the assumption true, any suggestion on
how to
>>>> fix the above cases?
>>>>
>>>
>>> I don't know if the above cases are problems. Have you written
and
>>> verified any of the test cases I've described?
>>>
>>>
>>>> One possibility is to find the correct CU in DwarfDebug, and
call
>>>> getOrCreate on the correct CU.
>>>> In your above example, if a later CU wants to add a member
function to
>>>> an earlier CU, we find the earlier CU in DwarfDebug by going
through the
>>>> context chain of the member function, and then call getOrCreate
on the
>>>> earlier CU.
>>>>
>>>> So we can try to make the statement "a CU only constructs
a DIE that
>>>> belongs to the CU",
>>>>
>>>
>>> I don't believe so - not based on your current implementation.
We'd need
>>> to think more carefully about mismatches between type definitions
(implicit
>>> special members, member template specializations, nested classes -
these
>>> three things can create differences between definitions) and
resolving the
>>> metadata so those mismatches don't exist.
>>>
>>> That means when we merge the tables of type metadata string
identifier
>>> to MDNode from each CU is that we may need to modify the type
metadata
>>> nodes to merge the child lists from all those CUs.
>>>
>>
>> We don't do that.
>>
>
> Yes - see the next paragraph - /if/ we did that, then the "we only
produce
> DIEs from the CU that contains them" could be true. It won't be
for now,
> with your change, unless we do that or something else.
>
>
>>  If we did that, perhaps we wouldn't have to retroactively add any
DIEs
>>> to previous CUs, in which case your statement might become true.
>>>
>>
>> Adding DIE to a previous CU happens with the current code base, but we
>> can still try to implement "a CU only constructs a DIE that
belongs to the
>> CU", with that, we can release all DIEs constructed by the CU when
we are
>> done emitting the CU.
>> This is actually related to emit-one-CU-at-a-time, not so much on this
>> patch.
>>
>
> That's correct - you were asking whether we could rely on the
assumption
> that "a CU only constructs a DIE that belongs to that CU" and I
said we
> can't at the moment because of this situation.
>
> When we move to emit-one-CU-at-a-time, we'll need to figure out how to
> address this issue.
>
>
>>
>>
>>>
>>> For now, it isn't, so far as I can tell. (again, I'm giving
you some
>>> pointers about how I believe some of these things work, but my
intention is
>>> that these should give you ideas about what you need to test, ways
you
>>> might be able to find worklist cases, etc - this testing needs to
be done
>>> to understand how your patch will interact with these systems)
>>>
>>>
>>>> that will help emit-one-CU-at-a-time?
>>>>
>>>
>>> If we do one-CU-at-a-time emission we're not going to be able
to make
>>> debug info as compact as we can with your current change, not
without more
>>> work, at least.
>>>
>>> Taking the implicit special member example, CU1 would have the
>>> definition of some type 'foo' and its normal members. CU2
has a declaration
>>> of 'foo' (or perhaps another definition, depending on
whether it's a
>>> polymorphic type, etc) and the implicit special member definition
(such as
>>> the copy ctor). When we do the right type referencing and uniquing
at the
>>> metadata level what's going to happen (I think) is we'll
have one
>>> definition of 'foo', but its list of members will be that
of the definition
>>> (what happens if we have more than one definition and they differ
because
>>> they have different lists of members? I guess we'll produce two
definitions
>>> of 'foo', one in each CU that has the definitions because
they'll be
>>> different metadata nodes).
>>>
>>
>> We will have two DIEs corresponding to the two definitions.
>>
>
> Unless one is a declaration and the other'
>
and the other's a definition, in which case we'll resolve in favor of
the
definition.
> But when we actually refer to the definition via DIRef, we resolve to one
of the two definitions.
The member DIEs will be added to the single class that we resolve the DIRef
to.

Sort of. We'll have a weird situation where the type definition's list
of
members is incomplete - and, especially, one node's context, resolved via
DIRef, will refer to a type that doesn't list that node as a child. That
might be... interesting.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131016/f3449f66/attachment.html>

On Wed, Oct 16, 2013 at 12:38 PM, David Blaikie <dblaikie at gmail.com>
wrote:
>
>
>
> On Wed, Oct 16, 2013 at 11:54 AM, Manman Ren <manman.ren at
gmail.com> wrote:
>
>>
>>
>>
>> On Wed, Oct 16, 2013 at 11:10 AM, David Blaikie <dblaikie at
gmail.com>wrote:
>>
>>>
>>>
>>>
>>> On Wed, Oct 16, 2013 at 10:54 AM, Manman Ren <manman.ren at
gmail.com>wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Tue, Oct 15, 2013 at 5:59 PM, David Blaikie <dblaikie at
gmail.com>wrote:
>>>>
>>>>>
>>>>>   In beginModule, we construct the CUs, but not all the
DIEs that
>>>>>>>> belong to the CU.
>>>>>>>> In endFunction, we started to construct the
scope DIEs. So in some
>>>>>>>> sense, we are adding things to prior CUs.
>>>>>>>>
>>>>>>>> Looking at void CompileUnit::addDIEEntry(DIE
*Die, uint16_t
>>>>>>>> Attribute, DIE *Entry), we can possibly have 3
CUs, this CU,
>>>>>>>> the CU Die belongs to (i.e. the CU the parent
chain points to), the
>>>>>>>> CU Entry belongs to.
>>>>>>>>
>>>>>>>
>>>>>>> When does "this CU" differ from both the
Die and the Entry?
>>>>>>>
>>>>>>
>>>>>> The signature of addDIEEntry didn't say that
"this CU" should be the
>>>>>> same as either Die's CU or Entry's CU.
>>>>>>
>>>>>
>>>>> No, but the code prior to your patch that's the only
really sane thing
>>>>> for it to do, right? Why would one CompileUnit be dealing
with some other
>>>>> CU's DIEs?
>>>>>
>>>>> This is why we should carefully think about the new
semantics we're
>>>>> introducing by doing cross-CU DIE references.
>>>>>
>>>>>
>>>>>>  If you think it should not happen, we should put a
comment in so
>>>>>> other developers will not violate it.
>>>>>> But that is irrelevant if we are going to make a single
assumption:
>>>>>>   "in addDIEEntry, if a DIE is without a parent,
it belongs to the CU
>>>>>> we call addDIEEntry on".
>>>>>>
>>>>>
>>>>> I'm not entirely sure that will be true. In the case of
implicit
>>>>> special members, such as the copy ctor, what could happen
is that a later
>>>>> CU might want to add a member function to an earlier CU.
That member
>>>>> function DIE will be being constructed from within the
latter CompileUnit,
>>>>> but its parent will be the former CU. Walking the parent
chain will find
>>>>> the correct (older CU) because we build the context for a
member function
>>>>> and attach the member function to that context before we
build any of its
>>>>> parameters, template arguments, etc.
>>>>>
>>>>> Exactly, so it is possible that a later CU can construct a
DIE that
>>>> belongs to another CU.
>>>>
>>>> I was rephrasing the statement in your earlier email and trying
to be
>>>> sure what assumption we are going to make.
>>>>   The claim Eric and I are making/discussed is that we should
assume
>>>> any DIE without a parent is in the current CU and/or that it
possibly
>>>> should never come up (I haven't thought through the
codepaths carefully
>>>> here - if we add CUs to their parent before creating their
members, I think
>>>> it should never come up).
>>>>
>>>> I know that the assumption may not be true with the current
code base,
>>>> but we need to either
>>>> 1> keep the worklist or
>>>> 2> remove the worklist and modify the code base to make the
assumption
>>>> true, right?
>>>>
>>>
>>> I'm OK assuming it to be true until we show it to not be true
and
>>> treating those cases as bugs and fixing them on an as-found basis.
>>>
>>
>> Okay, I attached a patch removing the worklist. Please review,
>>
>
> Will do.
>
patch looks fairly obvious/trivial

Have you tried any of the test cases I've described (special members,
nested types, and member templates - all used across CUs so an earlier CU
has the type definition and a latter one has one of these extra members)?
I'd like to see test results for these before we progress.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131016/fda059ac/attachment.html>

>
>
> patch looks fairly obvious/trivial
>
> Have you tried any of the test cases I've described (special members,
> nested types, and member templates - all used across CUs so an earlier CU
> has the type definition and a latter one has one of these extra members)?
> I'd like to see test results for these before we progress.
>
Yes, templates and nested types where we have two MDNodes for the same
class, and one has an extra member.
They work fine with the patch (no assertion failure).

Manman
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131016/a576b831/attachment.html>