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

[Eric Christopher]

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
>