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

[Eric Christopher]

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/4aff083d/attachment.html>