[llvm-commits] [PATCH] Common symbols aren't aligned in RuntimeDyld

[Amara Emerson]

Ping. Ok to commit?

Amara

-----Original Message-----
From: Amara Emerson [mailto:amara.emerson at gmail.com] 
Sent: 16 October 2012 22:02
To: Eric Christopher
Cc: Amara Emerson; llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] [PATCH] Common symbols aren't aligned in
RuntimeDyld

Thanks for the feedback. There was no good reason for not using a
symbol map of pairs, the code's more succinct now.

I've split the patch into 3 parts, one for implementing the
getSymbolValue stuff, one for using it in RuntimeDyld and one for the
test.

>And test the implementation and use of the common symbol bits if you can.
The test case initially attached will exercise the code (on ELF at
least) so I'm not sure what else you mean.

Amara


On 16 October 2012 01:05, Eric Christopher <echristo at gmail.com> wrote:
> One more comment: You should break the patch up into "implement the new
stuff",
> "use the new stuff" and "use the new stuff to fix my common symbol
> alignment problem".
>
> And test the implementation and use of the common symbol bits if you can.
>
> -eric
>
> On Mon, Oct 15, 2012 at 4:36 PM, Eric Christopher <echristo at gmail.com>
wrote:
>> Adding a getSymbolValue to Object isn't a problem. Grabbing the value
>> of an arbitrary symbol is generally useful.
>>
>> Now a few comments on the patch itself:
>>
>> +  uint64_t AlignOffset = 0;
>>
>> could probably put this closer to:
>>
>> +    CommonSymbolMap::const_iterator it_align =
CSAlignments.find(it->first);
>> +    if (it_align != CSAlignments.end()) {
>> +      // This symbol has an alignment requirement.
>> +      AlignOffset = OffsetToAlignment((uint64_t)Addr, it_align->second);
>> +      Addr += AlignOffset;
>> +      Offset += AlignOffset;
>> +      DEBUG(dbgs() << "Allocating common symbol " << Name << " address "
<<
>> +                      format("0x%x\n", Addr));
>> +    }
>>
>> this code.
>>
>> Also, out of curiosity why a new map rather than making the value of
>> the original common symbol map a pair of values?
>>
>> +unsigned RuntimeDyldELF::getCommonSymbolAlignment(const symbol_iterator
&si) {
>> +  uint64_t Align;
>> +  Check(si->getValue(Align));
>> +  return Align;
>> +}
>>
>> This needs a block comment explaining that the value of an SHN_COMMON
>> symbol is the alignment. Also it should probably take a symbol rather
>> than an iterator.
>>
>> -eric
>>
>> On Sun, Oct 14, 2012 at 9:02 AM, Amara Emerson <amara.emerson at arm.com>
wrote:
>>> The new refactored patch is attached. The ELF specific stuff was moved
into
>>> an overriding virtual method.
>>>
>>> Amara
>>>
>>> -----Original Message-----
>>> From: Jim Grosbach [mailto:grosbach at apple.com]
>>> Sent: 12 October 2012 21:58
>>> To: Amara Emerson
>>> Cc: Amara Emerson; echristo at google.com; llvm-commits at cs.uiuc.edu LLVM
>>> Subject: Re: [llvm-commits] [PATCH] Common symbols aren't aligned in
>>> RuntimeDyld
>>>
>>>
>>> On Oct 12, 2012, at 1:50 PM, Amara Emerson <amara.emerson at gmail.com>
wrote:
>>>
>>>> That makes sense, some refactoring should allow it to be moved into
>>>> RuntimeDyldELF.
>>>>
>>> Perfect. Thanks!
>>>
>>>> Regarding the use of ObjectFile in the first place, do you think
>>>> adding getSymbolValue() stubs to MachO and COFF are reasonable? I'm
>>>> not keen on the idea, but the abstraction doesn't leave any other
>>>> options that I can see.
>>>
>>> I'm not sure I see much alternative, either. Eric, any thoughts on
approach
>>> there?
>>>
>>>>
>>>> Amara
>>>>
>>>> On 12 October 2012 19:47, Jim Grosbach <grosbach at apple.com> wrote:
>>>>> The getObjFormat() is breaking the intended abstraction. If there's
file
>>> format specific logic required, that should be in the platform specific
>>> bits. RuntimeDyldELF in this case.
>>>>>
>>>>> Note that this is a good example of why I still don't much like the
use
>>> of the libObject ObjectBuffer stuff here (totally not your fault, just a
>>> general observation). Eric, if you're still keen to clean some of this
stuff
>>> up, here's one for you. ;)
>>>>>
>>>>> -Jim
>>>>> On Oct 12, 2012, at 7:05 AM, Amara Emerson <amara.emerson at arm.com>
wrote:
>>>>>
>>>>>> A new patch to use the OffsetToAlignment function in MathExtras.h,
>>> recommended by Tim during review of the lli alignment patch, instead of
>>> manual alignment offset calculation.
>>>>>>
>>>>>> Amara
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: llvm-commits-bounces at cs.uiuc.edu
>>> [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Amara Emerson
>>>>>> Sent: 12 October 2012 13:45
>>>>>> To: llvm-commits at cs.uiuc.edu
>>>>>> Subject: [llvm-commits] [PATCH] Common symbols aren't aligned in
>>> RuntimeDyld
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I noticed that common symbols in the RuntimeDyld do not have their
>>> alignment
>>>>>> requirements honoured at all. For ELF relocatable objects, the value
>>> field
>>>>>> of a common symbol in the symbol table is used to encode the
alignment
>>>>>> requirement. However, there seems to be no way to retrieve this
>>> information
>>>>>> currently and so the symbols cannot be aligned.
>>>>>>
>>>>>> The patch fixes this by adding a hook into ELFObjectFile to return
the
>>> value
>>>>>> of a symbol, and ensures proper alignment. The solution currently
>>> applies to
>>>>>> ELF only, the relevant hooks in MachO and COFF have been stubbed for
>>> now. A
>>>>>> test case is included as well.
>>>>>>
>>>>>> Please review the attached patch.
>>>>>>
>>>>>>
>>>
Amara<common-symbol-alignment-revised1.patch>_______________________________
>>> ________________
>>>>>> llvm-commits mailing list
>>>>>> llvm-commits at cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits