[llvm-commits] [PATCH] MC ARM Thumb: fix incorrect relocation generation (PR11214)
Kristof Beyls
kristof.beyls at arm.com
Wed Dec 21 07:01:01 PST 2011
Hi Jim,
(I included Rafael and Koan-sin in cc since they committed the code that
this
proposed patch changes)
Thanks for your answer.
I'm not an ELF expert, but I do think that R_ARM_THM_CALL is always the
correct
relocation for Thumb BL and BLX instructions, for the following reasons:
* I checked with 2 ELF experts in the office, and they agree that Thumb BL
and
BLX instructions always need to have relocation R_ARM_THM_CALL, no matter
whether it's a PLT or a non-PLT reference. This is in line with what the
ARM
AAELF ABI states, section 4.7.1.6. "Static Thumb32 relocations":
"R_ARM_THM_CALL is used to relocate Thumb BL (and ARMv5 Thumb BLX)
instructions."
There's no distinction between PLT or non-PLT references.
* I ran the assembler code in the regression test in the patch through both
GNU
as and armasm (the assembler from ARM Compiler 5, a.k.a. 'armcc'). They
both
produce the R_ARM_THUMB relocation, as expected.
* We found this issue because we were trying to compile and run a bare metal
version of dhrystone. Without the patch, the final image is not linked
correctly, resulting in an infinite loop. With this patch, it compiles
and
links correctly.
The code in the case statement that will be changed, came from a single
commit,
r131748, and was discussed in the following mail thread:
http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-May/040088.html.
Rafael or Koan-sin, do you happen to know or remember why patch r131748
special
cases MCSymbolRefExpr::VK_ARM_PLT?
$ svn log -c 131748
------------------------------------------------------------------------
r131748 | rafael | 2011-05-20 21:01:01 +0100 (Fri, 20 May 2011) | 4 lines
fixes target address tBL and tBLX and sets relocation type
of tBL/tBLX to R_ARM_THM_CALL (ARM ELF 4.7.1.6)
Patch by koan-sin tan.
------------------------------------------------------------------------
$ svn diff -c 131748
...
Index: lib/MC/ELFObjectWriter.cpp
===================================================================
--- lib/MC/ELFObjectWriter.cpp (revision 131747)
+++ lib/MC/ELFObjectWriter.cpp (revision 131748)
@@ -1441,6 +1441,17 @@
case ARM::fixup_t2_movw_lo16_pcrel:
Type = ELF::R_ARM_THM_MOVW_PREL_NC;
break;
+ case ARM::fixup_arm_thumb_bl:
+ case ARM::fixup_arm_thumb_blx:
+ switch (Modifier) {
+ case MCSymbolRefExpr::VK_ARM_PLT:
+ Type = ELF::R_ARM_THM_CALL;
+ break;
+ default:
+ Type = ELF::R_ARM_NONE;
+ break;
+ }
+ break;
}
...
Thanks,
Kristof
> -----Original Message-----
> From: Jim Grosbach [mailto:grosbach at apple.com]
> Sent: 20 December 2011 21:08
> To: Kristof Beyls
> Cc: James Molloy; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] MC ARM Thumb: fix incorrect
> relocation generation (PR11214)
>
> Right. I agree it's fairly clear that R_ARM_NONE is wrong. It's not
> clear, however, that R_ARM_THM_CALL is correct for non-PLT relocations.
> I suspect it's not else that conditional wouldn't have been there in
> the first place. I'm hoping that an ELF knowledgeable person will chime
> in and let us know for sure one way or the other.
>
> -Jim
>
>
> On Dec 20, 2011, at 12:06 PM, Kristof Beyls wrote:
>
> > The reason why I thought the patch would be OK is that the patch only
> > changes the behaviour for the thumb bl and blx instructions (i.e.
> only for
> > the cases ARM::fixup_arm_thumb_bl and ARM::fixup_arm_thumb_blx.
> >
> > For these instructions, I can't see why R_ARM_NONE would ever be the
> correct
> > relocation type. The definition of R_ARM_NONE from the AAELF ABI:
> > """
> > R_ARM_NONE records that the section containing the place to be
> relocated depends on the section defining the symbol mentioned in the
> relocation directive in a way otherwise invisible to the static linker.
> The effect is to prevent removal of sections that might otherwise
> appear to be unused.
> > """
> >
> > Thanks,
> >
> > Kristof
> >
> >> -----Original Message-----
> >> From: Jim Grosbach [mailto:grosbach at apple.com]
> >> Sent: 20 December 2011 19:33
> >> To: James Molloy
> >> Cc: Kristof Beyls; llvm-commits at cs.uiuc.edu
> >> Subject: Re: [llvm-commits] [PATCH] MC ARM Thumb: fix incorrect
> >> relocation generation (PR11214)
> >>
> >> Yeah, that's my guess. An assert(0 "unimplemented non-PLT call!") or
> >> something would have been better if that's the case exactly to
> prevent
> >> this sort of head-scratching. :)
> >>
> >> -Jim
> >>
> >> On Dec 20, 2011, at 11:21 AM, James Molloy wrote:
> >>
> >>> Yeah, I was confused about that too.
> >>>
> >>> I couldn't comprehend why that conditional was even there in the
> >> first place - an R_ARM_NONE relocation is *never* required for a
> call
> >> (it's a special no-op reloc).
> >>>
> >>> The only thing I could think of is that someone wanted to fix the
> >> relocation given to PLT entries and decided to keep the a broken
> >> behaviour for the default case, perhaps they had no knowledge about
> it.
> >>> ________________________________________
> >>> From: llvm-commits-bounces at cs.uiuc.edu [llvm-commits-
> >> bounces at cs.uiuc.edu] On Behalf Of Jim Grosbach [grosbach at apple.com]
> >>> Sent: 20 December 2011 19:07
> >>> To: Kristof Beyls
> >>> Cc: llvm-commits at cs.uiuc.edu
> >>> Subject: Re: [llvm-commits] [PATCH] MC ARM Thumb: fix incorrect
> >> relocation generation (PR11214)
> >>>
> >>> Someone with ELF knowledge should look at this. I'm a bit nervous
> >> about removing the conditional as it seems to imply that there's an
> >> different relocation that should be generated depending on the
> variant
> >> kind.
> >>>
> >>> -Jim
> >>>
> >>> On Dec 20, 2011, at 5:43 AM, Kristof Beyls wrote:
> >>>
> >>>> Hi,
> >>>>
> >>>> Please find a patch for fixing incorrect generation of relocation
> >>>> information on Thumb function calls.
> >>>> This should fix quite a few incorrect code generation problems in
> >> Thumb mode
> >>>> when using the integrated assembler.
> >>>> I believe this fixes PR11214
> >>>> (http://www.llvm.org/bugs/show_bug.cgi?id=11214).
> >>>>
> >>>> Is this OK?
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Kristof
> >>>>
> >>
> <fix_thumbcall_reloc.patch>____________________________________________
> >> ___
> >>>> llvm-commits mailing list
> >>>> llvm-commits at cs.uiuc.edu
> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>>
> >>> _______________________________________________
> >>> llvm-commits mailing list
> >>> llvm-commits at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>>
> >>>
> >>> -- IMPORTANT NOTICE: The contents of this email and any attachments
> >> are confidential and may also be privileged. If you are not the
> >> intended recipient, please notify the sender immediately and do not
> >> disclose the contents to any other person, use it for any purpose,
> or
> >> store or copy the information in any medium. Thank you.
> >>>
> >>
> >>
> >
> >
> > -- IMPORTANT NOTICE: The contents of this email and any attachments
> are confidential and may also be privileged. If you are not the
> intended recipient, please notify the sender immediately and do not
> disclose the contents to any other person, use it for any purpose, or
> store or copy the information in any medium. Thank you.
> >
>
>
More information about the llvm-commits
mailing list