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

[Måns Rullgård]

Jim Grosbach <grosbach at apple.com> writes:

> 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.

I don't have any fruit gear and know next to nothing about MachO, so
someone else will have 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.

I get proper ELF relocations for BL<c> instructions with this patch.

-- 
Måns Rullgård
mans at mansr.com