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

[Jim Grosbach]

LGTM so long as Eric is OK with the updates per his comments. Eric?

-Jim

On Oct 26, 2012, at 1:08 AM, Amara Emerson <Amara.Emerson at arm.com> wrote:

> 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
> 
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits