Sorry, I have been very behind on patch reviews. Bob and Jim, do you have any opinions about this? Evan On Jun 2, 2010, at 5:18 AM, Renato Golin wrote:> Hi, > > Is there any interest in this patch? Is there any better way of doing this (that will be accepted mainstream)? > > I noticed my cast check was wrong (LLVM cast asserts, rather than return a null pointer). Also, including ARMMCRelocation.h, that defines the relocation types (missing from last patch, sorry). > > Thanks, > --renato > > > From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On Behalf Of Renato Golin > Sent: 28 May 2010 16:06 > To: llvmdev at cs.uiuc.edu > Subject: [LLVMdev] Patch proposal: ARM MC relocations > > Hi all, > > I did some changes to the AsmPrinter in order to print relocation information in GAS format compatible with ARM ELF. It doesn’t look like the best solution, but there were some problems and the fact that this is my first attempt to change LLVM. > > My original assumption was that, changing MCExpr to accept relocation information, we could propagate that down later to whatever gets written from it. To change that, one needs to overwrite the LowerConstant function in AsmPrinter, but that’s a static function that rely on other static functions in the same file. Also, ARMMCInstLower (my first guess) seems to have not being used to generate anything. > > I know that MC is radically changing the area as we speak, so I also didn’t want to spend too much time on something that will disappear in a few months. I’m happy to conform to the new specifications if there is one and it works for ARM. > > In the end, the change I had to do was simpler, less generic and a bit hacky, so I’m sending this patch to the list (instead of the patch list directly) to request for comments. The patch applies to the trunk (a few minutes ago). > > Cheers, > --renato > <arm-relocations.patch><ARMMCRelocation.h>_______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100616/e4a6a224/attachment.html>
Is there a way to handle this entirely in the ARM AsmPrinter bits? Adding pieces
to the generic MC stuff feels a bit like overkill at first impression.
On a minor note:
You probably want dyn_cast<> instead of cast<> so you can do
something like:
if (const MCSectionELF* SectionELF = dyn_cast<MCSectionELF>(Section)) {
// ...
}
-Jim
On Jun 16, 2010, at 3:44 PM, Evan Cheng wrote:
> Sorry, I have been very behind on patch reviews. Bob and Jim, do you have
any opinions about this?
>
> Evan
>
> On Jun 2, 2010, at 5:18 AM, Renato Golin wrote:
>
>> Hi,
>>
>> Is there any interest in this patch? Is there any better way of doing
this (that will be accepted mainstream)?
>>
>> I noticed my cast check was wrong (LLVM cast asserts, rather than
return a null pointer). Also, including ARMMCRelocation.h, that defines the
relocation types (missing from last patch, sorry).
>>
>> Thanks,
>> --renato
>>
>>
>> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu] On Behalf Of Renato Golin
>> Sent: 28 May 2010 16:06
>> To: llvmdev at cs.uiuc.edu
>> Subject: [LLVMdev] Patch proposal: ARM MC relocations
>>
>> Hi all,
>>
>> I did some changes to the AsmPrinter in order to print relocation
information in GAS format compatible with ARM ELF. It doesn’t look like the best
solution, but there were some problems and the fact that this is my first
attempt to change LLVM.
>>
>> My original assumption was that, changing MCExpr to accept relocation
information, we could propagate that down later to whatever gets written from
it. To change that, one needs to overwrite the LowerConstant function in
AsmPrinter, but that’s a static function that rely on other static functions in
the same file. Also, ARMMCInstLower (my first guess) seems to have not being
used to generate anything.
>>
>> I know that MC is radically changing the area as we speak, so I also
didn’t want to spend too much time on something that will disappear in a few
months. I’m happy to conform to the new specifications if there is one and it
works for ARM.
>>
>> In the end, the change I had to do was simpler, less generic and a bit
hacky, so I’m sending this patch to the list (instead of the patch list
directly) to request for comments. The patch applies to the trunk (a few minutes
ago).
>>
>> Cheers,
>> --renato
>>
<arm-relocations.patch><ARMMCRelocation.h>_______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20100616/080bb82d/attachment.html>
On 17 June 2010 00:06, Jim Grosbach <grosbach at apple.com> wrote:> Is there a way to handle this entirely in the ARM AsmPrinter bits? Adding > pieces to the generic MC stuff feels a bit like overkill at first > impression.Hi Jim and Evan, At first, I thought I could do that, but it is MCAsmStreamer that is printing the final value of most symbols together with their EOL. So, unless I start using rewinding streams or print the symbols myself, I can't see other way. I could add a call-back there, instead of calling it Relocation->print(OS), call it TargetSpecific->prePrint(OS) and posPrint(OS), just before and after printing the value. Or even make them simple methods in ARM/MCAsmInfo, but then it'd be more than just "info" in there. Or have a reference on MAI to the target's asm printer, that might be the easiest, non-intrusive change... Also, I noted all relocation is dealt with by the backend. Just like GCC ignoring Clang's exception table completely or assigning the correct relocation information even when no information is available in the asm file. I understand that there are some default values we can omit from the assembly output (initialisation areas are one example of that), but when you start using multiple tool chains to get stuff done, it's better be explicit than rely on "defaults". In that example, for instance, I compiled with clang to IR, llc to asm, gcc to object and armlink to executable. Most of the examples I tested worked fine, but the missing relocation information made it difficult to interoperate.> You probably want dyn_cast<> instead of cast<> so you can do something like: > if (const MCSectionELF* SectionELF = dyn_cast<MCSectionELF>(Section)) { > // ... > }Precisely. I didn't want to use c++ dynamic_cast directly, but I only found out about dyn_cast after I sent the patch. Apologies. cheers, --renato