[LLVMdev] [Debug Info PATCH] for support of ref_addr and removal of DIE duplication
David Blaikie
dblaikie at gmail.com
Tue Oct 15 14:57:31 PDT 2013
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>
More information about the llvm-dev
mailing list