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

Thu Nov 1 17:31:24 PDT 2012

Yep. Sorry about the delay.

-eric

On Thu, Nov 1, 2012 at 5:25 PM, Jim Grosbach <grosbach at apple.com> wrote:
> 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
>