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