David Blaikie
2013-Oct-15 21:24 UTC
[LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
On Tue, Oct 15, 2013 at 1:56 PM, Manman Ren <manman.ren at gmail.com> wrote:> > > > On Tue, Oct 15, 2013 at 1:37 PM, David Blaikie <dblaikie at gmail.com> wrote: > >> >> >> >> On Tue, Oct 15, 2013 at 1:22 PM, Manman Ren <manman.ren at gmail.com> wrote: >> >>> >>> >>> >>> On Tue, Oct 15, 2013 at 11:34 AM, David Blaikie <dblaikie at gmail.com>wrote: >>> >>>> >>>> >>>> >>>> On Tue, Oct 15, 2013 at 10:51 AM, Manman Ren <manman.ren at gmail.com>wrote: >>>> >>>>> >>>>> >>>>> >>>>> On Tue, Oct 15, 2013 at 10:10 AM, David Blaikie <dblaikie at gmail.com>wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Tue, Oct 15, 2013 at 10:05 AM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Wed, Oct 9, 2013 at 5:22 PM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Oct 9, 2013 at 1:32 PM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> David, >>>>>>>>> >>>>>>>>> Thanks for reviewing! >>>>>>>>> >>>>>>>>> On Wed, Oct 9, 2013 at 11:36 AM, David Blaikie <dblaikie at gmail.com >>>>>>>>> > wrote: >>>>>>>>> >>>>>>>>>> Might be easier if these were on Phabricator, but here are some >>>>>>>>>> thoughts: >>>>>>>>>> >>>>>>>>>> 0001: >>>>>>>>>> This patch generally, while separated for legibility, is >>>>>>>>>> untested & difficult to discuss in isolation. >>>>>>>>>> >>>>>>>>> I agree, this patch adds the functionality but does not use it, >>>>>>>>> the 2nd patch uses ref_addr. >>>>>>>>> If you think I should merge the two and commit as a single patch, >>>>>>>>> let me know. >>>>>>>>> >>>>>>>>> >>>>>>>>>> I may need to refer to the second patch in reviewing this first >>>>>>>>>> one. >>>>>>>>>> DwarfDebug.cpp: >>>>>>>>>> computeSizeAndOffsets: >>>>>>>>>> I believe this produces the wrong offset for the 3rd CU and >>>>>>>>>> onwards. computeSizeAndOffset returns the EndOffset which is absolute, not >>>>>>>>>> relative to the Offset passed in, so it should be assigned to SecOffset, >>>>>>>>>> not added to it. (eg: if you have CUs at 0, 42, and 57 - the first pass >>>>>>>>>> through SecOffset will be zero, then it'll be 0 + 42, then on the 3rd it'll >>>>>>>>>> be (0 + 42) + 57 which isn't right, it should just be 57). Please add more >>>>>>>>>> test coverage while fixing this issue. >>>>>>>>>> >>>>>>>>> >>>>>>>>> computeSizeAndOffset takes an offset that is relative to the start >>>>>>>>> of the CU and returns the offset relative to the CU after laying out the >>>>>>>>> DIE. >>>>>>>>> The initial offset before laying out the CU DIE is the header >>>>>>>>> size, EndOffset will be the offset relative to the CU after laying out the >>>>>>>>> whole CU DIE. >>>>>>>>> We can think of EndOffset as the size of the whole CU DIE. >>>>>>>>> SecOffset is the offset from the Debug Info section, so we can update it by >>>>>>>>> adding the CU size. >>>>>>>>> >>>>>>>>> // Offset from the beginning of debug info section. >>>>>>>>> unsigned SecOffset = 0; >>>>>>>>> for (SmallVectorImpl<CompileUnit *>::iterator I = CUs.begin(), >>>>>>>>> E = CUs.end(); I != E; ++I) { >>>>>>>>> (*I)->setDebugInfoOffset(SecOffset); >>>>>>>>> unsigned Offset >>>>>>>>> sizeof(int32_t) + // Length of Compilation Unit Info >>>>>>>>> sizeof(int16_t) + // DWARF version number >>>>>>>>> sizeof(int32_t) + // Offset Into Abbrev. Section >>>>>>>>> sizeof(int8_t); // Pointer Size (in bytes) >>>>>>>>> >>>>>>>>> unsigned EndOffset = computeSizeAndOffset((*I)->getCUDie(), >>>>>>>>> Offset); >>>>>>>>> SecOffset += EndOffset; >>>>>>>>> } >>>>>>>>> >>>>>>>> >>>>>>>> Added more comments in attached patch. >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> Eric/Manman: rough design question: compute the absolute offset >>>>>>>>>> of each CU within the debug_info section and describe them all relative to >>>>>>>>>> a single symbol at the start of the debug_info section, or should we put a >>>>>>>>>> label at the start of each CU? >>>>>>>>>> >>>>>>>>> >>>>>>>>> Either way should work. But since we already have the section >>>>>>>>> offset for each CU, describing relative to the start of debug_info section >>>>>>>>> saves us a few symbols :) >>>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>>> 0002: >>>>>>>>>> ref_addr_relocation.ll: >>>>>>>>>> seems a bit vague in terms of how you test for the >>>>>>>>>> relocation. I think it'd make more sense to test the assembly, than the >>>>>>>>>> reafobj output, that way you can test that the correct bytes have the >>>>>>>>>> relocation rather than just that there's "some" .debug_info relocation in >>>>>>>>>> the file. For an example, see test/DebugInfo/X86/tls.ll I wrote - it has >>>>>>>>>> some "unannotated" bytes because we still don't have a nice way to annotate >>>>>>>>>> location bytes that are DWARF expressions, but it's close - I guess those >>>>>>>>>> should be CHECK-NEXTs, though. In any case, you should be able to check a >>>>>>>>>> few lines of assembly with the # DW_AT/DW_TAG annotation comments. >>>>>>>>>> >>>>>>>>> >>>>>>>>> I can check for ".quad .Lsection_info+38 #DW_AT_type". >>>>>>>>> >>>>>>>> Done in attached patch. >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> You'd need to add the tu3.cpp from my example if you want to >>>>>>>>>> demonstrate that the relocation is actually working as intended and >>>>>>>>>> avoiding the bogus result I showed. >>>>>>>>>> type-unique-simple-a.ll >>>>>>>>>> While I agree that having common test cases helps reduce the >>>>>>>>>> number of separate invocations we have, this test case seems like it might >>>>>>>>>> be becoming a little hard to follow what's under test. Just going from the >>>>>>>>>> diff I'm not sure what's what. Could you give a brief description of the >>>>>>>>>> state of type-unique-simple2 files? What's involved? What's it meant to >>>>>>>>>> test? What's it actually testing? >>>>>>>>>> >>>>>>>>> I can add more comments. The source files are included in the >>>>>>>>> testing case. I am checking that llvm-link only generates a single copy of >>>>>>>>> the struct and the backend generates a single DIE and uses ref_addr. >>>>>>>>> The changes are to check "the backend generates a single DIE and >>>>>>>>> uses ref_addr". >>>>>>>>> >>>>>>>> Done in attached patch. >>>>>>>> >>>>>>>>> >>>>>>>>> DIE.h: >>>>>>>>>> checkCompileUnit could probably be called >>>>>>>>>> "getCompileUnitOrNull", the name "check*" seems to imply that it does some >>>>>>>>>> checking, which isn't true. >>>>>>>>>> >>>>>>>>> Will do. >>>>>>>>> >>>>>>>> Done in attached patch. >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> DwarfCompileUnit.cpp: >>>>>>>>>> the "istype || issubprogram" check should probably be pulled >>>>>>>>>> out into a separate function, something like "isShareableAcrossCUs" or >>>>>>>>>> something like that (please, that's not the best name, let's discuss it >>>>>>>>>> further before we settle on a name) so that getDIE and insertDIE are sure >>>>>>>>>> to use the same test at all times. >>>>>>>>>> >>>>>>>>> Yes, the condition is shared between getDIE and insertDIE. I like >>>>>>>>> isSharableAcrossCUs, because that is why the map is in DwarfDebug instead >>>>>>>>> of CompileUnit. >>>>>>>>> >>>>>>>> Done in attached patch. >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> Why does addDIEEntry take a form? When does the caller get to >>>>>>>>>> choose the form rather than the callee deciding between ref4 and ref_addr >>>>>>>>>> based on context? >>>>>>>>>> >>>>>>>>> >>>>>>>>> addDIEEntry took a form before my change and I didn't check why it >>>>>>>>> did. >>>>>>>>> I will check if all callers always use ref4, if it it true, I will >>>>>>>>> submit a cleanup patch first. >>>>>>>>> >>>>>>>> Done in attached patch. >>>>>>>> >>>>>>>>> >>>>>>>>> I'm still unsure about this worklist thing - do your current >>>>>>>>>> tests cover the need for the worklist? ie: if we removed that logic, would >>>>>>>>>> tests fail? Can you describe a specific sequence where the worklist is >>>>>>>>>> necessary? >>>>>>>>>> >>>>>>>>> >>>>>>>>> If we are sure that DIEs are always added to an owner before >>>>>>>>> calling addDIEEntry, we don't need the worklist. >>>>>>>>> But I saw cases where that was not true, I will get a reduced >>>>>>>>> testing case. >>>>>>>>> >>>>>>>> >>>>>>>> If we try to assert both DIEs have owner in addDIEEntry, the >>>>>>>> following testing cases will fail: >>>>>>>> LLVM :: DebugInfo/X86/multiple-aranges.ll >>>>>>>> LLVM :: DebugInfo/X86/ref_addr_relocation.ll >>>>>>>> LLVM :: DebugInfo/X86/stmt-list-multiple-compile-units.ll >>>>>>>> LLVM :: DebugInfo/two-cus-from-same-file.ll >>>>>>>> LLVM :: Linker/type-unique-simple-a.ll >>>>>>>> LLVM :: Linker/type-unique-simple2.ll >>>>>>>> >>>>>>>> For ref_addr_relocation, it failed in >>>>>>>> #5 0x0000000100b169ba in llvm::DwarfDebug::addDIEEntry >>>>>>>> (this=0x103023600, Die=0x102e14090, Attribute=73, Entry=0x10302a9d0) at >>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:3071 >>>>>>>> #6 0x0000000100b040e0 in llvm::CompileUnit::addType >>>>>>>> (this=0x102e13ec0, Entity=0x102e14090, Ty={<llvm::DIScope> >>>>>>>> {<llvm::DIDescriptor> = {DbgNode = 0x102e05f30}, <No data fields>}, <No >>>>>>>> data fields>}, Attribute=73) at >>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:910 >>>>>>>> #7 0x0000000100b05bff in >>>>>>>> llvm::CompileUnit::createGlobalVariableDIE (this=0x102e13ec0, >>>>>>>> N=0x102e068c0) at >>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1505 >>>>>>>> >>>>>>>> If we look at DwarfCompileUnit.cpp: >>>>>>>> VariableDIE = new DIE(GV.getTag()); >>>>>>>> // Add to map. >>>>>>>> insertDIE(N, VariableDIE); >>>>>>>> >>>>>>>> // Add name and type. >>>>>>>> addString(VariableDIE, dwarf::DW_AT_name, GV.getDisplayName()); >>>>>>>> addType(VariableDIE, GTy); >>>>>>>> >>>>>>>> The VariableDIE is not added to an owner yet when calling addType. >>>>>>>> >>>>>>> >>>>>>> I believe I have addressed all review comments, the patches are >>>>>>> re-attached here for convenience. >>>>>>> >>>>>> >>>>>> So I'm still thinking about the work list work. >>>>>> >>>>>> If we don't know which CU a DIE is in - isn't it, necessarily, going >>>>>> to be in the current CU (& thus referenced by ref_data4 (using a CU-local >>>>>> offset), not ref_addr)? >>>>>> >>>>> >>>>> That may be true. But can we prove that? >>>>> >>>> >>>> We really shouldn't add extra complexity to the codebase just because >>>> we don't know how the codebase works - that's what leads to the kind of >>>> complexity we see in the debug info handling today. >>>> >>> >>>> >>>>> There are two ways of handling this: >>>>> 1> use a worklist to be conservative >>>>> 2> do not use a worklist, but add an assertion when emitting a DIE A, >>>>> make sure the DIE referenced with ref4 is indeed in the same CU as DIE A. >>>>> >>>> >>>> Please just add this assertion. If it fires we'll have a test case and >>>> a concrete justification for this complexity such that should anyone remove >>>> it later because it looked unnecessary, they'll at least have a failing >>>> test to explain why it was there in the first plac . >>>> >>> >>> The assertion fails even with a simple testing case when the referenced >>> DIE has an owner and the DIE itself does not have an owner. >>> >> >> OK, sorry - I should've read your description of the assertion more >> carefully. I believe the assertion you added wasn't the right thing to test >> for. >> >> I'm not sure there is a correct assertion to add here to detect the case >> you're describing - perhaps a complex verification after-the-fact could be >> done, but essentially if we have a DIE that's partially constructed/has no >> parent we should assume it's in the current unit. >> > > Why should we make the assumption that a DIE without a parent at some > point should belong to the CU that is constructing the DIE? >Because the code we have today only constructs one CU at a time. I know of no code that adds anything to prior CUs. Indeed this may become an invariant one day when we do CU-at-a-time DWARF emission to reduce memory overhead. There's nothing in the design today that I know of that would make CU-at-a-time DWARF emission anything other than trivial. We build all the DIEs for a CU in one go, then move on to the next CU - the CU_Nodes loop in beginModule does this.> 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/e06c6d34/attachment.html>
David Blaikie
2013-Oct-15 21:57 UTC
[LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
All that being said, I'm asking these questions because I don't know for
sure - but that we cannot add unjustified complexity, we must understand
why it is there and have tests to demonstrate its necessity.
So to play some Devil's Advocate - how does your patch handle the following
situation:
hdr.h:
struct foo {
template<typename T>
struct bar {
};
};
src1.cpp:
#include "hdr.h"
foo f;
src2.cpp:
#include "hdr.h"
struct baz {
foo::bar<baz> *b;
};
foo::bar<baz> b;
and how does your patch without the worklist handle this situation? Here's
the bug I expect the non-worklist version to exhibit:
When building the CU for src1.cpp we create the 'foo' DIE and cache it
at
the DwarfDebug level.
When building the CU for src2.cpp we create the 'foo::bar<baz>'
DIE, in
doing so, we must construct a DIE for 'baz' and while doing that we come
back around to find 'foo::bar<baz>' without a parent, assume
it's in the
current CU(src2.cpp) and use ref4.
Then we finish building 'foo::bar<baz>' and add it to
'foo' which is in the
src1.cpp CU.
Broken.
Eric and I chatted through some of these scenarios and we seem to think
there's a different solution - rather than using a worklist, what we
should've done was add 'foo::bar<baz>' to 'foo'
On Tue, Oct 15, 2013 at 2:24 PM, David Blaikie <dblaikie at gmail.com>
wrote:
>
>
>
> On Tue, Oct 15, 2013 at 1:56 PM, Manman Ren <manman.ren at gmail.com>
wrote:
>
>>
>>
>>
>> On Tue, Oct 15, 2013 at 1:37 PM, David Blaikie <dblaikie at
gmail.com>wrote:
>>
>>>
>>>
>>>
>>> On Tue, Oct 15, 2013 at 1:22 PM, Manman Ren <manman.ren at
gmail.com>wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Tue, Oct 15, 2013 at 11:34 AM, David Blaikie <dblaikie at
gmail.com>wrote:
>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Oct 15, 2013 at 10:51 AM, Manman Ren <manman.ren
at gmail.com>wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Oct 15, 2013 at 10:10 AM, David Blaikie
<dblaikie at gmail.com>wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Oct 15, 2013 at 10:05 AM, Manman Ren
<manman.ren at gmail.com>wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Oct 9, 2013 at 5:22 PM, Manman Ren
<manman.ren at gmail.com>wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Oct 9, 2013 at 1:32 PM, Manman Ren
<manman.ren at gmail.com>wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> David,
>>>>>>>>>>
>>>>>>>>>> Thanks for reviewing!
>>>>>>>>>>
>>>>>>>>>> On Wed, Oct 9, 2013 at 11:36 AM, David
Blaikie <
>>>>>>>>>> dblaikie at gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> Might be easier if these were on
Phabricator, but here are some
>>>>>>>>>>> thoughts:
>>>>>>>>>>>
>>>>>>>>>>> 0001:
>>>>>>>>>>> This patch generally, while
separated for legibility, is
>>>>>>>>>>> untested & difficult to discuss
in isolation.
>>>>>>>>>>>
>>>>>>>>>> I agree, this patch adds the
functionality but does not use it,
>>>>>>>>>> the 2nd patch uses ref_addr.
>>>>>>>>>> If you think I should merge the two and
commit as a single patch,
>>>>>>>>>> let me know.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> I may need to refer to the second
patch in reviewing this first
>>>>>>>>>>> one.
>>>>>>>>>>> DwarfDebug.cpp:
>>>>>>>>>>> computeSizeAndOffsets:
>>>>>>>>>>> I believe this produces the
wrong offset for the 3rd CU
>>>>>>>>>>> and onwards. computeSizeAndOffset
returns the EndOffset which is absolute,
>>>>>>>>>>> not relative to the Offset passed
in, so it should be assigned to
>>>>>>>>>>> SecOffset, not added to it. (eg: if
you have CUs at 0, 42, and 57 - the
>>>>>>>>>>> first pass through SecOffset will
be zero, then it'll be 0 + 42, then on
>>>>>>>>>>> the 3rd it'll be (0 + 42) + 57
which isn't right, it should just be 57).
>>>>>>>>>>> Please add more test coverage while
fixing this issue.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> computeSizeAndOffset takes an offset
that is relative to the
>>>>>>>>>> start of the CU and returns the offset
relative to the CU after laying out
>>>>>>>>>> the DIE.
>>>>>>>>>> The initial offset before laying out
the CU DIE is the header
>>>>>>>>>> size, EndOffset will be the offset
relative to the CU after laying out the
>>>>>>>>>> whole CU DIE.
>>>>>>>>>> We can think of EndOffset as the size
of the whole CU DIE.
>>>>>>>>>> SecOffset is the offset from the Debug
Info section, so we can update it by
>>>>>>>>>> adding the CU size.
>>>>>>>>>>
>>>>>>>>>> // Offset from the beginning of debug
info section.
>>>>>>>>>> unsigned SecOffset = 0;
>>>>>>>>>> for (SmallVectorImpl<CompileUnit
*>::iterator I = CUs.begin(),
>>>>>>>>>> E = CUs.end(); I != E; ++I) {
>>>>>>>>>>
(*I)->setDebugInfoOffset(SecOffset);
>>>>>>>>>> unsigned Offset
>>>>>>>>>> sizeof(int32_t) + // Length of
Compilation Unit Info
>>>>>>>>>> sizeof(int16_t) + // DWARF
version number
>>>>>>>>>> sizeof(int32_t) + // Offset Into
Abbrev. Section
>>>>>>>>>> sizeof(int8_t); // Pointer Size
(in bytes)
>>>>>>>>>>
>>>>>>>>>> unsigned EndOffset =
computeSizeAndOffset((*I)->getCUDie(),
>>>>>>>>>> Offset);
>>>>>>>>>> SecOffset += EndOffset;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Added more comments in attached patch.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Eric/Manman: rough design question:
compute the absolute offset
>>>>>>>>>>> of each CU within the debug_info
section and describe them all relative to
>>>>>>>>>>> a single symbol at the start of the
debug_info section, or should we put a
>>>>>>>>>>> label at the start of each CU?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Either way should work. But since we
already have the section
>>>>>>>>>> offset for each CU, describing relative
to the start of debug_info section
>>>>>>>>>> saves us a few symbols :)
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 0002:
>>>>>>>>>>> ref_addr_relocation.ll:
>>>>>>>>>>> seems a bit vague in terms of
how you test for the
>>>>>>>>>>> relocation. I think it'd make
more sense to test the assembly, than the
>>>>>>>>>>> reafobj output, that way you can
test that the correct bytes have the
>>>>>>>>>>> relocation rather than just that
there's "some" .debug_info relocation in
>>>>>>>>>>> the file. For an example, see
test/DebugInfo/X86/tls.ll I wrote - it has
>>>>>>>>>>> some "unannotated" bytes
because we still don't have a nice way to annotate
>>>>>>>>>>> location bytes that are DWARF
expressions, but it's close - I guess those
>>>>>>>>>>> should be CHECK-NEXTs, though. In
any case, you should be able to check a
>>>>>>>>>>> few lines of assembly with the #
DW_AT/DW_TAG annotation comments.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I can check for ".quad
.Lsection_info+38 #DW_AT_type".
>>>>>>>>>>
>>>>>>>>> Done in attached patch.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> You'd need to add the
tu3.cpp from my example if you want
>>>>>>>>>>> to demonstrate that the relocation
is actually working as intended and
>>>>>>>>>>> avoiding the bogus result I showed.
>>>>>>>>>>> type-unique-simple-a.ll
>>>>>>>>>>> While I agree that having
common test cases helps reduce the
>>>>>>>>>>> number of separate invocations we
have, this test case seems like it might
>>>>>>>>>>> be becoming a little hard to follow
what's under test. Just going from the
>>>>>>>>>>> diff I'm not sure what's
what. Could you give a brief description of the
>>>>>>>>>>> state of type-unique-simple2 files?
What's involved? What's it meant to
>>>>>>>>>>> test? What's it actually
testing?
>>>>>>>>>>>
>>>>>>>>>> I can add more comments. The source
files are included in the
>>>>>>>>>> testing case. I am checking that
llvm-link only generates a single copy of
>>>>>>>>>> the struct and the backend generates a
single DIE and uses ref_addr.
>>>>>>>>>> The changes are to check "the
backend generates a single DIE and
>>>>>>>>>> uses ref_addr".
>>>>>>>>>>
>>>>>>>>> Done in attached patch.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> DIE.h:
>>>>>>>>>>> checkCompileUnit could probably
be called
>>>>>>>>>>> "getCompileUnitOrNull",
the name "check*" seems to imply that it does some
>>>>>>>>>>> checking, which isn't true.
>>>>>>>>>>>
>>>>>>>>>> Will do.
>>>>>>>>>>
>>>>>>>>> Done in attached patch.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> DwarfCompileUnit.cpp:
>>>>>>>>>>> the "istype ||
issubprogram" check should probably be pulled
>>>>>>>>>>> out into a separate function,
something like "isShareableAcrossCUs" or
>>>>>>>>>>> something like that (please,
that's not the best name, let's discuss it
>>>>>>>>>>> further before we settle on a name)
so that getDIE and insertDIE are sure
>>>>>>>>>>> to use the same test at all times.
>>>>>>>>>>>
>>>>>>>>>> Yes, the condition is shared between
getDIE and insertDIE. I like
>>>>>>>>>> isSharableAcrossCUs, because that is
why the map is in DwarfDebug instead
>>>>>>>>>> of CompileUnit.
>>>>>>>>>>
>>>>>>>>> Done in attached patch.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Why does addDIEEntry take a
form? When does the caller get to
>>>>>>>>>>> choose the form rather than the
callee deciding between ref4 and ref_addr
>>>>>>>>>>> based on context?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> addDIEEntry took a form before my
change and I didn't check why
>>>>>>>>>> it did.
>>>>>>>>>> I will check if all callers always use
ref4, if it it true, I
>>>>>>>>>> will submit a cleanup patch first.
>>>>>>>>>>
>>>>>>>>> Done in attached patch.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I'm still unsure about this
worklist thing - do your current
>>>>>>>>>>> tests cover the need for the
worklist? ie: if we removed that logic, would
>>>>>>>>>>> tests fail? Can you describe a
specific sequence where the worklist is
>>>>>>>>>>> necessary?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If we are sure that DIEs are always
added to an owner before
>>>>>>>>>> calling addDIEEntry, we don't need
the worklist.
>>>>>>>>>> But I saw cases where that was not
true, I will get a reduced
>>>>>>>>>> testing case.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If we try to assert both DIEs have owner in
addDIEEntry, the
>>>>>>>>> following testing cases will fail:
>>>>>>>>> LLVM ::
DebugInfo/X86/multiple-aranges.ll
>>>>>>>>> LLVM ::
DebugInfo/X86/ref_addr_relocation.ll
>>>>>>>>> LLVM ::
DebugInfo/X86/stmt-list-multiple-compile-units.ll
>>>>>>>>> LLVM ::
DebugInfo/two-cus-from-same-file.ll
>>>>>>>>> LLVM :: Linker/type-unique-simple-a.ll
>>>>>>>>> LLVM :: Linker/type-unique-simple2.ll
>>>>>>>>>
>>>>>>>>> For ref_addr_relocation, it failed in
>>>>>>>>> #5 0x0000000100b169ba in
llvm::DwarfDebug::addDIEEntry
>>>>>>>>> (this=0x103023600, Die=0x102e14090,
Attribute=73, Entry=0x10302a9d0) at
>>>>>>>>>
/Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:3071
>>>>>>>>> #6 0x0000000100b040e0 in
llvm::CompileUnit::addType
>>>>>>>>> (this=0x102e13ec0, Entity=0x102e14090,
Ty={<llvm::DIScope> >>>>>>>>>
{<llvm::DIDescriptor> = {DbgNode = 0x102e05f30}, <No data fields>},
<No
>>>>>>>>> data fields>}, Attribute=73) at
>>>>>>>>>
/Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:910
>>>>>>>>> #7 0x0000000100b05bff in
>>>>>>>>> llvm::CompileUnit::createGlobalVariableDIE
(this=0x102e13ec0,
>>>>>>>>> N=0x102e068c0) at
>>>>>>>>>
/Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1505
>>>>>>>>>
>>>>>>>>> If we look at DwarfCompileUnit.cpp:
>>>>>>>>> VariableDIE = new DIE(GV.getTag());
>>>>>>>>> // Add to map.
>>>>>>>>> insertDIE(N, VariableDIE);
>>>>>>>>>
>>>>>>>>> // Add name and type.
>>>>>>>>> addString(VariableDIE,
dwarf::DW_AT_name, GV.getDisplayName());
>>>>>>>>> addType(VariableDIE, GTy);
>>>>>>>>>
>>>>>>>>> The VariableDIE is not added to an owner
yet when calling addType.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I believe I have addressed all review comments,
the patches are
>>>>>>>> re-attached here for convenience.
>>>>>>>>
>>>>>>>
>>>>>>> So I'm still thinking about the work list work.
>>>>>>>
>>>>>>> If we don't know which CU a DIE is in -
isn't it, necessarily, going
>>>>>>> to be in the current CU (& thus referenced by
ref_data4 (using a CU-local
>>>>>>> offset), not ref_addr)?
>>>>>>>
>>>>>>
>>>>>> That may be true. But can we prove that?
>>>>>>
>>>>>
>>>>> We really shouldn't add extra complexity to the
codebase just because
>>>>> we don't know how the codebase works - that's what
leads to the kind of
>>>>> complexity we see in the debug info handling today.
>>>>>
>>>>
>>>>>
>>>>>> There are two ways of handling this:
>>>>>> 1> use a worklist to be conservative
>>>>>> 2> do not use a worklist, but add an assertion when
emitting a DIE A,
>>>>>> make sure the DIE referenced with ref4 is indeed in the
same CU as DIE A.
>>>>>>
>>>>>
>>>>> Please just add this assertion. If it fires we'll have
a test case and
>>>>> a concrete justification for this complexity such that
should anyone remove
>>>>> it later because it looked unnecessary, they'll at
least have a failing
>>>>> test to explain why it was there in the first plac .
>>>>>
>>>>
>>>> The assertion fails even with a simple testing case when the
referenced
>>>> DIE has an owner and the DIE itself does not have an owner.
>>>>
>>>
>>> OK, sorry - I should've read your description of the assertion
more
>>> carefully. I believe the assertion you added wasn't the right
thing to test
>>> for.
>>>
>>> I'm not sure there is a correct assertion to add here to detect
the case
>>> you're describing - perhaps a complex verification
after-the-fact could be
>>> done, but essentially if we have a DIE that's partially
constructed/has no
>>> parent we should assume it's in the current unit.
>>>
>>
>> Why should we make the assumption that a DIE without a parent at some
>> point should belong to the CU that is constructing the DIE?
>>
>
> Because the code we have today only constructs one CU at a time. I know of
> no code that adds anything to prior CUs. Indeed this may become an
> invariant one day when we do CU-at-a-time DWARF emission to reduce memory
> overhead. There's nothing in the design today that I know of that would
> make CU-at-a-time DWARF emission anything other than trivial. We build all
> the DIEs for a CU in one go, then move on to the next CU - the CU_Nodes
> loop in beginModule does this.
>
>
>> 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/4080da29/attachment.html>
David Blaikie
2013-Oct-15 21:59 UTC
[LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
On Tue, Oct 15, 2013 at 2:57 PM, David Blaikie <dblaikie at gmail.com> wrote:> All that being said, I'm asking these questions because I don't know for > sure - but that we cannot add unjustified complexity, we must understand > why it is there and have tests to demonstrate its necessity. > > So to play some Devil's Advocate - how does your patch handle the > following situation: > > hdr.h: > struct foo { > template<typename T> > struct bar { > }; > }; > > src1.cpp: > #include "hdr.h" > foo f; > > src2.cpp: > #include "hdr.h" > struct baz { > foo::bar<baz> *b; > }; > > foo::bar<baz> b; > > > and how does your patch without the worklist handle this situation? Here's > the bug I expect the non-worklist version to exhibit: > > When building the CU for src1.cpp we create the 'foo' DIE and cache it at > the DwarfDebug level. > When building the CU for src2.cpp we create the 'foo::bar<baz>' DIE, in > doing so, we must construct a DIE for 'baz' and while doing that we come > back around to find 'foo::bar<baz>' without a parent, assume it's in the > current CU(src2.cpp) and use ref4. > Then we finish building 'foo::bar<baz>' and add it to 'foo' which is in > the src1.cpp CU. > Broken. > > Eric and I chatted through some of these scenarios and we seem to think > there's a different solution - rather than using a worklist, what we > should've done was add 'foo::bar<baz>' to 'foo' >As soon as we build the DIE. We might even need to go as far as I did in getOrCreateContextDIE, which is to ensure the context is retrieved/constructed before we build the type DIE, then add the type DIE to the context before we build/add its parameters, template arguments, etc. But again, having the specific, concrete case is important before we go trying to solve the problem. Please test the above example, see if your solution, with the worklist removed, exhibits the bug I described, and if so - consider whether the solution I've described would address it. Thanks, - David> > > On Tue, Oct 15, 2013 at 2:24 PM, David Blaikie <dblaikie at gmail.com> wrote: > >> >> >> >> On Tue, Oct 15, 2013 at 1:56 PM, Manman Ren <manman.ren at gmail.com> wrote: >> >>> >>> >>> >>> On Tue, Oct 15, 2013 at 1:37 PM, David Blaikie <dblaikie at gmail.com>wrote: >>> >>>> >>>> >>>> >>>> On Tue, Oct 15, 2013 at 1:22 PM, Manman Ren <manman.ren at gmail.com>wrote: >>>> >>>>> >>>>> >>>>> >>>>> On Tue, Oct 15, 2013 at 11:34 AM, David Blaikie <dblaikie at gmail.com>wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Tue, Oct 15, 2013 at 10:51 AM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Tue, Oct 15, 2013 at 10:10 AM, David Blaikie <dblaikie at gmail.com>wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Oct 15, 2013 at 10:05 AM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Oct 9, 2013 at 5:22 PM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Oct 9, 2013 at 1:32 PM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> David, >>>>>>>>>>> >>>>>>>>>>> Thanks for reviewing! >>>>>>>>>>> >>>>>>>>>>> On Wed, Oct 9, 2013 at 11:36 AM, David Blaikie < >>>>>>>>>>> dblaikie at gmail.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> Might be easier if these were on Phabricator, but here are some >>>>>>>>>>>> thoughts: >>>>>>>>>>>> >>>>>>>>>>>> 0001: >>>>>>>>>>>> This patch generally, while separated for legibility, is >>>>>>>>>>>> untested & difficult to discuss in isolation. >>>>>>>>>>>> >>>>>>>>>>> I agree, this patch adds the functionality but does not use it, >>>>>>>>>>> the 2nd patch uses ref_addr. >>>>>>>>>>> If you think I should merge the two and commit as a single >>>>>>>>>>> patch, let me know. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> I may need to refer to the second patch in reviewing this >>>>>>>>>>>> first one. >>>>>>>>>>>> DwarfDebug.cpp: >>>>>>>>>>>> computeSizeAndOffsets: >>>>>>>>>>>> I believe this produces the wrong offset for the 3rd CU >>>>>>>>>>>> and onwards. computeSizeAndOffset returns the EndOffset which is absolute, >>>>>>>>>>>> not relative to the Offset passed in, so it should be assigned to >>>>>>>>>>>> SecOffset, not added to it. (eg: if you have CUs at 0, 42, and 57 - the >>>>>>>>>>>> first pass through SecOffset will be zero, then it'll be 0 + 42, then on >>>>>>>>>>>> the 3rd it'll be (0 + 42) + 57 which isn't right, it should just be 57). >>>>>>>>>>>> Please add more test coverage while fixing this issue. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> computeSizeAndOffset takes an offset that is relative to the >>>>>>>>>>> start of the CU and returns the offset relative to the CU after laying out >>>>>>>>>>> the DIE. >>>>>>>>>>> The initial offset before laying out the CU DIE is the header >>>>>>>>>>> size, EndOffset will be the offset relative to the CU after laying out the >>>>>>>>>>> whole CU DIE. >>>>>>>>>>> We can think of EndOffset as the size of the whole CU DIE. >>>>>>>>>>> SecOffset is the offset from the Debug Info section, so we can update it by >>>>>>>>>>> adding the CU size. >>>>>>>>>>> >>>>>>>>>>> // Offset from the beginning of debug info section. >>>>>>>>>>> unsigned SecOffset = 0; >>>>>>>>>>> for (SmallVectorImpl<CompileUnit *>::iterator I = CUs.begin(), >>>>>>>>>>> E = CUs.end(); I != E; ++I) { >>>>>>>>>>> (*I)->setDebugInfoOffset(SecOffset); >>>>>>>>>>> unsigned Offset >>>>>>>>>>> sizeof(int32_t) + // Length of Compilation Unit Info >>>>>>>>>>> sizeof(int16_t) + // DWARF version number >>>>>>>>>>> sizeof(int32_t) + // Offset Into Abbrev. Section >>>>>>>>>>> sizeof(int8_t); // Pointer Size (in bytes) >>>>>>>>>>> >>>>>>>>>>> unsigned EndOffset = computeSizeAndOffset((*I)->getCUDie(), >>>>>>>>>>> Offset); >>>>>>>>>>> SecOffset += EndOffset; >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Added more comments in attached patch. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> Eric/Manman: rough design question: compute the absolute offset >>>>>>>>>>>> of each CU within the debug_info section and describe them all relative to >>>>>>>>>>>> a single symbol at the start of the debug_info section, or should we put a >>>>>>>>>>>> label at the start of each CU? >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Either way should work. But since we already have the section >>>>>>>>>>> offset for each CU, describing relative to the start of debug_info section >>>>>>>>>>> saves us a few symbols :) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> 0002: >>>>>>>>>>>> ref_addr_relocation.ll: >>>>>>>>>>>> seems a bit vague in terms of how you test for the >>>>>>>>>>>> relocation. I think it'd make more sense to test the assembly, than the >>>>>>>>>>>> reafobj output, that way you can test that the correct bytes have the >>>>>>>>>>>> relocation rather than just that there's "some" .debug_info relocation in >>>>>>>>>>>> the file. For an example, see test/DebugInfo/X86/tls.ll I wrote - it has >>>>>>>>>>>> some "unannotated" bytes because we still don't have a nice way to annotate >>>>>>>>>>>> location bytes that are DWARF expressions, but it's close - I guess those >>>>>>>>>>>> should be CHECK-NEXTs, though. In any case, you should be able to check a >>>>>>>>>>>> few lines of assembly with the # DW_AT/DW_TAG annotation comments. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I can check for ".quad .Lsection_info+38 #DW_AT_type". >>>>>>>>>>> >>>>>>>>>> Done in attached patch. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> You'd need to add the tu3.cpp from my example if you want >>>>>>>>>>>> to demonstrate that the relocation is actually working as intended and >>>>>>>>>>>> avoiding the bogus result I showed. >>>>>>>>>>>> type-unique-simple-a.ll >>>>>>>>>>>> While I agree that having common test cases helps reduce >>>>>>>>>>>> the number of separate invocations we have, this test case seems like it >>>>>>>>>>>> might be becoming a little hard to follow what's under test. Just going >>>>>>>>>>>> from the diff I'm not sure what's what. Could you give a brief description >>>>>>>>>>>> of the state of type-unique-simple2 files? What's involved? What's it meant >>>>>>>>>>>> to test? What's it actually testing? >>>>>>>>>>>> >>>>>>>>>>> I can add more comments. The source files are included in the >>>>>>>>>>> testing case. I am checking that llvm-link only generates a single copy of >>>>>>>>>>> the struct and the backend generates a single DIE and uses ref_addr. >>>>>>>>>>> The changes are to check "the backend generates a single DIE and >>>>>>>>>>> uses ref_addr". >>>>>>>>>>> >>>>>>>>>> Done in attached patch. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> DIE.h: >>>>>>>>>>>> checkCompileUnit could probably be called >>>>>>>>>>>> "getCompileUnitOrNull", the name "check*" seems to imply that it does some >>>>>>>>>>>> checking, which isn't true. >>>>>>>>>>>> >>>>>>>>>>> Will do. >>>>>>>>>>> >>>>>>>>>> Done in attached patch. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> DwarfCompileUnit.cpp: >>>>>>>>>>>> the "istype || issubprogram" check should probably be >>>>>>>>>>>> pulled out into a separate function, something like "isShareableAcrossCUs" >>>>>>>>>>>> or something like that (please, that's not the best name, let's discuss it >>>>>>>>>>>> further before we settle on a name) so that getDIE and insertDIE are sure >>>>>>>>>>>> to use the same test at all times. >>>>>>>>>>>> >>>>>>>>>>> Yes, the condition is shared between getDIE and insertDIE. I >>>>>>>>>>> like isSharableAcrossCUs, because that is why the map is in DwarfDebug >>>>>>>>>>> instead of CompileUnit. >>>>>>>>>>> >>>>>>>>>> Done in attached patch. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> Why does addDIEEntry take a form? When does the caller get >>>>>>>>>>>> to choose the form rather than the callee deciding between ref4 and >>>>>>>>>>>> ref_addr based on context? >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> addDIEEntry took a form before my change and I didn't check why >>>>>>>>>>> it did. >>>>>>>>>>> I will check if all callers always use ref4, if it it true, I >>>>>>>>>>> will submit a cleanup patch first. >>>>>>>>>>> >>>>>>>>>> Done in attached patch. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I'm still unsure about this worklist thing - do your current >>>>>>>>>>>> tests cover the need for the worklist? ie: if we removed that logic, would >>>>>>>>>>>> tests fail? Can you describe a specific sequence where the worklist is >>>>>>>>>>>> necessary? >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> If we are sure that DIEs are always added to an owner before >>>>>>>>>>> calling addDIEEntry, we don't need the worklist. >>>>>>>>>>> But I saw cases where that was not true, I will get a reduced >>>>>>>>>>> testing case. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> If we try to assert both DIEs have owner in addDIEEntry, the >>>>>>>>>> following testing cases will fail: >>>>>>>>>> LLVM :: DebugInfo/X86/multiple-aranges.ll >>>>>>>>>> LLVM :: DebugInfo/X86/ref_addr_relocation.ll >>>>>>>>>> LLVM :: DebugInfo/X86/stmt-list-multiple-compile-units.ll >>>>>>>>>> LLVM :: DebugInfo/two-cus-from-same-file.ll >>>>>>>>>> LLVM :: Linker/type-unique-simple-a.ll >>>>>>>>>> LLVM :: Linker/type-unique-simple2.ll >>>>>>>>>> >>>>>>>>>> For ref_addr_relocation, it failed in >>>>>>>>>> #5 0x0000000100b169ba in llvm::DwarfDebug::addDIEEntry >>>>>>>>>> (this=0x103023600, Die=0x102e14090, Attribute=73, Entry=0x10302a9d0) at >>>>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:3071 >>>>>>>>>> #6 0x0000000100b040e0 in llvm::CompileUnit::addType >>>>>>>>>> (this=0x102e13ec0, Entity=0x102e14090, Ty={<llvm::DIScope> >>>>>>>>>> {<llvm::DIDescriptor> = {DbgNode = 0x102e05f30}, <No data fields>}, <No >>>>>>>>>> data fields>}, Attribute=73) at >>>>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:910 >>>>>>>>>> #7 0x0000000100b05bff in >>>>>>>>>> llvm::CompileUnit::createGlobalVariableDIE (this=0x102e13ec0, >>>>>>>>>> N=0x102e068c0) at >>>>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1505 >>>>>>>>>> >>>>>>>>>> If we look at DwarfCompileUnit.cpp: >>>>>>>>>> VariableDIE = new DIE(GV.getTag()); >>>>>>>>>> // Add to map. >>>>>>>>>> insertDIE(N, VariableDIE); >>>>>>>>>> >>>>>>>>>> // Add name and type. >>>>>>>>>> addString(VariableDIE, dwarf::DW_AT_name, >>>>>>>>>> GV.getDisplayName()); >>>>>>>>>> addType(VariableDIE, GTy); >>>>>>>>>> >>>>>>>>>> The VariableDIE is not added to an owner yet when calling addType. >>>>>>>>>> >>>>>>>>> >>>>>>>>> I believe I have addressed all review comments, the patches are >>>>>>>>> re-attached here for convenience. >>>>>>>>> >>>>>>>> >>>>>>>> So I'm still thinking about the work list work. >>>>>>>> >>>>>>>> If we don't know which CU a DIE is in - isn't it, necessarily, >>>>>>>> going to be in the current CU (& thus referenced by ref_data4 (using a >>>>>>>> CU-local offset), not ref_addr)? >>>>>>>> >>>>>>> >>>>>>> That may be true. But can we prove that? >>>>>>> >>>>>> >>>>>> We really shouldn't add extra complexity to the codebase just because >>>>>> we don't know how the codebase works - that's what leads to the kind of >>>>>> complexity we see in the debug info handling today. >>>>>> >>>>> >>>>>> >>>>>>> There are two ways of handling this: >>>>>>> 1> use a worklist to be conservative >>>>>>> 2> do not use a worklist, but add an assertion when emitting a DIE >>>>>>> A, make sure the DIE referenced with ref4 is indeed in the same CU as DIE A. >>>>>>> >>>>>> >>>>>> Please just add this assertion. If it fires we'll have a test case >>>>>> and a concrete justification for this complexity such that should anyone >>>>>> remove it later because it looked unnecessary, they'll at least have a >>>>>> failing test to explain why it was there in the first plac . >>>>>> >>>>> >>>>> The assertion fails even with a simple testing case when the >>>>> referenced DIE has an owner and the DIE itself does not have an owner. >>>>> >>>> >>>> OK, sorry - I should've read your description of the assertion more >>>> carefully. I believe the assertion you added wasn't the right thing to test >>>> for. >>>> >>>> I'm not sure there is a correct assertion to add here to detect the >>>> case you're describing - perhaps a complex verification after-the-fact >>>> could be done, but essentially if we have a DIE that's partially >>>> constructed/has no parent we should assume it's in the current unit. >>>> >>> >>> Why should we make the assumption that a DIE without a parent at some >>> point should belong to the CU that is constructing the DIE? >>> >> >> Because the code we have today only constructs one CU at a time. I know >> of no code that adds anything to prior CUs. Indeed this may become an >> invariant one day when we do CU-at-a-time DWARF emission to reduce memory >> overhead. There's nothing in the design today that I know of that would >> make CU-at-a-time DWARF emission anything other than trivial. We build all >> the DIEs for a CU in one go, then move on to the next CU - the CU_Nodes >> loop in beginModule does this. >> >> >>> 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/563da0b0/attachment.html>
Manman Ren
2013-Oct-15 23:59 UTC
[LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
On Tue, Oct 15, 2013 at 2:24 PM, David Blaikie <dblaikie at gmail.com> wrote:> > > > On Tue, Oct 15, 2013 at 1:56 PM, Manman Ren <manman.ren at gmail.com> wrote: > >> >> >> >> On Tue, Oct 15, 2013 at 1:37 PM, David Blaikie <dblaikie at gmail.com>wrote: >> >>> >>> >>> >>> On Tue, Oct 15, 2013 at 1:22 PM, Manman Ren <manman.ren at gmail.com>wrote: >>> >>>> >>>> >>>> >>>> On Tue, Oct 15, 2013 at 11:34 AM, David Blaikie <dblaikie at gmail.com>wrote: >>>> >>>>> >>>>> >>>>> >>>>> On Tue, Oct 15, 2013 at 10:51 AM, Manman Ren <manman.ren at gmail.com>wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Tue, Oct 15, 2013 at 10:10 AM, David Blaikie <dblaikie at gmail.com>wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Tue, Oct 15, 2013 at 10:05 AM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Oct 9, 2013 at 5:22 PM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Oct 9, 2013 at 1:32 PM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> David, >>>>>>>>>> >>>>>>>>>> Thanks for reviewing! >>>>>>>>>> >>>>>>>>>> On Wed, Oct 9, 2013 at 11:36 AM, David Blaikie < >>>>>>>>>> dblaikie at gmail.com> wrote: >>>>>>>>>> >>>>>>>>>>> Might be easier if these were on Phabricator, but here are some >>>>>>>>>>> thoughts: >>>>>>>>>>> >>>>>>>>>>> 0001: >>>>>>>>>>> This patch generally, while separated for legibility, is >>>>>>>>>>> untested & difficult to discuss in isolation. >>>>>>>>>>> >>>>>>>>>> I agree, this patch adds the functionality but does not use it, >>>>>>>>>> the 2nd patch uses ref_addr. >>>>>>>>>> If you think I should merge the two and commit as a single patch, >>>>>>>>>> let me know. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> I may need to refer to the second patch in reviewing this first >>>>>>>>>>> one. >>>>>>>>>>> DwarfDebug.cpp: >>>>>>>>>>> computeSizeAndOffsets: >>>>>>>>>>> I believe this produces the wrong offset for the 3rd CU >>>>>>>>>>> and onwards. computeSizeAndOffset returns the EndOffset which is absolute, >>>>>>>>>>> not relative to the Offset passed in, so it should be assigned to >>>>>>>>>>> SecOffset, not added to it. (eg: if you have CUs at 0, 42, and 57 - the >>>>>>>>>>> first pass through SecOffset will be zero, then it'll be 0 + 42, then on >>>>>>>>>>> the 3rd it'll be (0 + 42) + 57 which isn't right, it should just be 57). >>>>>>>>>>> Please add more test coverage while fixing this issue. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> computeSizeAndOffset takes an offset that is relative to the >>>>>>>>>> start of the CU and returns the offset relative to the CU after laying out >>>>>>>>>> the DIE. >>>>>>>>>> The initial offset before laying out the CU DIE is the header >>>>>>>>>> size, EndOffset will be the offset relative to the CU after laying out the >>>>>>>>>> whole CU DIE. >>>>>>>>>> We can think of EndOffset as the size of the whole CU DIE. >>>>>>>>>> SecOffset is the offset from the Debug Info section, so we can update it by >>>>>>>>>> adding the CU size. >>>>>>>>>> >>>>>>>>>> // Offset from the beginning of debug info section. >>>>>>>>>> unsigned SecOffset = 0; >>>>>>>>>> for (SmallVectorImpl<CompileUnit *>::iterator I = CUs.begin(), >>>>>>>>>> E = CUs.end(); I != E; ++I) { >>>>>>>>>> (*I)->setDebugInfoOffset(SecOffset); >>>>>>>>>> unsigned Offset >>>>>>>>>> sizeof(int32_t) + // Length of Compilation Unit Info >>>>>>>>>> sizeof(int16_t) + // DWARF version number >>>>>>>>>> sizeof(int32_t) + // Offset Into Abbrev. Section >>>>>>>>>> sizeof(int8_t); // Pointer Size (in bytes) >>>>>>>>>> >>>>>>>>>> unsigned EndOffset = computeSizeAndOffset((*I)->getCUDie(), >>>>>>>>>> Offset); >>>>>>>>>> SecOffset += EndOffset; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>> >>>>>>>>> Added more comments in attached patch. >>>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> Eric/Manman: rough design question: compute the absolute offset >>>>>>>>>>> of each CU within the debug_info section and describe them all relative to >>>>>>>>>>> a single symbol at the start of the debug_info section, or should we put a >>>>>>>>>>> label at the start of each CU? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Either way should work. But since we already have the section >>>>>>>>>> offset for each CU, describing relative to the start of debug_info section >>>>>>>>>> saves us a few symbols :) >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 0002: >>>>>>>>>>> ref_addr_relocation.ll: >>>>>>>>>>> seems a bit vague in terms of how you test for the >>>>>>>>>>> relocation. I think it'd make more sense to test the assembly, than the >>>>>>>>>>> reafobj output, that way you can test that the correct bytes have the >>>>>>>>>>> relocation rather than just that there's "some" .debug_info relocation in >>>>>>>>>>> the file. For an example, see test/DebugInfo/X86/tls.ll I wrote - it has >>>>>>>>>>> some "unannotated" bytes because we still don't have a nice way to annotate >>>>>>>>>>> location bytes that are DWARF expressions, but it's close - I guess those >>>>>>>>>>> should be CHECK-NEXTs, though. In any case, you should be able to check a >>>>>>>>>>> few lines of assembly with the # DW_AT/DW_TAG annotation comments. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I can check for ".quad .Lsection_info+38 #DW_AT_type". >>>>>>>>>> >>>>>>>>> Done in attached patch. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> You'd need to add the tu3.cpp from my example if you want >>>>>>>>>>> to demonstrate that the relocation is actually working as intended and >>>>>>>>>>> avoiding the bogus result I showed. >>>>>>>>>>> type-unique-simple-a.ll >>>>>>>>>>> While I agree that having common test cases helps reduce the >>>>>>>>>>> number of separate invocations we have, this test case seems like it might >>>>>>>>>>> be becoming a little hard to follow what's under test. Just going from the >>>>>>>>>>> diff I'm not sure what's what. Could you give a brief description of the >>>>>>>>>>> state of type-unique-simple2 files? What's involved? What's it meant to >>>>>>>>>>> test? What's it actually testing? >>>>>>>>>>> >>>>>>>>>> I can add more comments. The source files are included in the >>>>>>>>>> testing case. I am checking that llvm-link only generates a single copy of >>>>>>>>>> the struct and the backend generates a single DIE and uses ref_addr. >>>>>>>>>> The changes are to check "the backend generates a single DIE and >>>>>>>>>> uses ref_addr". >>>>>>>>>> >>>>>>>>> Done in attached patch. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> DIE.h: >>>>>>>>>>> checkCompileUnit could probably be called >>>>>>>>>>> "getCompileUnitOrNull", the name "check*" seems to imply that it does some >>>>>>>>>>> checking, which isn't true. >>>>>>>>>>> >>>>>>>>>> Will do. >>>>>>>>>> >>>>>>>>> Done in attached patch. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> DwarfCompileUnit.cpp: >>>>>>>>>>> the "istype || issubprogram" check should probably be pulled >>>>>>>>>>> out into a separate function, something like "isShareableAcrossCUs" or >>>>>>>>>>> something like that (please, that's not the best name, let's discuss it >>>>>>>>>>> further before we settle on a name) so that getDIE and insertDIE are sure >>>>>>>>>>> to use the same test at all times. >>>>>>>>>>> >>>>>>>>>> Yes, the condition is shared between getDIE and insertDIE. I like >>>>>>>>>> isSharableAcrossCUs, because that is why the map is in DwarfDebug instead >>>>>>>>>> of CompileUnit. >>>>>>>>>> >>>>>>>>> Done in attached patch. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> Why does addDIEEntry take a form? When does the caller get to >>>>>>>>>>> choose the form rather than the callee deciding between ref4 and ref_addr >>>>>>>>>>> based on context? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> addDIEEntry took a form before my change and I didn't check why >>>>>>>>>> it did. >>>>>>>>>> I will check if all callers always use ref4, if it it true, I >>>>>>>>>> will submit a cleanup patch first. >>>>>>>>>> >>>>>>>>> Done in attached patch. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> I'm still unsure about this worklist thing - do your current >>>>>>>>>>> tests cover the need for the worklist? ie: if we removed that logic, would >>>>>>>>>>> tests fail? Can you describe a specific sequence where the worklist is >>>>>>>>>>> necessary? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> If we are sure that DIEs are always added to an owner before >>>>>>>>>> calling addDIEEntry, we don't need the worklist. >>>>>>>>>> But I saw cases where that was not true, I will get a reduced >>>>>>>>>> testing case. >>>>>>>>>> >>>>>>>>> >>>>>>>>> If we try to assert both DIEs have owner in addDIEEntry, the >>>>>>>>> following testing cases will fail: >>>>>>>>> LLVM :: DebugInfo/X86/multiple-aranges.ll >>>>>>>>> LLVM :: DebugInfo/X86/ref_addr_relocation.ll >>>>>>>>> LLVM :: DebugInfo/X86/stmt-list-multiple-compile-units.ll >>>>>>>>> LLVM :: DebugInfo/two-cus-from-same-file.ll >>>>>>>>> LLVM :: Linker/type-unique-simple-a.ll >>>>>>>>> LLVM :: Linker/type-unique-simple2.ll >>>>>>>>> >>>>>>>>> For ref_addr_relocation, it failed in >>>>>>>>> #5 0x0000000100b169ba in llvm::DwarfDebug::addDIEEntry >>>>>>>>> (this=0x103023600, Die=0x102e14090, Attribute=73, Entry=0x10302a9d0) at >>>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:3071 >>>>>>>>> #6 0x0000000100b040e0 in llvm::CompileUnit::addType >>>>>>>>> (this=0x102e13ec0, Entity=0x102e14090, Ty={<llvm::DIScope> >>>>>>>>> {<llvm::DIDescriptor> = {DbgNode = 0x102e05f30}, <No data fields>}, <No >>>>>>>>> data fields>}, Attribute=73) at >>>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:910 >>>>>>>>> #7 0x0000000100b05bff in >>>>>>>>> llvm::CompileUnit::createGlobalVariableDIE (this=0x102e13ec0, >>>>>>>>> N=0x102e068c0) at >>>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1505 >>>>>>>>> >>>>>>>>> If we look at DwarfCompileUnit.cpp: >>>>>>>>> VariableDIE = new DIE(GV.getTag()); >>>>>>>>> // Add to map. >>>>>>>>> insertDIE(N, VariableDIE); >>>>>>>>> >>>>>>>>> // Add name and type. >>>>>>>>> addString(VariableDIE, dwarf::DW_AT_name, GV.getDisplayName()); >>>>>>>>> addType(VariableDIE, GTy); >>>>>>>>> >>>>>>>>> The VariableDIE is not added to an owner yet when calling addType. >>>>>>>>> >>>>>>>> >>>>>>>> I believe I have addressed all review comments, the patches are >>>>>>>> re-attached here for convenience. >>>>>>>> >>>>>>> >>>>>>> So I'm still thinking about the work list work. >>>>>>> >>>>>>> If we don't know which CU a DIE is in - isn't it, necessarily, going >>>>>>> to be in the current CU (& thus referenced by ref_data4 (using a CU-local >>>>>>> offset), not ref_addr)? >>>>>>> >>>>>> >>>>>> That may be true. But can we prove that? >>>>>> >>>>> >>>>> We really shouldn't add extra complexity to the codebase just because >>>>> we don't know how the codebase works - that's what leads to the kind of >>>>> complexity we see in the debug info handling today. >>>>> >>>> >>>>> >>>>>> There are two ways of handling this: >>>>>> 1> use a worklist to be conservative >>>>>> 2> do not use a worklist, but add an assertion when emitting a DIE A, >>>>>> make sure the DIE referenced with ref4 is indeed in the same CU as DIE A. >>>>>> >>>>> >>>>> Please just add this assertion. If it fires we'll have a test case and >>>>> a concrete justification for this complexity such that should anyone remove >>>>> it later because it looked unnecessary, they'll at least have a failing >>>>> test to explain why it was there in the first plac . >>>>> >>>> >>>> The assertion fails even with a simple testing case when the referenced >>>> DIE has an owner and the DIE itself does not have an owner. >>>> >>> >>> OK, sorry - I should've read your description of the assertion more >>> carefully. I believe the assertion you added wasn't the right thing to test >>> for. >>> >>> I'm not sure there is a correct assertion to add here to detect the case >>> you're describing - perhaps a complex verification after-the-fact could be >>> done, but essentially if we have a DIE that's partially constructed/has no >>> parent we should assume it's in the current unit. >>> >> >> Why should we make the assumption that a DIE without a parent at some >> point should belong to the CU that is constructing the DIE? >> > > Because the code we have today only constructs one CU at a time. I know of > no code that adds anything to prior CUs. Indeed this may become an > invariant one day when we do CU-at-a-time DWARF emission to reduce memory > overhead. There's nothing in the design today that I know of that would > make CU-at-a-time DWARF emission anything other than trivial. We build all > the DIEs for a CU in one go, then move on to the next CU - the CU_Nodes > loop in beginModule does this. >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 Die or CU does not have an owner, what assumption should we make to decide between ref_addr and ref4 inside addDIEEntry? 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. 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. To implement the assertions, we need to keep a list of all DIEs that are constructed by a CU. 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. 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/906bdb32/attachment.html>
David Blaikie
2013-Oct-16 00:22 UTC
[LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
On Tue, Oct 15, 2013 at 4:59 PM, Manman Ren <manman.ren at gmail.com> wrote:> > > > On Tue, Oct 15, 2013 at 2:24 PM, David Blaikie <dblaikie at gmail.com> wrote: > >> >> >> >> On Tue, Oct 15, 2013 at 1:56 PM, Manman Ren <manman.ren at gmail.com> wrote: >> >>> >>> >>> >>> On Tue, Oct 15, 2013 at 1:37 PM, David Blaikie <dblaikie at gmail.com>wrote: >>> >>>> >>>> >>>> >>>> On Tue, Oct 15, 2013 at 1:22 PM, Manman Ren <manman.ren at gmail.com>wrote: >>>> >>>>> >>>>> >>>>> >>>>> On Tue, Oct 15, 2013 at 11:34 AM, David Blaikie <dblaikie at gmail.com>wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Tue, Oct 15, 2013 at 10:51 AM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Tue, Oct 15, 2013 at 10:10 AM, David Blaikie <dblaikie at gmail.com>wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Oct 15, 2013 at 10:05 AM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Oct 9, 2013 at 5:22 PM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Oct 9, 2013 at 1:32 PM, Manman Ren <manman.ren at gmail.com>wrote: >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> David, >>>>>>>>>>> >>>>>>>>>>> Thanks for reviewing! >>>>>>>>>>> >>>>>>>>>>> On Wed, Oct 9, 2013 at 11:36 AM, David Blaikie < >>>>>>>>>>> dblaikie at gmail.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> Might be easier if these were on Phabricator, but here are some >>>>>>>>>>>> thoughts: >>>>>>>>>>>> >>>>>>>>>>>> 0001: >>>>>>>>>>>> This patch generally, while separated for legibility, is >>>>>>>>>>>> untested & difficult to discuss in isolation. >>>>>>>>>>>> >>>>>>>>>>> I agree, this patch adds the functionality but does not use it, >>>>>>>>>>> the 2nd patch uses ref_addr. >>>>>>>>>>> If you think I should merge the two and commit as a single >>>>>>>>>>> patch, let me know. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> I may need to refer to the second patch in reviewing this >>>>>>>>>>>> first one. >>>>>>>>>>>> DwarfDebug.cpp: >>>>>>>>>>>> computeSizeAndOffsets: >>>>>>>>>>>> I believe this produces the wrong offset for the 3rd CU >>>>>>>>>>>> and onwards. computeSizeAndOffset returns the EndOffset which is absolute, >>>>>>>>>>>> not relative to the Offset passed in, so it should be assigned to >>>>>>>>>>>> SecOffset, not added to it. (eg: if you have CUs at 0, 42, and 57 - the >>>>>>>>>>>> first pass through SecOffset will be zero, then it'll be 0 + 42, then on >>>>>>>>>>>> the 3rd it'll be (0 + 42) + 57 which isn't right, it should just be 57). >>>>>>>>>>>> Please add more test coverage while fixing this issue. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> computeSizeAndOffset takes an offset that is relative to the >>>>>>>>>>> start of the CU and returns the offset relative to the CU after laying out >>>>>>>>>>> the DIE. >>>>>>>>>>> The initial offset before laying out the CU DIE is the header >>>>>>>>>>> size, EndOffset will be the offset relative to the CU after laying out the >>>>>>>>>>> whole CU DIE. >>>>>>>>>>> We can think of EndOffset as the size of the whole CU DIE. >>>>>>>>>>> SecOffset is the offset from the Debug Info section, so we can update it by >>>>>>>>>>> adding the CU size. >>>>>>>>>>> >>>>>>>>>>> // Offset from the beginning of debug info section. >>>>>>>>>>> unsigned SecOffset = 0; >>>>>>>>>>> for (SmallVectorImpl<CompileUnit *>::iterator I = CUs.begin(), >>>>>>>>>>> E = CUs.end(); I != E; ++I) { >>>>>>>>>>> (*I)->setDebugInfoOffset(SecOffset); >>>>>>>>>>> unsigned Offset >>>>>>>>>>> sizeof(int32_t) + // Length of Compilation Unit Info >>>>>>>>>>> sizeof(int16_t) + // DWARF version number >>>>>>>>>>> sizeof(int32_t) + // Offset Into Abbrev. Section >>>>>>>>>>> sizeof(int8_t); // Pointer Size (in bytes) >>>>>>>>>>> >>>>>>>>>>> unsigned EndOffset = computeSizeAndOffset((*I)->getCUDie(), >>>>>>>>>>> Offset); >>>>>>>>>>> SecOffset += EndOffset; >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Added more comments in attached patch. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> Eric/Manman: rough design question: compute the absolute offset >>>>>>>>>>>> of each CU within the debug_info section and describe them all relative to >>>>>>>>>>>> a single symbol at the start of the debug_info section, or should we put a >>>>>>>>>>>> label at the start of each CU? >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Either way should work. But since we already have the section >>>>>>>>>>> offset for each CU, describing relative to the start of debug_info section >>>>>>>>>>> saves us a few symbols :) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> 0002: >>>>>>>>>>>> ref_addr_relocation.ll: >>>>>>>>>>>> seems a bit vague in terms of how you test for the >>>>>>>>>>>> relocation. I think it'd make more sense to test the assembly, than the >>>>>>>>>>>> reafobj output, that way you can test that the correct bytes have the >>>>>>>>>>>> relocation rather than just that there's "some" .debug_info relocation in >>>>>>>>>>>> the file. For an example, see test/DebugInfo/X86/tls.ll I wrote - it has >>>>>>>>>>>> some "unannotated" bytes because we still don't have a nice way to annotate >>>>>>>>>>>> location bytes that are DWARF expressions, but it's close - I guess those >>>>>>>>>>>> should be CHECK-NEXTs, though. In any case, you should be able to check a >>>>>>>>>>>> few lines of assembly with the # DW_AT/DW_TAG annotation comments. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I can check for ".quad .Lsection_info+38 #DW_AT_type". >>>>>>>>>>> >>>>>>>>>> Done in attached patch. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> You'd need to add the tu3.cpp from my example if you want >>>>>>>>>>>> to demonstrate that the relocation is actually working as intended and >>>>>>>>>>>> avoiding the bogus result I showed. >>>>>>>>>>>> type-unique-simple-a.ll >>>>>>>>>>>> While I agree that having common test cases helps reduce >>>>>>>>>>>> the number of separate invocations we have, this test case seems like it >>>>>>>>>>>> might be becoming a little hard to follow what's under test. Just going >>>>>>>>>>>> from the diff I'm not sure what's what. Could you give a brief description >>>>>>>>>>>> of the state of type-unique-simple2 files? What's involved? What's it meant >>>>>>>>>>>> to test? What's it actually testing? >>>>>>>>>>>> >>>>>>>>>>> I can add more comments. The source files are included in the >>>>>>>>>>> testing case. I am checking that llvm-link only generates a single copy of >>>>>>>>>>> the struct and the backend generates a single DIE and uses ref_addr. >>>>>>>>>>> The changes are to check "the backend generates a single DIE and >>>>>>>>>>> uses ref_addr". >>>>>>>>>>> >>>>>>>>>> Done in attached patch. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> DIE.h: >>>>>>>>>>>> checkCompileUnit could probably be called >>>>>>>>>>>> "getCompileUnitOrNull", the name "check*" seems to imply that it does some >>>>>>>>>>>> checking, which isn't true. >>>>>>>>>>>> >>>>>>>>>>> Will do. >>>>>>>>>>> >>>>>>>>>> Done in attached patch. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> DwarfCompileUnit.cpp: >>>>>>>>>>>> the "istype || issubprogram" check should probably be >>>>>>>>>>>> pulled out into a separate function, something like "isShareableAcrossCUs" >>>>>>>>>>>> or something like that (please, that's not the best name, let's discuss it >>>>>>>>>>>> further before we settle on a name) so that getDIE and insertDIE are sure >>>>>>>>>>>> to use the same test at all times. >>>>>>>>>>>> >>>>>>>>>>> Yes, the condition is shared between getDIE and insertDIE. I >>>>>>>>>>> like isSharableAcrossCUs, because that is why the map is in DwarfDebug >>>>>>>>>>> instead of CompileUnit. >>>>>>>>>>> >>>>>>>>>> Done in attached patch. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> Why does addDIEEntry take a form? When does the caller get >>>>>>>>>>>> to choose the form rather than the callee deciding between ref4 and >>>>>>>>>>>> ref_addr based on context? >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> addDIEEntry took a form before my change and I didn't check why >>>>>>>>>>> it did. >>>>>>>>>>> I will check if all callers always use ref4, if it it true, I >>>>>>>>>>> will submit a cleanup patch first. >>>>>>>>>>> >>>>>>>>>> Done in attached patch. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I'm still unsure about this worklist thing - do your current >>>>>>>>>>>> tests cover the need for the worklist? ie: if we removed that logic, would >>>>>>>>>>>> tests fail? Can you describe a specific sequence where the worklist is >>>>>>>>>>>> necessary? >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> If we are sure that DIEs are always added to an owner before >>>>>>>>>>> calling addDIEEntry, we don't need the worklist. >>>>>>>>>>> But I saw cases where that was not true, I will get a reduced >>>>>>>>>>> testing case. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> If we try to assert both DIEs have owner in addDIEEntry, the >>>>>>>>>> following testing cases will fail: >>>>>>>>>> LLVM :: DebugInfo/X86/multiple-aranges.ll >>>>>>>>>> LLVM :: DebugInfo/X86/ref_addr_relocation.ll >>>>>>>>>> LLVM :: DebugInfo/X86/stmt-list-multiple-compile-units.ll >>>>>>>>>> LLVM :: DebugInfo/two-cus-from-same-file.ll >>>>>>>>>> LLVM :: Linker/type-unique-simple-a.ll >>>>>>>>>> LLVM :: Linker/type-unique-simple2.ll >>>>>>>>>> >>>>>>>>>> For ref_addr_relocation, it failed in >>>>>>>>>> #5 0x0000000100b169ba in llvm::DwarfDebug::addDIEEntry >>>>>>>>>> (this=0x103023600, Die=0x102e14090, Attribute=73, Entry=0x10302a9d0) at >>>>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:3071 >>>>>>>>>> #6 0x0000000100b040e0 in llvm::CompileUnit::addType >>>>>>>>>> (this=0x102e13ec0, Entity=0x102e14090, Ty={<llvm::DIScope> >>>>>>>>>> {<llvm::DIDescriptor> = {DbgNode = 0x102e05f30}, <No data fields>}, <No >>>>>>>>>> data fields>}, Attribute=73) at >>>>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:910 >>>>>>>>>> #7 0x0000000100b05bff in >>>>>>>>>> llvm::CompileUnit::createGlobalVariableDIE (this=0x102e13ec0, >>>>>>>>>> N=0x102e068c0) at >>>>>>>>>> /Users/manmanren/llvm_git/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1505 >>>>>>>>>> >>>>>>>>>> If we look at DwarfCompileUnit.cpp: >>>>>>>>>> VariableDIE = new DIE(GV.getTag()); >>>>>>>>>> // Add to map. >>>>>>>>>> insertDIE(N, VariableDIE); >>>>>>>>>> >>>>>>>>>> // Add name and type. >>>>>>>>>> addString(VariableDIE, dwarf::DW_AT_name, >>>>>>>>>> GV.getDisplayName()); >>>>>>>>>> addType(VariableDIE, GTy); >>>>>>>>>> >>>>>>>>>> The VariableDIE is not added to an owner yet when calling addType. >>>>>>>>>> >>>>>>>>> >>>>>>>>> I believe I have addressed all review comments, the patches are >>>>>>>>> re-attached here for convenience. >>>>>>>>> >>>>>>>> >>>>>>>> So I'm still thinking about the work list work. >>>>>>>> >>>>>>>> If we don't know which CU a DIE is in - isn't it, necessarily, >>>>>>>> going to be in the current CU (& thus referenced by ref_data4 (using a >>>>>>>> CU-local offset), not ref_addr)? >>>>>>>> >>>>>>> >>>>>>> That may be true. But can we prove that? >>>>>>> >>>>>> >>>>>> We really shouldn't add extra complexity to the codebase just because >>>>>> we don't know how the codebase works - that's what leads to the kind of >>>>>> complexity we see in the debug info handling today. >>>>>> >>>>> >>>>>> >>>>>>> There are two ways of handling this: >>>>>>> 1> use a worklist to be conservative >>>>>>> 2> do not use a worklist, but add an assertion when emitting a DIE >>>>>>> A, make sure the DIE referenced with ref4 is indeed in the same CU as DIE A. >>>>>>> >>>>>> >>>>>> Please just add this assertion. If it fires we'll have a test case >>>>>> and a concrete justification for this complexity such that should anyone >>>>>> remove it later because it looked unnecessary, they'll at least have a >>>>>> failing test to explain why it was there in the first plac . >>>>>> >>>>> >>>>> The assertion fails even with a simple testing case when the >>>>> referenced DIE has an owner and the DIE itself does not have an owner. >>>>> >>>> >>>> OK, sorry - I should've read your description of the assertion more >>>> carefully. I believe the assertion you added wasn't the right thing to test >>>> for. >>>> >>>> I'm not sure there is a correct assertion to add here to detect the >>>> case you're describing - perhaps a complex verification after-the-fact >>>> could be done, but essentially if we have a DIE that's partially >>>> constructed/has no parent we should assume it's in the current unit. >>>> >>> >>> Why should we make the assumption that a DIE without a parent at some >>> point should belong to the CU that is constructing the DIE? >>> >> >> Because the code we have today only constructs one CU at a time. I know >> of no code that adds anything to prior CUs. Indeed this may become an >> invariant one day when we do CU-at-a-time DWARF emission to reduce memory >> overhead. There's nothing in the design today that I know of that would >> make CU-at-a-time DWARF emission anything other than trivial. We build all >> the DIEs for a CU in one go, then move on to the next CU - the CU_Nodes >> loop in beginModule does this. >> > > 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?> 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).> 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?> 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.> 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).> 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). - 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/51b11d0f/attachment.html>
Possibly Parallel Threads
- [LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
- [LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
- [LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
- [LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
- [LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication