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

> 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.

>
>
>>
>>
>>> 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.

>
>
>>
>>
>>> 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/20131015/38e4afae/attachment.html>

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?

If we are going to make the assumption true, any suggestion on how to fix
the above cases? 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", that will help emit-one-CU-at-a-time? 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/821ece02/attachment.html>

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.

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. 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.

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). 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/8f52fca4/attachment.html>