[llvm-commits] [PATCH] Dwarf: support for LTO where a single object file can have multiple line tables

[Eric Christopher]

LGTM.

-eric


On Tue, Feb 5, 2013 at 1:19 PM, Manman Ren <mren at apple.com> wrote:

>
> I thought I covered that issue, but I didn't make all the necessary
> changes.
>
> Updated patch: change int->unsigned
>
> Thanks,
> Manman
>
>
> On Feb 5, 2013, at 1:05 PM, Eric Christopher <echristo at gmail.com> wrote:
>
> Missed the int->unsigned. With that change it's great. Thanks for all of
> the work here. :)
>
> -eric
>
>
> On Tue, Feb 5, 2013 at 12:49 PM, Manman Ren <mren at apple.com> wrote:
>
>>
>> Updated patch attached which addresses the following issues:
>> b, c, e, f and g
>>
>> We now generate a symbol for start of the first line table and a Lset for
>> the difference between section_line and the first line table, so I updated
>> 7 testing cases for that.
>> The special handling of "CUID==0" is gone.
>>
>> This patch passes "make check-all" at r174431.
>>
>> Thanks,
>> Manman
>>
>>
>>
>> On Feb 1, 2013, at 12:40 AM, Eric Christopher wrote:
>>
>> Hi Manman,
>>
>> Looking better. Few more comments/items for discussion:
>>
>> a) Concerned about the performance difference you mentioned with
>> MCLineDivisions, but for now we can just use the map you've got, I'll
>> experiment with just doing the lookup on the densemap myself.
>>
>> b)  Let's go ahead and get a typedef for this:
>>
>> +    std::map<int, MCLineEntryCollection> MCLineDivisions;
>>
>> and then use it.
>>
>> c) Also on that note since CU ids are all non-negative it should be
>> unsigned. (Should fix some casting you had to do as well).
>>
>> d)  This comment:
>>
>> +  // CUID and MCLineTableSymbols are set in DwarfDebug, when DwarfDebug
>> does
>> +  // not exist, CUID will be 0 and MCLineTableSymbols will be empty.
>> +  // Handle Compile Unit 0, the line table start symbol is the section
>> symbol.
>>
>> I'm not sure there's a reason to handle CU 0 any different than the rest
>> of the CUs is there? It should just be the contents of the line table for
>> that compile unit. Basically trying to unify the handling as much as
>> possible there.
>>
>> e)  Similarly instead of this:
>>
>> +  if (CUID == 0)
>> +    // This is the first line table.
>> +    LineStartSym = context.CreateTempSymbol();
>>
>> you could set the symbol when you create the rest of them.
>>
>> f) Similarly again:
>>
>> +  // Define start line table label for each Compile Unit.
>> +  MCSymbol *LineTableStartSym = NewCU->getUniqueID() == 0 ?
>> +                                Asm->GetTempSymbol("section_line") :
>> +                                Asm->GetTempSymbol("line_table_start",
>> +                                                   NewCU->getUniqueID());
>>
>> Go ahead and just use line_table_start for all of the CUs to avoid the
>> extra conditionals.
>>
>> g) And then here:
>>
>> +  else if (NewCU->getUniqueID() == 0)
>> +    NewCU->addUInt(Die, dwarf::DW_AT_stmt_list, dwarf::DW_FORM_data4, 0);
>>    else
>> -    NewCU->addUInt(Die, dwarf::DW_AT_stmt_list, dwarf::DW_FORM_data4, 0);
>> +    NewCU->addDelta(Die, dwarf::DW_AT_stmt_list, dwarf::DW_FORM_data4,
>> +                    LineTableStartSym,
>> Asm->GetTempSymbol("section_line"));
>>
>> if you save the section line symbol earlier (there's a spot where we save
>> the section symbols we'll use later) then everything can be a label
>> subtraction.
>>
>> h) Might be nice to check something with .file/.loc directives (more than
>> test/MC/loc.s) to make sure the line table is being emitted as you'd
>> expect, but that can be a follow on after we get the rest of this done :)
>>
>> -eric
>>
>>
>>
>> On Thu, Jan 31, 2013 at 10:03 AM, Manman Ren <mren at apple.com> wrote:
>>
>>>
>>> Patch updated:
>>> 1> added comments
>>> 2> use 0 as default CUID to simplify logic
>>> 3> use std::map and get rid of MCLineIDs
>>>
>>> Please review.
>>>
>>> Thanks,
>>> Manman
>>>
>>>
>>> On Jan 30, 2013, at 5:28 PM, Eric Christopher <echristo at gmail.com>
>>> wrote:
>>>
>>>
>>> Any thoughts here?
>>>>
>>>> I used MCLineIDs for fast look-up, do you have any suggestion on what
>>>> to use here?
>>>>
>>>>
>>> It took me a while, but I see what you're doing. Effectively I think you
>>> can just use a lookup in MCLineDivisionsto see if anything is in the set?
>>> Since the SmallSet is going to be stored in exactly the same way anyhow.
>>> Also since I've not bothered to look are the CU ids dense or sparse? i.e.
>>> 0, 1, 2 or in metadata node order (0, 12, 255, 444)?
>>>
>>>
>>>>  The bit with CUID = -1 or CUID == 0 seems confusing, can you explain
>>>>> this a bit? At times -1 seems to be for invalid CUs and at other times the
>>>>> first/only CU?
>>>>>
>>>>> "-1" is the default CUID, when DwarfDebug does not exist.
>>>>> So the first line table will be for CUID 0 when DwarfDebug exists, and
>>>>> will be for CUID -1 when DwarfDebug does not exist.
>>>>>
>>>>>
>>>> This seems a bit confusing. Since, for example, we emit a CU for
>>>> assembly files and so something will always be emitting a CU. I could
>>>> easily not be seeing something though. At worst it really needs some
>>>> comments.
>>>>
>>>> I added some comments.
>>>> +  // CUID and MCLineTableSymbols are set in DwarfDebug, when
>>>> DwarfDebug does
>>>> +  // not exist, CUID will be -1 and MCLineTableSymbols will be empty.
>>>>
>>>> Updated patch is attached.
>>>>
>>>>
>>> Right. I'm not sure why this division exists. Is there some reason why
>>> it's -1 in the other case that I'm missing?
>>>
>>> -eric
>>>
>>>
>>>
>>>
>>
>>
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130205/02753c20/attachment.html>