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

[Manman Ren]

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/d4efd5cf/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: line_table_4.patch
Type: application/octet-stream
Size: 18264 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130205/d4efd5cf/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130205/d4efd5cf/attachment-0001.html>