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

[Eric Christopher]

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/20130201/be3deb4a/attachment.html>