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

[Jim Grosbach]

On Mar 26, 2012, at 9:53 AM, Tim Northover <tim.northover at arm.com> wrote:

> Thanks for the review Jim.
> 
>> 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.
> 
> Good idea. I've updated the patch. As well as this change, I renamed 
> fixup_arm_bl to fixup_arm_uncondbl to clarify its usage and added a test that 
> this fixup is applied by MC.

Makes sense.

> 
>> Second, MachO still wants a relocation for these instructions.
>> processFixupValue() needs updated to do that.
> 
> I'm afraid I don't quite follow here. The patch I sent already adds a check 
> for fixup_arm_condbl to processFixupValue with the others. Is there something 
> else I should have done?

Nope, that's it. Something probably didn't apply right earlier on my end or I was just blind and missed it. Regardless, the check we need is there in the current patch, so it's taken care of.

>> 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.
> 
> Possibly, though I may not understand the important details here. 
> 
> Is this is deferring to the linker something LLVM could handle properly itself 
> if we could be bothered? (though possibly by violating a clean structure: we'd 
> have to know whether a destination was ARM or Thumb when relaxing the fixup).

Theoretically, yes. The fixup would have to adjust the opcode as well as the destination address in that case. That's not normally something fixups do. Off the top of my head, we could possible leverage branch relaxation to handle this aspect of things if we really want to. I'm not sure it's worth it though. Just letting the linker always handle it keeps things simpler with minimal additional overhead.

+  // The following fixups handle the ARM BL instructions. These can be
+  // conditionalised, however the ARM ELF ABI requires a different relocation in
+  // that case: R_ARM_JUMP24 instead of R_ARM_CALL.

Two minor things here. Grammar nit: "[…]conditionalized; however, the[…]." An additional sentence explaining why the relocation needs to be different would help. Without this background discussion, I'd be scratching my head at this.

OK to commit w/ those tweaks. Thanks again for doing this!

Regards,
  Jim