[arm] branches to weak symbols

[arm] branches to weak symbols

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

This patch fixes some arm cases where the assembler did not consider that a 
weakly defined symbol might be overridden in the final link.  It would use a 
short thumb branch resulting in an unoverridable relocation (and inability to 
build the linux kernel)

The same error is made in write.c's fixup_segment (and affects this arm problem) 
  It may affect other targets.

Fixed thusly, tested on arm-eabi. ok?

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery


[-- Attachment #2: all.diff --]
[-- Type: text/plain, Size: 8311 bytes --]

2010-04-28  Nathan Sidwell  <nathan@codesourcery.com>

	* write.c (fixup_segment): Do not assume we know the section a
	defined weak symbol is in.
	* config/tc-arm.c (relax_adr, relax_branch, md_apply_fix): Treat
	weak symbols as not known to be in the same section, even if they
	are defined.

	testsuite/
	* gas/arm/weakdef-1.s: New.
	* gas/arm/weakdef-1.d: New.
	* gas/arm/weakdef-2.s: New.
	* gas/arm/weakdef-2.d: New.
	* gas/arm/weakdef-2.l: New.

Index: write.c
===================================================================
RCS file: /cvs/src/src/gas/write.c,v
retrieving revision 1.129
diff -c -3 -p -r1.129 write.c
*** write.c	23 Jan 2010 12:05:32 -0000	1.129
--- write.c	28 Apr 2010 08:40:28 -0000
*************** fixup_segment (fixS *fixP, segT this_seg
*** 992,998 ****
  
        if (fixP->fx_addsy)
  	{
! 	  if (add_symbol_segment == this_segment
  	      && !TC_FORCE_RELOCATION_LOCAL (fixP))
  	    {
  	      /* This fixup was made when the symbol's segment was
--- 992,1000 ----
  
        if (fixP->fx_addsy)
  	{
! 	  if (S_IS_WEAK (fixP->fx_addsy))
! 	    ; // even if it is defined, it might be overridden later
! 	  else if (add_symbol_segment == this_segment
  	      && !TC_FORCE_RELOCATION_LOCAL (fixP))
  	    {
  	      /* This fixup was made when the symbol's segment was
Index: config/tc-arm.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-arm.c,v
retrieving revision 1.441
diff -c -3 -p -r1.441 tc-arm.c
*** config/tc-arm.c	15 Apr 2010 10:56:37 -0000	1.441
--- config/tc-arm.c	28 Apr 2010 08:40:34 -0000
*************** relax_adr (fragS *fragp, asection *sec, 
*** 18530,18536 ****
    /* Assume worst case for symbols not known to be in the same section.  */
    if (fragp->fr_symbol == NULL
        || !S_IS_DEFINED (fragp->fr_symbol)
!       || sec != S_GET_SEGMENT (fragp->fr_symbol))
      return 4;
  
    val = relaxed_symbol_addr (fragp, stretch);
--- 18530,18537 ----
    /* Assume worst case for symbols not known to be in the same section.  */
    if (fragp->fr_symbol == NULL
        || !S_IS_DEFINED (fragp->fr_symbol)
!       || sec != S_GET_SEGMENT (fragp->fr_symbol)
!       || S_IS_WEAK (fragp->fr_symbol))
      return 4;
  
    val = relaxed_symbol_addr (fragp, stretch);
*************** relax_branch (fragS *fragp, asection *se
*** 18573,18579 ****
  
    /* Assume worst case for symbols not known to be in the same section.  */
    if (!S_IS_DEFINED (fragp->fr_symbol)
!       || sec != S_GET_SEGMENT (fragp->fr_symbol))
      return 4;
  
  #ifdef OBJ_ELF
--- 18574,18581 ----
  
    /* Assume worst case for symbols not known to be in the same section.  */
    if (!S_IS_DEFINED (fragp->fr_symbol)
!       || sec != S_GET_SEGMENT (fragp->fr_symbol)
!       || S_IS_WEAK (fragp->fr_symbol))
      return 4;
  
  #ifdef OBJ_ELF
*************** md_apply_fix (fixS *	fixP,
*** 19784,19805 ****
  	 not have a reloc for it, so tc_gen_reloc will reject it.  */
        fixP->fx_done = 1;
  
!       if (fixP->fx_addsy
! 	  && ! S_IS_DEFINED (fixP->fx_addsy))
  	{
! 	  as_bad_where (fixP->fx_file, fixP->fx_line,
! 			_("undefined symbol %s used as an immediate value"),
! 			S_GET_NAME (fixP->fx_addsy));
! 	  break;
! 	}
  
!       if (fixP->fx_addsy
! 	  && S_GET_SEGMENT (fixP->fx_addsy) != seg)
! 	{
! 	  as_bad_where (fixP->fx_file, fixP->fx_line,
! 			_("symbol %s is in a different section"),
! 			S_GET_NAME (fixP->fx_addsy));
! 	  break;
  	}
  
        newimm = encode_arm_immediate (value);
--- 19786,19808 ----
  	 not have a reloc for it, so tc_gen_reloc will reject it.  */
        fixP->fx_done = 1;
  
!       if (fixP->fx_addsy)
  	{
! 	  const char *msg = 0;
  
! 	  if (! S_IS_DEFINED (fixP->fx_addsy))
! 	    msg = _("undefined symbol %s used as an immediate value");
! 	  else if (S_GET_SEGMENT (fixP->fx_addsy) != seg)
! 	    msg = _("symbol %s is in a different section");
! 	  else if (S_IS_WEAK (fixP->fx_addsy))
! 	    msg = _("symbol %s is weak and may be overridden later");
! 
! 	  if (msg)
! 	    {
! 	      as_bad_where (fixP->fx_file, fixP->fx_line,
! 			    msg, S_GET_NAME (fixP->fx_addsy));
! 	      break;
! 	    }
  	}
  
        newimm = encode_arm_immediate (value);
*************** md_apply_fix (fixS *	fixP,
*** 19825,19848 ****
  	unsigned int highpart = 0;
  	unsigned int newinsn  = 0xe1a00000; /* nop.  */
  
! 	if (fixP->fx_addsy
! 	    && ! S_IS_DEFINED (fixP->fx_addsy))
  	  {
! 	    as_bad_where (fixP->fx_file, fixP->fx_line,
! 			  _("undefined symbol %s used as an immediate value"),
! 			  S_GET_NAME (fixP->fx_addsy));
! 	    break;
! 	  }
  
! 	if (fixP->fx_addsy
! 	    && S_GET_SEGMENT (fixP->fx_addsy) != seg)
! 	  {
! 	    as_bad_where (fixP->fx_file, fixP->fx_line,
! 			  _("symbol %s is in a different section"),
! 			  S_GET_NAME (fixP->fx_addsy));
! 	    break;
! 	  }
  
  	newimm = encode_arm_immediate (value);
  	temp = md_chars_to_number (buf, INSN_SIZE);
  
--- 19828,19852 ----
  	unsigned int highpart = 0;
  	unsigned int newinsn  = 0xe1a00000; /* nop.  */
  
! 	if (fixP->fx_addsy)
  	  {
! 	    const char *msg = 0;
  
! 	    if (! S_IS_DEFINED (fixP->fx_addsy))
! 	      msg = _("undefined symbol %s used as an immediate value");
! 	    else if (S_GET_SEGMENT (fixP->fx_addsy) != seg)
! 	      msg = _("symbol %s is in a different section");
! 	    else if (S_IS_WEAK (fixP->fx_addsy))
! 	      msg = _("symbol %s is weak and may be overridden later");
  
+ 	    if (msg)
+ 	      {
+ 		as_bad_where (fixP->fx_file, fixP->fx_line,
+ 			      msg, S_GET_NAME (fixP->fx_addsy));
+ 		break;
+ 	      }
+ 	  }
+ 	
  	newimm = encode_arm_immediate (value);
  	temp = md_chars_to_number (buf, INSN_SIZE);
  
Index: testsuite/gas/arm/weakdef-1.d
===================================================================
RCS file: testsuite/gas/arm/weakdef-1.d
diff -N testsuite/gas/arm/weakdef-1.d
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gas/arm/weakdef-1.d	28 Apr 2010 08:40:34 -0000
***************
*** 0 ****
--- 1,18 ----
+ # name: Thumb branch to weak
+ # as:
+ # objdump: -dr
+ 
+ .*: +file format .*arm.*
+ 
+ 
+ Disassembly of section .text:
+ 
+ 0+000 <Weak>:
+    0:	e7fe      	b.n	2 <Strong>
+ 			0: R_ARM_THM_JUMP11	Strong
+ 
+ 0+002 <Strong>:
+    2:	f7ff bffe 	b.w	0 <Random>
+ 			2: R_ARM_THM_JUMP24	Random
+    6:	f7ff bffe 	b.w	0 <Weak>
+ 			6: R_ARM_THM_JUMP24	Weak
Index: testsuite/gas/arm/weakdef-1.s
===================================================================
RCS file: testsuite/gas/arm/weakdef-1.s
diff -N testsuite/gas/arm/weakdef-1.s
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gas/arm/weakdef-1.s	28 Apr 2010 08:40:34 -0000
***************
*** 0 ****
--- 1,18 ----
+ 	.syntax unified
+ 	.text
+ 	.thumb
+ 
+ 	.globl	Weak
+ 	.weak	Weak
+ 	.thumb_func
+ 	.type	Weak, %function
+ Weak:
+ 	b	Strong
+ 	.size	Weak, .-Weak
+ 	
+ 	.globl	Strong
+ 	.type	Strong, %function
+ Strong:
+ 	b	Random
+ 	b	Weak
+ 	.size	Strong, .-Strong
Index: testsuite/gas/arm/weakdef-2.d
===================================================================
RCS file: testsuite/gas/arm/weakdef-2.d
diff -N testsuite/gas/arm/weakdef-2.d
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gas/arm/weakdef-2.d	28 Apr 2010 08:40:34 -0000
***************
*** 0 ****
--- 1,3 ----
+ # name: adr of weak
+ # as:
+ # error-output: weakdef-2.l
Index: testsuite/gas/arm/weakdef-2.l
===================================================================
RCS file: testsuite/gas/arm/weakdef-2.l
diff -N testsuite/gas/arm/weakdef-2.l
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gas/arm/weakdef-2.l	28 Apr 2010 08:40:34 -0000
***************
*** 0 ****
--- 1,3 ----
+ [^:]*: Assembler messages:
+ [^:]*:9: Error: symbol Weak is weak and may be overridden later
+ [^:]*:10: Error: symbol Weak is weak and may be overridden later
Index: testsuite/gas/arm/weakdef-2.s
===================================================================
RCS file: testsuite/gas/arm/weakdef-2.s
diff -N testsuite/gas/arm/weakdef-2.s
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gas/arm/weakdef-2.s	28 Apr 2010 08:40:34 -0000
***************
*** 0 ****
--- 1,10 ----
+ 	.syntax unified
+ 	.text
+ 	.globl	Strong
+ Strong:	
+ 	adrl	r0,Strong
+ 	adr	r0,Strong
+ 	.globl	Weak
+ 	.weak	Weak
+ Weak:	adrl	r0,Weak
+ 	adr	r0,Weak

Re: [arm] branches to weak symbols

[Alan Modra]

On Wed, Apr 28, 2010 at 09:49:17AM +0100, Nathan Sidwell wrote:
> *** write.c	23 Jan 2010 12:05:32 -0000	1.129
> --- write.c	28 Apr 2010 08:40:28 -0000
> *************** fixup_segment (fixS *fixP, segT this_seg
> *** 992,998 ****
>   
>         if (fixP->fx_addsy)
>   	{
> ! 	  if (add_symbol_segment == this_segment
>   	      && !TC_FORCE_RELOCATION_LOCAL (fixP))
>   	    {
>   	      /* This fixup was made when the symbol's segment was
> --- 992,1000 ----
>   
>         if (fixP->fx_addsy)
>   	{
> ! 	  if (S_IS_WEAK (fixP->fx_addsy))
> ! 	    ; // even if it is defined, it might be overridden later
> ! 	  else if (add_symbol_segment == this_segment
>   	      && !TC_FORCE_RELOCATION_LOCAL (fixP))
>   	    {
>   	      /* This fixup was made when the symbol's segment was

I don't believe this is correct.  The default TC_FORCE_RELOCATION*
macros should avoid applying relocations against weak symbols.  See
generic_force_reloc and S_FORCE_RELOC.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [arm] branches to weak symbols

[Nathan Sidwell]

Alan Modra wrote:

> I don't believe this is correct.  The default TC_FORCE_RELOCATION*
> macros should avoid applying relocations against weak symbols.  See
> generic_force_reloc and S_FORCE_RELOC.

ok, unfortunately arm_force_relocation is doing something funky with weak 
symbols in some cases, so the obvious changes don't DTRT.  We'll investigate 
further.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery

Re: [arm] branches to weak symbols

[Vincent Rivière]

Nathan Sidwell wrote:
> This patch fixes some arm cases where the assembler did not consider 
> that a weakly defined symbol might be overridden in the final link.

Just in case it could help, your bug may have some similarities with 
this one, which had been fixed for m68k targets using a.out.
http://sourceware.org/bugzilla/show_bug.cgi?id=3041

-- 
Vincent Rivière

Re: [arm] branches to weak symbols

[Nick Clifton]

Hi Nathan,

> This patch fixes some arm cases where the assembler did not consider
> that a weakly defined symbol might be overridden in the final link.

> 2010-04-28  Nathan Sidwell  <nathan@codesourcery.com>
>
> 	* write.c (fixup_segment): Do not assume we know the section a
> 	defined weak symbol is in.
> 	* config/tc-arm.c (relax_adr, relax_branch, md_apply_fix): Treat
> 	weak symbols as not known to be in the same section, even if they
> 	are defined.
>
> 	testsuite/
> 	* gas/arm/weakdef-1.s: New.
> 	* gas/arm/weakdef-1.d: New.
> 	* gas/arm/weakdef-2.s: New.
> 	* gas/arm/weakdef-2.d: New.
> 	* gas/arm/weakdef-2.l: New.

Approved and applied.  Note - I made one small change which was to 
disable the new tests for non-ELF targeted ARM toolchains.

Cheers
   Nick

Re: [arm] branches to weak symbols

[Daniel Jacobowitz]

On Thu, Apr 29, 2010 at 03:45:24PM +0100, Nick Clifton wrote:
> Hi Nathan,
> 
> >This patch fixes some arm cases where the assembler did not consider
> >that a weakly defined symbol might be overridden in the final link.
> 
> >2010-04-28  Nathan Sidwell  <nathan@codesourcery.com>
> >
> >	* write.c (fixup_segment): Do not assume we know the section a
> >	defined weak symbol is in.
> >	* config/tc-arm.c (relax_adr, relax_branch, md_apply_fix): Treat
> >	weak symbols as not known to be in the same section, even if they
> >	are defined.
> >
> >	testsuite/
> >	* gas/arm/weakdef-1.s: New.
> >	* gas/arm/weakdef-1.d: New.
> >	* gas/arm/weakdef-2.s: New.
> >	* gas/arm/weakdef-2.d: New.
> >	* gas/arm/weakdef-2.l: New.
> 
> Approved and applied.  Note - I made one small change which was to
> disable the new tests for non-ELF targeted ARM toolchains.

I have to agree with Alan that the write.c change is not correct
(specifically, I think it will not catch the other cases checked for
in generic_force_reloc, like IFUNC).  The current behavior is
deliberate:

  /* Resolve these relocations even if the symbol is extern or weak.  */
  if (fixp->fx_r_type == BFD_RELOC_ARM_IMMEDIATE
      || fixp->fx_r_type == BFD_RELOC_ARM_OFFSET_IMM
      || fixp->fx_r_type == BFD_RELOC_ARM_ADRL_IMMEDIATE
      || fixp->fx_r_type == BFD_RELOC_ARM_T32_ADD_IMM
      || fixp->fx_r_type == BFD_RELOC_ARM_T32_IMMEDIATE
      || fixp->fx_r_type == BFD_RELOC_ARM_T32_IMM12
      || fixp->fx_r_type == BFD_RELOC_ARM_T32_ADD_PC12)
    return 0;

I believe it's that last relocation type in this case?

This block was originally added for those first three relocations.  I
don't know why; it was part of the big Thumb-2 commit.  Paul extended
the list when implementing some relaxation.

Paul, do you know what this change was for?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [arm] branches to weak symbols

[Nick Clifton]

Hi Nathan,

> Alan Modra wrote:
> ok, unfortunately arm_force_relocation is doing something funky with
> weak symbols in some cases, so the obvious changes don't DTRT. We'll
> investigate further.

I have reverted the write.c part of your original patch and moved the 
change into tc-arm.h for now.  I hope that this will be enough, but if 
not then please feel free to fix it yourself or else ping me.

Cheers
   Nick