public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* linker crash in arm stub generation
@ 2009-06-11 17:09 Phil Blundell
  2009-06-12 12:36 ` Nick Clifton
  0 siblings, 1 reply; 22+ messages in thread
From: Phil Blundell @ 2009-06-11 17:09 UTC (permalink / raw)
  To: binutils

This testcase causes the linker (CVS head from a couple of days ago,
arm-linuxgnueabi target) to segfault in arm_build_one_stub():

$ cat test1.s
	.cpu arm10tdmi
	.fpu softvfp
	.eabi_attribute 20, 1
	.eabi_attribute 21, 1
	.eabi_attribute 23, 3
	.eabi_attribute 24, 1
	.eabi_attribute 25, 1
	.eabi_attribute 26, 2
	.eabi_attribute 30, 2
	.eabi_attribute 18, 4
	.code	16
	.file	"t.c"
	.text
	.align	2
	.global	foo
	.code	16
	.thumb_func
	.type	foo, %function
foo:
	@ sp needed for prologue
	bx	lr
	.size	foo, .-foo
	.ident	"GCC: (GNU) 4.4.0"
	.section	.note.GNU-stack,"",%progbits
$ cat test2.s
        bl      foo(PLT)
$ ../gas/as-new -o test1.o test1.s
$ ../gas/as-new -o test2.o test2.s
$ ./ld-new -shared -o test1.so test1.o
$ ./ld-new -shared test2.o test1.so
Segmentation fault
$

p.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-11 17:09 linker crash in arm stub generation Phil Blundell
@ 2009-06-12 12:36 ` Nick Clifton
  2009-06-12 13:19   ` Christophe LYON
  0 siblings, 1 reply; 22+ messages in thread
From: Nick Clifton @ 2009-06-12 12:36 UTC (permalink / raw)
  To: Phil Blundell; +Cc: binutils

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

Hi Phil,

> This testcase causes the linker (CVS head from a couple of days ago,
> arm-linuxgnueabi target) to segfault in arm_build_one_stub():

Please could you open a bug report for this bug ?

Once you have done that, please try out this patch which I think should 
solve the problem.

Cheers
   Nick

[-- Attachment #2: elf32-arm.c.patch --]
[-- Type: text/plain, Size: 843 bytes --]

Index: bfd/elf32-arm.c
===================================================================
RCS file: /cvs/src/src/bfd/elf32-arm.c,v
retrieving revision 1.196
diff -c -3 -p -r1.196 elf32-arm.c
*** bfd/elf32-arm.c	22 May 2009 11:58:44 -0000	1.196
--- bfd/elf32-arm.c	12 Jun 2009 12:34:47 -0000
*************** arm_build_one_stub (struct bfd_hash_entr
*** 3450,3455 ****
--- 3450,3460 ----
    stub_addr = stub_sec->output_section->vma + stub_sec->output_offset
      + stub_entry->stub_offset;
  
+   if (stub_entry->target_section == NULL
+       || stub_entry->target_section->output_section == NULL)
+     /* The output section has been deleted, ignore this stub.  */
+     return TRUE;
+ 
    /* This is the address of the stub destination.  */
    sym_value = (stub_entry->target_value
  	       + stub_entry->target_section->output_offset

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-12 12:36 ` Nick Clifton
@ 2009-06-12 13:19   ` Christophe LYON
  2009-06-12 13:49     ` Nick Clifton
  2009-06-12 14:06     ` Daniel Jacobowitz
  0 siblings, 2 replies; 22+ messages in thread
From: Christophe LYON @ 2009-06-12 13:19 UTC (permalink / raw)
  To: Nick Clifton, Phil Blundell; +Cc: binutils

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

Hi Phil and Nick,

On 12.06.2009 14:35, Nick Clifton wrote:
> Hi Phil,
> 
>> This testcase causes the linker (CVS head from a couple of days ago,
>> arm-linuxgnueabi target) to segfault in arm_build_one_stub():
> 
> Please could you open a bug report for this bug ?
> 
> Once you have done that, please try out this patch which I think should 
> solve the problem.
> 


I think your patch is not adequate: it will prevent the stub code from 
being generated, but the user code will still jump to the stub. So the 
link won't fail, but the result will be wrong.

Instead, I propose this patch, which prevents mode-switching stub 
generation when the call goes through a PLT entry.

I think there should already be a test for this in the testsuite, I have 
probably forgotten this configuration.

Christophe.

[-- Attachment #2: elf32-arm.c.patch --]
[-- Type: text/plain, Size: 1079 bytes --]

Index: bfd/elf32-arm.c
===================================================================
RCS file: /cvs/src/src/bfd/elf32-arm.c,v
retrieving revision 1.196
diff -p -c -u -r1.196 elf32-arm.c
--- bfd/elf32-arm.c	22 May 2009 11:58:44 -0000	1.196
+++ bfd/elf32-arm.c	12 Jun 2009 13:18:20 -0000
@@ -3175,11 +3175,15 @@ arm_type_of_stub (struct bfd_link_info *
 
 	  /* We have an extra 2-bytes reach because of
 	     the mode change (bit 24 (H) of BLX encoding).  */
+	  /* A stub is needed only if this call is not throught a PLT
+	     entry, because PLT stubs handle mode switching
+	     already.  */
 	  if (branch_offset > (ARM_MAX_FWD_BRANCH_OFFSET + 2)
 	      || (branch_offset < ARM_MAX_BWD_BRANCH_OFFSET)
-	      || ((r_type == R_ARM_CALL) && !globals->use_blx)
-	      || (r_type == R_ARM_JUMP24)
-	      || (r_type == R_ARM_PLT32))
+	      || ( (((r_type == R_ARM_CALL) && !globals->use_blx)
+		    || (r_type == R_ARM_JUMP24)
+		    || (r_type == R_ARM_PLT32))
+		   && !use_plt))
 	    {
 	      stub_type = (info->shared | globals->pic_veneer)
 		/* PIC stubs.  */

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-12 13:19   ` Christophe LYON
@ 2009-06-12 13:49     ` Nick Clifton
  2009-06-12 14:06     ` Daniel Jacobowitz
  1 sibling, 0 replies; 22+ messages in thread
From: Nick Clifton @ 2009-06-12 13:49 UTC (permalink / raw)
  To: Christophe LYON; +Cc: Phil Blundell, binutils

Hi Christophe,

> I think your patch is not adequate: it will prevent the stub code from 
> being generated, but the user code will still jump to the stub. So the 
> link won't fail, but the result will be wrong.

Ah, good point.


> Instead, I propose this patch, which prevents mode-switching stub 
> generation when the call goes through a PLT entry.

Thanks!

Cheers
   Nick


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-12 13:19   ` Christophe LYON
  2009-06-12 13:49     ` Nick Clifton
@ 2009-06-12 14:06     ` Daniel Jacobowitz
  2009-06-12 14:13       ` Christophe LYON
  1 sibling, 1 reply; 22+ messages in thread
From: Daniel Jacobowitz @ 2009-06-12 14:06 UTC (permalink / raw)
  To: Christophe LYON; +Cc: Nick Clifton, Phil Blundell, binutils

On Fri, Jun 12, 2009 at 03:19:21PM +0200, Christophe LYON wrote:
> I think your patch is not adequate: it will prevent the stub code from  
> being generated, but the user code will still jump to the stub. So the  
> link won't fail, but the result will be wrong.
>
> Instead, I propose this patch, which prevents mode-switching stub  
> generation when the call goes through a PLT entry.
>
> I think there should already be a test for this in the testsuite, I have  
> probably forgotten this configuration.

This looks good to me - and I agree that a test would be helpful!

-- 
Daniel Jacobowitz
CodeSourcery

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-12 14:06     ` Daniel Jacobowitz
@ 2009-06-12 14:13       ` Christophe LYON
  2009-06-15 14:22         ` Christophe LYON
  0 siblings, 1 reply; 22+ messages in thread
From: Christophe LYON @ 2009-06-12 14:13 UTC (permalink / raw)
  To: binutils; +Cc: Nick Clifton, Phil Blundell

On 12.06.2009 16:06, Daniel Jacobowitz wrote:
> On Fri, Jun 12, 2009 at 03:19:21PM +0200, Christophe LYON wrote:
>> I think your patch is not adequate: it will prevent the stub code from  
>> being generated, but the user code will still jump to the stub. So the  
>> link won't fail, but the result will be wrong.
>>
>> Instead, I propose this patch, which prevents mode-switching stub  
>> generation when the call goes through a PLT entry.
>>
>> I think there should already be a test for this in the testsuite, I have  
>> probably forgotten this configuration.
> 
> This looks good to me - and I agree that a test would be helpful!
> 
I'll propose one + proper ChangeLog entry next week.

Christophe.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-12 14:13       ` Christophe LYON
@ 2009-06-15 14:22         ` Christophe LYON
  2009-06-15 17:59           ` Daniel Jacobowitz
  0 siblings, 1 reply; 22+ messages in thread
From: Christophe LYON @ 2009-06-15 14:22 UTC (permalink / raw)
  To: binutils; +Cc: Nick Clifton, Phil Blundell, Daniel Jacobowitz

Hi,

>> This looks good to me - and I agree that a test would be helpful!
>>
> I'll propose one + proper ChangeLog entry next week.
> 

I have started to look at this problem more closely, and I have one 
question: in elf32-arm.c:allocate_dynrelocs(), there is this comment:

   /* If this symbol is not defined in a regular file, and we are
      not generating a shared library, then set the symbol to this
      location in the .plt.  This is required to make function
      pointers compare as equal between the normal executable and
      the shared library.  */

Why is the behaviour different when generating a shared lib?

I thought I had understood the comment about function pointers 
comparison, but I am wondering now....

Christophe.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-15 14:22         ` Christophe LYON
@ 2009-06-15 17:59           ` Daniel Jacobowitz
  2009-06-17 15:44             ` Christophe LYON
  0 siblings, 1 reply; 22+ messages in thread
From: Daniel Jacobowitz @ 2009-06-15 17:59 UTC (permalink / raw)
  To: Christophe LYON; +Cc: binutils, Nick Clifton, Phil Blundell

On Mon, Jun 15, 2009 at 04:20:52PM +0200, Christophe LYON wrote:
> Hi,
>
>>> This looks good to me - and I agree that a test would be helpful!
>>>
>> I'll propose one + proper ChangeLog entry next week.
>>
>
> I have started to look at this problem more closely, and I have one  
> question: in elf32-arm.c:allocate_dynrelocs(), there is this comment:
>
>   /* If this symbol is not defined in a regular file, and we are
>      not generating a shared library, then set the symbol to this
>      location in the .plt.  This is required to make function
>      pointers compare as equal between the normal executable and
>      the shared library.  */
>
> Why is the behaviour different when generating a shared lib?
>
> I thought I had understood the comment about function pointers  
> comparison, but I am wondering now....

A PLT entry with a non-zero address is used as the canonical location
of the function.  This is only ever required in an executable, never
in a shared library.  If all accesses to the address are PIC - which
they must be, in a shared library - then they can be easily adjusted
to point to the function's address.  And it's better to do that,
because calls through those pointers will go directly to the function
instead of to the PLT.

In an executable, this might not be the case.  For instance you might
have the address of the funtion in a constant pool in the text
segment.  If that happens, the linker must fix the address of the
function at static link time, even if the definition turns out to be
in a shared library.

Such code is (or is supposed to be, anyway) rejected in shared
objects.

-- 
Daniel Jacobowitz
CodeSourcery

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-15 17:59           ` Daniel Jacobowitz
@ 2009-06-17 15:44             ` Christophe LYON
  2009-06-17 16:09               ` Phil Blundell
  2009-06-17 18:10               ` Daniel Jacobowitz
  0 siblings, 2 replies; 22+ messages in thread
From: Christophe LYON @ 2009-06-17 15:44 UTC (permalink / raw)
  To: Christophe LYON, binutils, Nick Clifton, Phil Blundell

Hi Daniel,

Thank you for your answer, but I am afraid things are still not clear to 
me, despite reading your answer several times :-(

(part of the confusion probably comes from the fact that I deal with 
different architectures, too...)

>> I have started to look at this problem more closely, and I have one  
>> question: in elf32-arm.c:allocate_dynrelocs(), there is this comment:
>>
>>   /* If this symbol is not defined in a regular file, and we are
>>      not generating a shared library, then set the symbol to this
>>      location in the .plt.  This is required to make function
>>      pointers compare as equal between the normal executable and
>>      the shared library.  */
>>
>> Why is the behaviour different when generating a shared lib?
>>
>> I thought I had understood the comment about function pointers  
>> comparison, but I am wondering now....
> 
> A PLT entry with a non-zero address is used as the canonical location
> of the function.
This "canonical" location is only used by the dynamic linker, when it 
patches the dyn relocs pointing to this symbol, right? (when the address 
of the function is stored in a constant pool for instance, or in GOT)

 > This is only ever required in an executable, never
> in a shared library.  If all accesses to the address are PIC - which
> they must be, in a shared library - then they can be easily adjusted
> to point to the function's address.
Easy because the dyn linker needs to patch the GOT only (ie one entry 
instead of several references)?

 > And it's better to do that,
> because calls through those pointers will go directly to the function
> instead of to the PLT.
So you mean that in a shared lib, PLT are generated, but not executed 
because the dyn linker manages to make these indirect calls go directly 
to the function?

But then, how is symbol preemption handled? I mean, if a shared lib is 
actually shared, ie used by two different executables, and one of them 
preempts the function definition, but not the other, I think the calls 
need to go through the PLT so that different GOTs are used to reach 
different functions.

> In an executable, this might not be the case.  For instance you might
> have the address of the funtion in a constant pool in the text
> segment.  If that happens, the linker must fix the address of the
> function at static link time, even if the definition turns out to be
> in a shared library.
Why couldn't this be performed at load time?

> Such code is (or is supposed to be, anyway) rejected in shared
> objects.
> 

I thought your answer would help me solve my actual problem, but since 
it seems that I need better understanding, I will expose my actual 
question here:
If I am generating a shared lib, let's say that some ARM code references 
a THUMB function in a shared lib.
As the target is in a shared lib, we need to have a PLT generated.
But, we also need to know if we need to change modes, and if we need a 
long branch stub.

However, because of the comment I mentioned earlier, the destination is 
not recorded as being the PLT, so we don't know the actual distance, and 
the symbol type is not switched to ARM type.

This scenario is handled properly when generating an executable, but 
when generating a shared lib, the current generation is broken.

Christophe.

PS: I have noted that Phil Blundell has inadvertently committed a wrong 
patch for the issue being discussed (as part of another commit of his). 
I don't know if it should be cancelled separately or if it can't wait 
until I propose a full patch + testcase.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-17 15:44             ` Christophe LYON
@ 2009-06-17 16:09               ` Phil Blundell
  2009-06-17 18:10               ` Daniel Jacobowitz
  1 sibling, 0 replies; 22+ messages in thread
From: Phil Blundell @ 2009-06-17 16:09 UTC (permalink / raw)
  To: Christophe LYON; +Cc: binutils, Nick Clifton

On Wed, 2009-06-17 at 17:34 +0200, Christophe LYON wrote:
> PS: I have noted that Phil Blundell has inadvertently committed a wrong 
> patch for the issue being discussed (as part of another commit of his). 
> I don't know if it should be cancelled separately or if it can't wait 
> until I propose a full patch + testcase.

Ah, whoops.  Sorry about that, I'll undo the mistaken checkin.

p.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-17 15:44             ` Christophe LYON
  2009-06-17 16:09               ` Phil Blundell
@ 2009-06-17 18:10               ` Daniel Jacobowitz
  2009-06-18 14:25                 ` Christophe LYON
  1 sibling, 1 reply; 22+ messages in thread
From: Daniel Jacobowitz @ 2009-06-17 18:10 UTC (permalink / raw)
  To: Christophe LYON; +Cc: binutils, Nick Clifton, Phil Blundell

On Wed, Jun 17, 2009 at 05:34:13PM +0200, Christophe LYON wrote:
>> A PLT entry with a non-zero address is used as the canonical location
>> of the function.
> This "canonical" location is only used by the dynamic linker, when it  
> patches the dyn relocs pointing to this symbol, right? (when the address  
> of the function is stored in a constant pool for instance, or in GOT)

That's right.  Except that a constant pool should never have dynamic
relocations; it's read-only.  But for instance an initialized writable
data structure could have such a relocation.

> > This is only ever required in an executable, never
>> in a shared library.  If all accesses to the address are PIC - which
>> they must be, in a shared library - then they can be easily adjusted
>> to point to the function's address.
> Easy because the dyn linker needs to patch the GOT only (ie one entry  
> instead of several references)?

It doesn't matter.  Easy because there are appropriate dynamic
relocations.  There aren't dynamic relocations for, for instance,
movw / movt.

> > And it's better to do that,
>> because calls through those pointers will go directly to the function
>> instead of to the PLT.
> So you mean that in a shared lib, PLT are generated, but not executed  
> because the dyn linker manages to make these indirect calls go directly  
> to the function?

No, PLTs are generated and used - but only for lazy resolution.  If
you have a global variable "void (*funp) (void) = function" you can't
lazily resolve "function" because the value of funp is observable by
the program.  The address of a called function is not observable.

> But then, how is symbol preemption handled? I mean, if a shared lib is  
> actually shared, ie used by two different executables, and one of them  
> preempts the function definition, but not the other, I think the calls  
> need to go through the PLT so that different GOTs are used to reach  
> different functions.

This is an SVR4 dynamic linking environment we're talking about.
There's only ever one executable in the address space.  In a flat,
non-virtual address space, like uClinux, you don't do PIC this same
way.  There needs to be a separate copy of the entire writable
data segment, not just the GOT.

>> In an executable, this might not be the case.  For instance you might
>> have the address of the funtion in a constant pool in the text
>> segment.  If that happens, the linker must fix the address of the
>> function at static link time, even if the definition turns out to be
>> in a shared library.
> Why couldn't this be performed at load time?

Several reasons.  The appropriate dynamic relocations are not
defined.  It's slow.  And it requires making the read-only text
segment writable to patch it, which is bad for both performance and
security.

> I thought your answer would help me solve my actual problem, but since it 
> seems that I need better understanding, I will expose my actual question 
> here:
> If I am generating a shared lib, let's say that some ARM code references  
> a THUMB function in a shared lib.
> As the target is in a shared lib, we need to have a PLT generated.
> But, we also need to know if we need to change modes, and if we need a  
> long branch stub.
>
> However, because of the comment I mentioned earlier, the destination is  
> not recorded as being the PLT, so we don't know the actual distance, and  
> the symbol type is not switched to ARM type.

A branch to a preemptible symbol is always supposed to go to the PLT
entry, and we know the mode of the PLT entry - or sometimes it's
available in both modes depending on the mode of the call site ("bx
pc" Thumb stub in front of the PLT).  So you get to pick which PLT
entry you're going to reach from the long branch stub.  I'd imagine
it's generally profitable to use the ARM mode PLT entry point.

-- 
Daniel Jacobowitz
CodeSourcery

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-17 18:10               ` Daniel Jacobowitz
@ 2009-06-18 14:25                 ` Christophe LYON
  2009-06-18 14:36                   ` Christophe LYON
  0 siblings, 1 reply; 22+ messages in thread
From: Christophe LYON @ 2009-06-18 14:25 UTC (permalink / raw)
  To: binutils, Nick Clifton, Phil Blundell

Hi,

After a bit more work, I am able to propose a new patch for this issue.
I have enriched the tests to be sure to check calls inside shared libs 
to defined/undefined and ARM/Thumb symbols.

Checked with arm-none-eabi and arm-linux-gnueabi with no regression.

Christophe.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-18 14:25                 ` Christophe LYON
@ 2009-06-18 14:36                   ` Christophe LYON
  2009-06-22  9:24                     ` Nick Clifton
  2009-08-26  1:21                     ` Fix Thumb-2 shared libraries Daniel Jacobowitz
  0 siblings, 2 replies; 22+ messages in thread
From: Christophe LYON @ 2009-06-18 14:36 UTC (permalink / raw)
  To: binutils, Nick Clifton, Phil Blundell

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

Oops! I forgot to attach the patch, sorry.


On 18.06.2009 16:23, Christophe LYON wrote:
> Hi,
> 
> After a bit more work, I am able to propose a new patch for this issue.
> I have enriched the tests to be sure to check calls inside shared libs 
> to defined/undefined and ARM/Thumb symbols.
> 
> Checked with arm-none-eabi and arm-linux-gnueabi with no regression.
> 
> Christophe.
> 
> 
> 


[-- Attachment #2: pltshared.changelog --]
[-- Type: text/plain, Size: 623 bytes --]

2009-06-18  Christophe Lyon  <christophe.lyon@st.com>

	bfd/
	* elf32-arm.c (elf32_arm_size_stubs): Use PLT address as
	destination for defined dynamic symbols when deciding whether to
	insert a stub or not.
	(allocate_dynrelocs): Make sure functions are not marked as Thumb
	when actually accessed through a PLT, even when generating a
	shared lib.

	ld/testsuite:
	* ld-arm/farcall-mixed-app.s: Add new references to check more
	modes switching.
	* ld-arm/farcall-mixed-lib1.s: Likewise.
	* ld-arm/farcall-mixed-app-v5.d: Update expected result.
	* farcall-mixed-app.d: Likewise.
	* ld-arm/farcall-mixed-lib.d: Likewise.

[-- Attachment #3: pltshared.patch --]
[-- Type: text/plain, Size: 10779 bytes --]

Index: bfd/elf32-arm.c
===================================================================
RCS file: /cvs/src/src/bfd/elf32-arm.c,v
retrieving revision 1.200
diff -u -p -r1.200 elf32-arm.c
--- bfd/elf32-arm.c	17 Jun 2009 18:08:34 -0000	1.200
+++ bfd/elf32-arm.c	18 Jun 2009 14:12:33 -0000
@@ -4361,7 +4361,25 @@ elf32_arm_size_stubs (bfd *output_bfd,
 			{
 			  sym_sec = hash->root.root.u.def.section;
 			  sym_value = hash->root.root.u.def.value;
-			  if (sym_sec->output_section != NULL)
+
+			  struct elf32_arm_link_hash_table *globals =
+						  elf32_arm_hash_table (info);
+
+			  /* For a destination in a shared library,
+			     use the PLT stub as target address to
+			     decide whether a branch stub is
+			     needed.  */
+			  if (globals->splt != NULL && hash != NULL
+			      && hash->root.plt.offset != (bfd_vma) -1)
+			    {
+			      sym_sec = globals->splt;
+			      sym_value = hash->root.plt.offset;
+			      if (sym_sec->output_section != NULL)
+				destination = (sym_value
+					       + sym_sec->output_offset
+					       + sym_sec->output_section->vma);
+			    }
+			  else if (sym_sec->output_section != NULL)
 			    destination = (sym_value + irela->r_addend
 					   + sym_sec->output_offset
 					   + sym_sec->output_section->vma);
@@ -11273,14 +11291,14 @@ allocate_dynrelocs (struct elf_link_hash
 	    {
 	      h->root.u.def.section = s;
 	      h->root.u.def.value = h->plt.offset;
-
-	      /* Make sure the function is not marked as Thumb, in case
-		 it is the target of an ABS32 relocation, which will
-		 point to the PLT entry.  */
-	      if (ELF_ST_TYPE (h->type) == STT_ARM_TFUNC)
-		h->type = ELF_ST_INFO (ELF_ST_BIND (h->type), STT_FUNC);
 	    }
 
+	  /* Make sure the function is not marked as Thumb, in case
+	     it is the target of an ABS32 relocation, which will
+	     point to the PLT entry.  */
+	  if (ELF_ST_TYPE (h->type) == STT_ARM_TFUNC)
+	    h->type = ELF_ST_INFO (ELF_ST_BIND (h->type), STT_FUNC);
+
 	  /* Make room for this entry.  */
 	  s->size += htab->plt_entry_size;
 
Index: ld/testsuite/ld-arm/farcall-mixed-app-v5.d
===================================================================
RCS file: /cvs/src/src/ld/testsuite/ld-arm/farcall-mixed-app-v5.d,v
retrieving revision 1.1
diff -u -p -r1.1 farcall-mixed-app-v5.d
--- ld/testsuite/ld-arm/farcall-mixed-app-v5.d	5 Mar 2009 17:28:21 -0000	1.1
+++ ld/testsuite/ld-arm/farcall-mixed-app-v5.d	18 Jun 2009 14:12:36 -0000
@@ -25,11 +25,11 @@ Disassembly of section .text:
  .*:	e1a0c00d 	mov	ip, sp
  .*:	e92dd800 	push	{fp, ip, lr, pc}
  .*:	eb000008 	bl	.* <__app_func_veneer>
+ .*:	ebfffff8 	bl	.* <_start-0xc>
+ .*:	ebfffff4 	bl	.* <_start-0x18>
  .*:	e89d6800 	ldm	sp, {fp, sp, lr}
  .*:	e12fff1e 	bx	lr
  .*:	e1a00000 	nop			\(mov r0,r0\)
- .*:	e1a00000 	nop			\(mov r0,r0\)
- .*:	e1a00000 	nop			\(mov r0,r0\)
 
 .* <app_tfunc_close>:
  .*:	b500      	push	{lr}
@@ -49,12 +49,12 @@ Disassembly of section .far_arm:
 .* <app_func>:
  .*:	e1a0c00d 	mov	ip, sp
  .*:	e92dd800 	push	{fp, ip, lr, pc}
- .*:	eb000008 	bl	.* <__lib_func1_veneer>
+ .*:	eb00000a 	bl	.* <__lib_func1_veneer>
+ .*:	eb000007 	bl	.* <__lib_func2_veneer>
  .*:	e89d6800 	ldm	sp, {fp, sp, lr}
  .*:	e12fff1e 	bx	lr
  .*:	e1a00000 	nop			\(mov r0,r0\)
  .*:	e1a00000 	nop			\(mov r0,r0\)
- .*:	e1a00000 	nop			\(mov r0,r0\)
 
 .* <app_func2>:
  .*:	e12fff1e 	bx	lr
@@ -62,8 +62,11 @@ Disassembly of section .far_arm:
  .*:	e1a00000 	nop			\(mov r0,r0\)
  .*:	e1a00000 	nop			\(mov r0,r0\)
 
+.* <__lib_func2_veneer>:
+ .*:	e51ff004 	ldr	pc, \[pc, #-4\]	; 2100034 <__lib_func2_veneer\+0x4>
+ .*:	00008218 	.word	0x00008218
 .* <__lib_func1_veneer>:
- .*:	e51ff004 	ldr	pc, \[pc, #-4\]	; 2100034 <__lib_func1_veneer\+0x4>
+ .*:	e51ff004 	ldr	pc, \[pc, #-4\]	; 210003c <__lib_func1_veneer\+0x4>
  .*:	00008224 	.word	0x00008224
 
 Disassembly of section .far_thumb:
Index: ld/testsuite/ld-arm/farcall-mixed-app.d
===================================================================
RCS file: /cvs/src/src/ld/testsuite/ld-arm/farcall-mixed-app.d,v
retrieving revision 1.1
diff -u -p -r1.1 farcall-mixed-app.d
--- ld/testsuite/ld-arm/farcall-mixed-app.d	5 Mar 2009 17:28:21 -0000	1.1
+++ ld/testsuite/ld-arm/farcall-mixed-app.d	18 Jun 2009 14:12:36 -0000
@@ -27,11 +27,11 @@ Disassembly of section .text:
  .*:	e1a0c00d 	mov	ip, sp
  .*:	e92dd800 	push	{fp, ip, lr, pc}
  .*:	eb000008 	bl	.* <__app_func_veneer>
+ .*:	ebfffff5 	bl	.* <_start-0x18>
+ .*:	ebfffff1 	bl	.* <_start-0x24>
  .*:	e89d6800 	ldm	sp, {fp, sp, lr}
  .*:	e12fff1e 	bx	lr
  .*:	e1a00000 	nop			\(mov r0,r0\)
- .*:	e1a00000 	nop			\(mov r0,r0\)
- .*:	e1a00000 	nop			\(mov r0,r0\)
 
 .* <app_tfunc_close>:
  .*:	b500      	push	{lr}
@@ -51,12 +51,12 @@ Disassembly of section .far_arm:
 .* <app_func>:
  .*:	e1a0c00d 	mov	ip, sp
  .*:	e92dd800 	push	{fp, ip, lr, pc}
- .*:	eb000008 	bl	.* <__lib_func1_veneer>
+ .*:	eb00000a 	bl	.* <__lib_func1_veneer>
+ .*:	eb000007 	bl	.* <__lib_func2_veneer>
  .*:	e89d6800 	ldm	sp, {fp, sp, lr}
  .*:	e12fff1e 	bx	lr
  .*:	e1a00000 	nop			\(mov r0,r0\)
  .*:	e1a00000 	nop			\(mov r0,r0\)
- .*:	e1a00000 	nop			\(mov r0,r0\)
 
 .* <app_func2>:
  .*:	e12fff1e 	bx	lr
@@ -64,8 +64,11 @@ Disassembly of section .far_arm:
  .*:	e1a00000 	nop			\(mov r0,r0\)
  .*:	e1a00000 	nop			\(mov r0,r0\)
 
+.* <__lib_func2_veneer>:
+ .*:	e51ff004 	ldr	pc, \[pc, #-4\]	; 2100034 <__lib_func2_veneer\+0x4>
+ .*:	0000821c 	.word	0x0000821c
 .* <__lib_func1_veneer>:
- .*:	e51ff004 	ldr	pc, \[pc, #-4\]	; 2100034 <__lib_func1_veneer\+0x4>
+ .*:	e51ff004 	ldr	pc, \[pc, #-4\]	; 210003c <__lib_func1_veneer\+0x4>
  .*:	00008228 	.word	0x00008228
 
 Disassembly of section .far_thumb:
Index: ld/testsuite/ld-arm/farcall-mixed-app.s
===================================================================
RCS file: /cvs/src/src/ld/testsuite/ld-arm/farcall-mixed-app.s,v
retrieving revision 1.1
diff -u -p -r1.1 farcall-mixed-app.s
--- ld/testsuite/ld-arm/farcall-mixed-app.s	5 Mar 2009 17:28:21 -0000	1.1
+++ ld/testsuite/ld-arm/farcall-mixed-app.s	18 Jun 2009 14:12:36 -0000
@@ -5,6 +5,8 @@ _start:
 	mov	ip, sp
 	stmdb	sp!, {r11, ip, lr, pc}
 	bl	app_func
+	bl	lib_func1
+	bl	lib_func2
 	ldmia	sp, {r11, sp, lr}
 	bx lr
 
@@ -30,6 +32,7 @@ app_func:
 	mov	ip, sp
 	stmdb	sp!, {r11, ip, lr, pc}
 	bl	lib_func1
+	bl	lib_func2
 	ldmia	sp, {r11, sp, lr}
 	bx lr
 
Index: ld/testsuite/ld-arm/farcall-mixed-lib.d
===================================================================
RCS file: /cvs/src/src/ld/testsuite/ld-arm/farcall-mixed-lib.d,v
retrieving revision 1.3
diff -u -p -r1.3 farcall-mixed-lib.d
--- ld/testsuite/ld-arm/farcall-mixed-lib.d	21 Apr 2009 22:05:04 -0000	1.3
+++ ld/testsuite/ld-arm/farcall-mixed-lib.d	18 Jun 2009 14:12:36 -0000
@@ -17,6 +17,13 @@ Disassembly of section .plt:
  .*:	e28fc6.* 	add	ip, pc, #.*	; 0x.*
  .*:	e28cca.* 	add	ip, ip, #.*	; 0x.*
  .*:	e5bcf.* 	ldr	pc, \[ip, #.*\]!
+ .*:	e28fc6.* 	add	ip, pc, #.*	; 0x.*
+ .*:	e28cca.* 	add	ip, ip, #.*	; 0x.*
+ .*:	e5bcf.* 	ldr	pc, \[ip, #.*\]!
+ .*:	e28fc6.* 	add	ip, pc, #.*	; 0x.*
+ .*:	e28cca.* 	add	ip, ip, #.*	; 0x.*
+ .*:	e5bcf.* 	ldr	pc, \[ip, #.*\]!
+
 Disassembly of section .text:
 
 .* <lib_func1>:
@@ -24,46 +31,62 @@ Disassembly of section .text:
  .*:	e92dd800 	push	{fp, ip, lr, pc}
  .*:	ebfffff. 	bl	.* <lib_func1-0x..?>
  .*:	ebfffff. 	bl	.* <lib_func1-0x..?>
+ .*:	ebfffff. 	bl	.* <lib_func1-0x..?>
+ .*:	ebfffff. 	bl	.* <lib_func1-0x..?>
  .*:	e89d6800 	ldm	sp, {fp, sp, lr}
  .*:	e12fff1e 	bx	lr
 	...
- .*:	e1a00000 	.word	0xe1a00000
- .*:	e1a00000 	.word	0xe1a00000
 
 .* <lib_func2>:
- .*:	f000 e806 	blx	1000300 <__app_func_from_thumb>
- .*:	f000 e80a 	blx	100030c <__app_func_weak_from_thumb>
+ .*:	f000 e80e 	blx	1000350 <__app_func_from_thumb>
+ .*:	f000 e818 	blx	1000368 <__app_func_weak_from_thumb>
+ .*:	f000 e810 	blx	100035c <__lib_func3_from_thumb>
+ .*:	f000 e81a 	blx	1000374 <__lib_func4_from_thumb>
  .*:	4770      	bx	lr
  .*:	46c0      	nop			\(mov r8, r8\)
  .*:	46c0      	nop			\(mov r8, r8\)
  .*:	46c0      	nop			\(mov r8, r8\)
+ .*:	46c0      	nop			\(mov r8, r8\)
+ .*:	46c0      	nop			\(mov r8, r8\)
+ .*:	46c0      	nop			\(mov r8, r8\)
+ .*:	46c0      	nop			\(mov r8, r8\)
 
 .* <__app_func_from_thumb>:
- .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 1000308 <__app_func_from_thumb\+0x8>
+ .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 1000358 <__app_func_from_thumb\+0x8>
+ .*:	e08ff00c 	add	pc, pc, ip
+ .*:	feffff84 	.word	0xfeffff84
+
+.* <__lib_func3_from_thumb>:
+ .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 1000364 <__lib_func3_from_thumb\+0x8>
  .*:	e08ff00c 	add	pc, pc, ip
- .*:	feffffa8 	.word	0xfeffffa8
+ .*:	feffff90 	.word	0xfeffff90
 
 .* <__app_func_weak_from_thumb>:
- .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 1000314 <__app_func_weak_from_thumb\+0x8>
+ .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 1000370 <__app_func_weak_from_thumb\+0x8>
+ .*:	e08ff00c 	add	pc, pc, ip
+ .*:	feffff78 	.word	0xfeffff78
+
+.* <__lib_func4_from_thumb>:
+ .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 100037c <__lib_func4_from_thumb\+0x8>
  .*:	e08ff00c 	add	pc, pc, ip
- .*:	feffffa8 	.word	0xfeffffa8
+ .*:	feffff84 	.word	0xfeffff84
 	...
 
 .* <lib_func3>:
- .*:	f000 e80c 	blx	200033c <__app_func_from_thumb>
- .*:	f000 e804 	blx	2000330 <__app_func_weak_from_thumb>
+ .*:	f000 e80c 	blx	20003ac <__app_func_from_thumb>
+ .*:	f000 e804 	blx	20003a0 <__app_func_weak_from_thumb>
  .*:	4770      	bx	lr
  .*:	46c0      	nop			\(mov r8, r8\)
  .*:	46c0      	nop			\(mov r8, r8\)
  .*:	46c0      	nop			\(mov r8, r8\)
 
 .* <__app_func_weak_from_thumb>:
- .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 2000338 <__app_func_weak_from_thumb\+0x8>
+ .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 20003a8 <__app_func_weak_from_thumb\+0x8>
  .*:	e08ff00c 	add	pc, pc, ip
- .*:	fdffff84 	.word	0xfdffff84
+ .*:	fdffff40 	.word	0xfdffff40
 
 .* <__app_func_from_thumb>:
- .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 2000344 <__app_func_from_thumb\+0x8>
+ .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 20003b4 <__app_func_from_thumb\+0x8>
  .*:	e08ff00c 	add	pc, pc, ip
- .*:	fdffff6c 	.word	0xfdffff6c
+ .*:	fdffff28 	.word	0xfdffff28
 	...
Index: ld/testsuite/ld-arm/farcall-mixed-lib1.s
===================================================================
RCS file: /cvs/src/src/ld/testsuite/ld-arm/farcall-mixed-lib1.s,v
retrieving revision 1.1
diff -u -p -r1.1 farcall-mixed-lib1.s
--- ld/testsuite/ld-arm/farcall-mixed-lib1.s	17 Apr 2009 13:04:41 -0000	1.1
+++ ld/testsuite/ld-arm/farcall-mixed-lib1.s	18 Jun 2009 14:12:36 -0000
@@ -14,6 +14,8 @@ lib_func1:
 	bl	app_func
 	.weak	app_func_weak
 	bl	app_func_weak
+	bl	lib_func3
+	bl	lib_func4
 	ldmia	sp, {r11, sp, lr}
 	bx lr
 	.size lib_func1, . - lib_func1
@@ -27,5 +29,7 @@ lib_func1:
 lib_func2:
 	bl	app_func
 	bl	app_func_weak
+	bl	lib_func3
+	bl	lib_func4
 	bx lr
 	.size lib_func2, . - lib_func2

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-18 14:36                   ` Christophe LYON
@ 2009-06-22  9:24                     ` Nick Clifton
  2009-06-22 11:33                       ` Christophe LYON
  2009-08-26  1:21                     ` Fix Thumb-2 shared libraries Daniel Jacobowitz
  1 sibling, 1 reply; 22+ messages in thread
From: Nick Clifton @ 2009-06-22  9:24 UTC (permalink / raw)
  To: Christophe LYON; +Cc: binutils, Phil Blundell

Hi Christophe,

> 2009-06-18  Christophe Lyon  <christophe.lyon@st.com>
> 
> 	bfd/
> 	* elf32-arm.c (elf32_arm_size_stubs): Use PLT address as
> 	destination for defined dynamic symbols when deciding whether to
> 	insert a stub or not.
> 	(allocate_dynrelocs): Make sure functions are not marked as Thumb
> 	when actually accessed through a PLT, even when generating a
> 	shared lib.
> 
> 	ld/testsuite:
> 	* ld-arm/farcall-mixed-app.s: Add new references to check more
> 	modes switching.
> 	* ld-arm/farcall-mixed-lib1.s: Likewise.
> 	* ld-arm/farcall-mixed-app-v5.d: Update expected result.
> 	* farcall-mixed-app.d: Likewise.
> 	* ld-arm/farcall-mixed-lib.d: Likewise.

Approved - please apply.

Cheers
   Nick

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: linker crash in arm stub generation
  2009-06-22  9:24                     ` Nick Clifton
@ 2009-06-22 11:33                       ` Christophe LYON
  0 siblings, 0 replies; 22+ messages in thread
From: Christophe LYON @ 2009-06-22 11:33 UTC (permalink / raw)
  To: Nick Clifton; +Cc: binutils, Phil Blundell


> Approved - please apply.
> 
Thanks, I have just committed it.

Christophe.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Fix Thumb-2 shared libraries
  2009-06-18 14:36                   ` Christophe LYON
  2009-06-22  9:24                     ` Nick Clifton
@ 2009-08-26  1:21                     ` Daniel Jacobowitz
  2009-08-26  3:11                       ` Daniel Jacobowitz
  2009-08-26 10:39                       ` Christophe LYON
  1 sibling, 2 replies; 22+ messages in thread
From: Daniel Jacobowitz @ 2009-08-26  1:21 UTC (permalink / raw)
  To: Christophe LYON, binutils

On Thu, Jun 18, 2009 at 04:25:38PM +0200, Christophe LYON wrote:
> Oops! I forgot to attach the patch, sorry.
> 
> 
> On 18.06.2009 16:23, Christophe LYON wrote:
> >Hi,
> >
> >After a bit more work, I am able to propose a new patch for this issue.
> >I have enriched the tests to be sure to check calls inside shared
> >libs to defined/undefined and ARM/Thumb symbols.
> >
> >Checked with arm-none-eabi and arm-linux-gnueabi with no regression.
> >
> >Christophe.

Unfortunately, this patch breaks Thumb-2 shared libraries.

> @@ -11273,14 +11291,14 @@ allocate_dynrelocs (struct elf_link_hash
>  	    {
>  	      h->root.u.def.section = s;
>  	      h->root.u.def.value = h->plt.offset;
> -
> -	      /* Make sure the function is not marked as Thumb, in case
> -		 it is the target of an ABS32 relocation, which will
> -		 point to the PLT entry.  */
> -	      if (ELF_ST_TYPE (h->type) == STT_ARM_TFUNC)
> -		h->type = ELF_ST_INFO (ELF_ST_BIND (h->type), STT_FUNC);
>  	    }
>  
> +	  /* Make sure the function is not marked as Thumb, in case
> +	     it is the target of an ABS32 relocation, which will
> +	     point to the PLT entry.  */
> +	  if (ELF_ST_TYPE (h->type) == STT_ARM_TFUNC)
> +	    h->type = ELF_ST_INFO (ELF_ST_BIND (h->type), STT_FUNC);
> +

The comment doesn't make sense outside of the previous if block.  What
used to happen, inside that block, was we'd mark the function
definition in the ELF symbol table as pointing at the PLT entry.  At
that point, the address of the PLT entry is ARM code, so we had to
adjust the function type.

But now the symbol table still points to the Thumb code, and we've
marked the entry as STT_FUNC.  It ends up without the ISA bit set.
This causes R_ARM_JUMP_SLOT relocations to jump to the target function
as if it were ARM code - does not work.

If I revert just this part of your patch, there is one additional
failure in the testsuite (thanks, as usual, for test cases!).  We use
blx to branch to the PLT entry, so I assume that's what this hunk was
for.  We have to change the use of the symbol in this case, rather
than the definition.

The test still needs a fixup, because the version that's there now
would not work at runtime.  It has:

.* <__lib_func3_from_thumb>:
 .*:    e59fc000        ldr     ip, \[pc, #0\]  ; 1000364 <__lib_func3_from_thumb\+0x8>
 .*:    e08ff00c        add     pc, pc, ip
 .*:    feffff90        .word   0xfeffff90

That will branch to lib_func3 in ARM mode, but lib_func3 is a Thumb
function.

Here's a patch I'm testing for this failure.

-- 
Daniel Jacobowitz
CodeSourcery

2009-08-25  Daniel Jacobowitz  <dan@codesourcery.com>

	* elf32-arm.c (elf32_arm_final_link_relocate): Set sym_flags
	for the mode of target PLT entries.
	(allocate_dynrelocs): Only adjust symbol type if setting its
	value.

Index: elf32-arm.c
===================================================================
--- elf32-arm.c	(revision 259038)
+++ elf32-arm.c	(working copy)
@@ -7330,6 +7330,9 @@ elf32_arm_final_link_relocate (reloc_how
 			   + splt->output_offset
 			   + h->plt.offset);
 		  *unresolved_reloc_p = FALSE;
+		  /* The PLT entry is in ARM mode, regardless of the
+		     target function.  */
+		  sym_flags = STT_FUNC;
 		}
 
 	      /* A branch to an undefined weak symbol is turned into a
@@ -7756,10 +7759,14 @@ elf32_arm_final_link_relocate (reloc_how
  		/* If the Thumb BLX instruction is available, convert the
 		   BL to a BLX instruction to call the ARM-mode PLT entry.  */
 		lower_insn = (lower_insn & ~0x1000) | 0x0800;
+		sym_flags = STT_FUNC;
  	      }
  	    else
- 	      /* Target the Thumb stub before the ARM PLT entry.  */
- 	      value -= PLT_THUMB_STUB_SIZE;
+	      {
+		/* Target the Thumb stub before the ARM PLT entry.  */
+		value -= PLT_THUMB_STUB_SIZE;
+		sym_flags = STT_ARM_TFUNC;
+	      }
 	    *unresolved_reloc_p = FALSE;
 	  }
 
@@ -11753,13 +11760,13 @@ allocate_dynrelocs (struct elf_link_hash
 	    {
 	      h->root.u.def.section = s;
 	      h->root.u.def.value = h->plt.offset;
-	    }
 
-	  /* Make sure the function is not marked as Thumb, in case
-	     it is the target of an ABS32 relocation, which will
-	     point to the PLT entry.  */
-	  if (ELF_ST_TYPE (h->type) == STT_ARM_TFUNC)
-	    h->type = ELF_ST_INFO (ELF_ST_BIND (h->type), STT_FUNC);
+	      /* Make sure the function is not marked as Thumb, in case
+		 it is the target of an ABS32 relocation, which will
+		 point to the PLT entry.  */
+	      if (ELF_ST_TYPE (h->type) == STT_ARM_TFUNC)
+		h->type = ELF_ST_INFO (ELF_ST_BIND (h->type), STT_FUNC);
+	    }
 
 	  /* Make room for this entry.  */
 	  s->size += htab->plt_entry_size;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Fix Thumb-2 shared libraries
  2009-08-26  1:21                     ` Fix Thumb-2 shared libraries Daniel Jacobowitz
@ 2009-08-26  3:11                       ` Daniel Jacobowitz
  2009-09-09 18:36                         ` Daniel Jacobowitz
  2009-08-26 10:39                       ` Christophe LYON
  1 sibling, 1 reply; 22+ messages in thread
From: Daniel Jacobowitz @ 2009-08-26  3:11 UTC (permalink / raw)
  To: Christophe LYON, binutils

On Tue, Aug 25, 2009 at 02:51:58PM -0400, Daniel Jacobowitz wrote:
> Here's a patch I'm testing for this failure.

This time, the whole patch.

-- 
Daniel Jacobowitz
CodeSourcery

2009-08-25  Daniel Jacobowitz  <dan@codesourcery.com>

	bfd/
	* elf32-arm.c (elf32_arm_final_link_relocate): Set sym_flags
	for the mode of target PLT entries.
	(allocate_dynrelocs): Only adjust symbol type if setting its
	value.

2009-08-25  Daniel Jacobowitz  <dan@codesourcery.com>

	ld/testsuite/
	* ld-arm/farcall-mixed-lib.d: Update.

Index: ld/testsuite/ld-arm/farcall-mixed-lib.d
===================================================================
--- ld/testsuite/ld-arm/farcall-mixed-lib.d	(revision 259133)
+++ ld/testsuite/ld-arm/farcall-mixed-lib.d	(working copy)
@@ -39,9 +39,9 @@ Disassembly of section .text:
 
 .* <lib_func2>:
  .*:	f000 e80e 	blx	1000350 <__app_func_from_thumb>
- .*:	f000 e818 	blx	1000368 <__app_func_weak_from_thumb>
- .*:	f000 e810 	blx	100035c <__lib_func3_from_thumb>
- .*:	f000 e81a 	blx	1000374 <__lib_func4_from_thumb>
+ .*:	f000 e81a 	blx	100036c <__app_func_weak_from_thumb>
+ .*:	f000 e810 	blx	100035c <__lib_func3_veneer>
+ .*:	f000 e81c 	blx	1000378 <__lib_func4_from_thumb>
  .*:	4770      	bx	lr
  .*:	46c0      	nop			; \(mov r8, r8\)
  .*:	46c0      	nop			; \(mov r8, r8\)
@@ -56,20 +56,21 @@ Disassembly of section .text:
  .*:	e08ff00c 	add	pc, pc, ip
  .*:	feffff84 	.word	0xfeffff84
 
-.* <__lib_func3_from_thumb>:
- .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 1000364 <__lib_func3_from_thumb\+0x8>
- .*:	e08ff00c 	add	pc, pc, ip
- .*:	feffff90 	.word	0xfeffff90
+.* <__lib_func3_veneer>:
+ .*:	e59fc004 	ldr	ip, \[pc, #4\]	; 1000368 <__lib_func3_veneer\+0xc>
+ .*:	e08fc00c 	add	ip, pc, ip
+ .*:	e12fff1c 	bx	ip
+ .*:	feffff91 	.word	0xfeffff91
 
 .* <__app_func_weak_from_thumb>:
- .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 1000370 <__app_func_weak_from_thumb\+0x8>
+ .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 1000374 <__app_func_weak_from_thumb\+0x8>
  .*:	e08ff00c 	add	pc, pc, ip
- .*:	feffff78 	.word	0xfeffff78
+ .*:	feffff74 	.word	0xfeffff74
 
 .* <__lib_func4_from_thumb>:
- .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 100037c <__lib_func4_from_thumb\+0x8>
+ .*:	e59fc000 	ldr	ip, \[pc, #0\]	; 1000380 <__lib_func4_from_thumb\+0x8>
  .*:	e08ff00c 	add	pc, pc, ip
- .*:	feffff84 	.word	0xfeffff84
+ .*:	feffff80 	.word	0xfeffff80
 	...
 
 .* <lib_func3>:
Index: bfd/elf32-arm.c
===================================================================
--- bfd/elf32-arm.c	(revision 259133)
+++ bfd/elf32-arm.c	(working copy)
@@ -7330,6 +7330,9 @@ elf32_arm_final_link_relocate (reloc_how
 			   + splt->output_offset
 			   + h->plt.offset);
 		  *unresolved_reloc_p = FALSE;
+		  /* The PLT entry is in ARM mode, regardless of the
+		     target function.  */
+		  sym_flags = STT_FUNC;
 		}
 
 	      /* A branch to an undefined weak symbol is turned into a
@@ -7756,10 +7759,14 @@ elf32_arm_final_link_relocate (reloc_how
  		/* If the Thumb BLX instruction is available, convert the
 		   BL to a BLX instruction to call the ARM-mode PLT entry.  */
 		lower_insn = (lower_insn & ~0x1000) | 0x0800;
+		sym_flags = STT_FUNC;
  	      }
  	    else
- 	      /* Target the Thumb stub before the ARM PLT entry.  */
- 	      value -= PLT_THUMB_STUB_SIZE;
+	      {
+		/* Target the Thumb stub before the ARM PLT entry.  */
+		value -= PLT_THUMB_STUB_SIZE;
+		sym_flags = STT_ARM_TFUNC;
+	      }
 	    *unresolved_reloc_p = FALSE;
 	  }
 
@@ -11753,13 +11760,13 @@ allocate_dynrelocs (struct elf_link_hash
 	    {
 	      h->root.u.def.section = s;
 	      h->root.u.def.value = h->plt.offset;
-	    }
 
-	  /* Make sure the function is not marked as Thumb, in case
-	     it is the target of an ABS32 relocation, which will
-	     point to the PLT entry.  */
-	  if (ELF_ST_TYPE (h->type) == STT_ARM_TFUNC)
-	    h->type = ELF_ST_INFO (ELF_ST_BIND (h->type), STT_FUNC);
+	      /* Make sure the function is not marked as Thumb, in case
+		 it is the target of an ABS32 relocation, which will
+		 point to the PLT entry.  */
+	      if (ELF_ST_TYPE (h->type) == STT_ARM_TFUNC)
+		h->type = ELF_ST_INFO (ELF_ST_BIND (h->type), STT_FUNC);
+	    }
 
 	  /* Make room for this entry.  */
 	  s->size += htab->plt_entry_size;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Fix Thumb-2 shared libraries
  2009-08-26  1:21                     ` Fix Thumb-2 shared libraries Daniel Jacobowitz
  2009-08-26  3:11                       ` Daniel Jacobowitz
@ 2009-08-26 10:39                       ` Christophe LYON
  2009-08-26 15:00                         ` Daniel Jacobowitz
  1 sibling, 1 reply; 22+ messages in thread
From: Christophe LYON @ 2009-08-26 10:39 UTC (permalink / raw)
  To: binutils

On 25.08.2009 20:51, Daniel Jacobowitz wrote:
  > Unfortunately, this patch breaks Thumb-2 shared libraries.
Would you mind giving an example?

> But now the symbol table still points to the Thumb code, and we've
> marked the entry as STT_FUNC.  It ends up without the ISA bit set.
> This causes R_ARM_JUMP_SLOT relocations to jump to the target function
> as if it were ARM code - does not work.
I expected this part of allocate_dynrelocs() to be used only by
CALL-like relocs, and thought it was better to fix the definition in one
place, rather than several uses, which is error prone.

> If I revert just this part of your patch, there is one additional
> failure in the testsuite (thanks, as usual, for test cases!).  We use
> blx to branch to the PLT entry, so I assume that's what this hunk was
> for.  We have to change the use of the symbol in this case, rather
> than the definition.
Agreed.

> The test still needs a fixup, because the version that's there now
> would not work at runtime.  It has:
> 
> .* <__lib_func3_from_thumb>:
>  .*:    e59fc000        ldr     ip, \[pc, #0\]  ; 1000364 <__lib_func3_from_thumb\+0x8>
>  .*:    e08ff00c        add     pc, pc, ip
>  .*:    feffff90        .word   0xfeffff90
> 
> That will branch to lib_func3 in ARM mode, but lib_func3 is a Thumb
> function.
> 

Good catch! Sorry not to have noticed this, despite having re-read my
patch quite a few times before committing!

> Here's a patch I'm testing for this failure.
It seems OK, but I think you forgot to add a test case :-)


Christophe.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Fix Thumb-2 shared libraries
  2009-08-26 10:39                       ` Christophe LYON
@ 2009-08-26 15:00                         ` Daniel Jacobowitz
  2009-08-26 17:24                           ` Christophe LYON
  0 siblings, 1 reply; 22+ messages in thread
From: Daniel Jacobowitz @ 2009-08-26 15:00 UTC (permalink / raw)
  To: Christophe LYON; +Cc: binutils

On Wed, Aug 26, 2009 at 11:01:47AM +0200, Christophe LYON wrote:
> On 25.08.2009 20:51, Daniel Jacobowitz wrote:
>  > Unfortunately, this patch breaks Thumb-2 shared libraries.
> Would you mind giving an example?

Sure.  I built glibc as a Thumb-2 binary.  Because we change the type
of function symbols, calloc was marked as an ARM mode entry point in
ld.so's dynamic symbol table.  When ld.so resolves its own
relocations, it filled in an ARM-mode address in the PLT GOT.  So the
first call to calloc from ld.so caused us to execute Thumb code in ARM
mode.

> >Here's a patch I'm testing for this failure.
> It seems OK, but I think you forgot to add a test case :-)

I think your testcase serves nicely - now that I've fixed it to test
for correct behavior :-)

I'm going to wait for another internal test run before I commit this,
but it looks good so far.

-- 
Daniel Jacobowitz
CodeSourcery

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Fix Thumb-2 shared libraries
  2009-08-26 15:00                         ` Daniel Jacobowitz
@ 2009-08-26 17:24                           ` Christophe LYON
  0 siblings, 0 replies; 22+ messages in thread
From: Christophe LYON @ 2009-08-26 17:24 UTC (permalink / raw)
  To: binutils


>>> Here's a patch I'm testing for this failure.
>> It seems OK, but I think you forgot to add a test case :-)
> 
> I think your testcase serves nicely - now that I've fixed it to test
> for correct behavior :-)
> 
I was thinking of a test involving thumb-2, as at first I thought the 
bug occurred only in thumb-2 as you mention in the subject.

> I'm going to wait for another internal test run before I commit this,
> but it looks good so far.
Good!

Christophe.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Fix Thumb-2 shared libraries
  2009-08-26  3:11                       ` Daniel Jacobowitz
@ 2009-09-09 18:36                         ` Daniel Jacobowitz
  2009-09-14 12:30                           ` Daniel Jacobowitz
  0 siblings, 1 reply; 22+ messages in thread
From: Daniel Jacobowitz @ 2009-09-09 18:36 UTC (permalink / raw)
  To: binutils, Tristan Gingold; +Cc: Christophe LYON

On Tue, Aug 25, 2009 at 06:14:33PM -0400, Daniel Jacobowitz wrote:
> 2009-08-25  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	bfd/
> 	* elf32-arm.c (elf32_arm_final_link_relocate): Set sym_flags
> 	for the mode of target PLT entries.
> 	(allocate_dynrelocs): Only adjust symbol type if setting its
> 	value.
> 
> 2009-08-25  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	ld/testsuite/
> 	* ld-arm/farcall-mixed-lib.d: Update.

I have belatedly checked this in.

Tristan, I've carelessly missed the branch creation.  Is it OK to
apply this to the 2.20 branch?


-- 
Daniel Jacobowitz
CodeSourcery

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Fix Thumb-2 shared libraries
  2009-09-09 18:36                         ` Daniel Jacobowitz
@ 2009-09-14 12:30                           ` Daniel Jacobowitz
  0 siblings, 0 replies; 22+ messages in thread
From: Daniel Jacobowitz @ 2009-09-14 12:30 UTC (permalink / raw)
  To: binutils; +Cc: Tristan Gingold, Christophe LYON

On Wed, Sep 09, 2009 at 02:36:09PM -0400, Daniel Jacobowitz wrote:
> I have belatedly checked this in.
> 
> Tristan, I've carelessly missed the branch creation.  Is it OK to
> apply this to the 2.20 branch?

Checked in on the branch.

-- 
Daniel Jacobowitz
CodeSourcery

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2009-09-14 12:30 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-11 17:09 linker crash in arm stub generation Phil Blundell
2009-06-12 12:36 ` Nick Clifton
2009-06-12 13:19   ` Christophe LYON
2009-06-12 13:49     ` Nick Clifton
2009-06-12 14:06     ` Daniel Jacobowitz
2009-06-12 14:13       ` Christophe LYON
2009-06-15 14:22         ` Christophe LYON
2009-06-15 17:59           ` Daniel Jacobowitz
2009-06-17 15:44             ` Christophe LYON
2009-06-17 16:09               ` Phil Blundell
2009-06-17 18:10               ` Daniel Jacobowitz
2009-06-18 14:25                 ` Christophe LYON
2009-06-18 14:36                   ` Christophe LYON
2009-06-22  9:24                     ` Nick Clifton
2009-06-22 11:33                       ` Christophe LYON
2009-08-26  1:21                     ` Fix Thumb-2 shared libraries Daniel Jacobowitz
2009-08-26  3:11                       ` Daniel Jacobowitz
2009-09-09 18:36                         ` Daniel Jacobowitz
2009-09-14 12:30                           ` Daniel Jacobowitz
2009-08-26 10:39                       ` Christophe LYON
2009-08-26 15:00                         ` Daniel Jacobowitz
2009-08-26 17:24                           ` Christophe LYON

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).