[llvm-commits] [PATCH] ARM: conditional BL should use fixup_arm_condbranch

[Jim Grosbach]

On Mar 26, 2012, at 8:51 AM, Tim Northover <Tim.Northover at arm.com> wrote:

> On Friday 23 Mar 2012 00:04:52 Måns Rullgård wrote:
>> By using the same fixup kind for both, the distinction is lost in the
>> object writers.  Perhaps adding another fixup kind for conditional calls
>> (fixup_arm_condbl?) would be a solution.  The Mach-o writer could handle
>> this identically to fixup_arm_bl, while the ELF writer would select the
>> appropriate relocation type.  To me this seems cleaner than littering
>> the otherwise generic code emitter with platform-specifics.
> 
> We've also hit this issue, it causes fairly widespread regressions on ELF 
> runtime tests.
> 
> I'd tend to agree that's a better fix and I've attached a patch following that 
> route. Could someone review it please?
> 
> Regards.
> 
> Tim.<condcall_fixup.patch>

The case for a new fixup kind is compelling here, I agree.

The patch generally looks fine, with a couple of comments.

First, purely on the aesthetic, I'd like to capture a short summary of all of this in a comment about the fixups. It can be kinda reconstructed from the ARMFixupKinds.h comments, but something that ties it all together would be good. Just consolidating and rephrasing the comments there into a more cohesive form seems reasonable.

Second, MachO still wants a relocation for these instructions. processFixupValue() needs updated to do that.

Lastly, somewhat orthogonal to the original issue we're discussing, given the description of how the ELF relocations work, it sounds like you do still want a relocation for every call, like MachO does, but just need which relocation to use to be different. Otherwise you'll run into the same issue darwin did and have MC statically resolve a BL[X] instruction's destination and not issue a relocation, but the interworking will be wrong. Something to consider, anyway.

-Jim