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

[Tim Northover]

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.

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

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

Tim.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: condcall_fixup_2.patch
Type: text/x-patch
Size: 7851 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120326/2092d433/attachment.bin>