[PATCH] Linker fix for data-only sections

[PATCH] Linker fix for data-only sections

[Daniel Gutson]

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

The attached patch fixes a bug that caused the linker to incorrectly 
mark parts of the output as containing code, rather than data,
when linking data-only sections not explicitly tagged as such.
The bug resulted in incorrect disassembly.

Technical details:
The bug occurred when a data-only section (without a $d mapping symbol,
which is permitted) was inserted after a section containing mapping
symbols (where the last one wasn't a $d), inside the same output
section. So, the data-only 'inherited' the effect of the last mapping
symbol since it didn't provide one.

The attached patch attempts to fix this issue by always adding a $d at
the beginning of data sections that don't have a mapping symbol. This 
may result in (harmless) redundancy of mapping symbols.
I added this fix in the elf_backend_output_arch_local_syms hook, since 
it looks for this purpose:
[elflink.c, calling the hook:]
   /* If backend needs to output some local symbols not present in the hash
      table, do it now.  */

I tested this fix by running the following test suites:
     * binutils
     * gas
     * ld (with a new test case)
     * gcc
     * g++
     * libstdc++

Please let me know if OK to commit.

ChangeLog:

2010-02-02  Daniel Gutson  <dgutson@codesourcery.com>

	bfd/
	* elf32-arm.c (elf32_arm_output_arch_local_syms): add
	missing mapping symbol to data only sections.

	ld/testsuite/
	* ld-arm/arm-elf.exp (armelftests): New test case added.
	* ld-arm/data-only-map.s: New file.
	* ld-arm/data-only-map.d: New file.

Thanks!
	Daniel.

-- 
Daniel Gutson
CodeSourcery
www.codesourcery.com

[-- Attachment #2: data-only-map.patch --]
[-- Type: text/x-diff, Size: 4464 bytes --]

? data-only-map.patch
Index: bfd/elf32-arm.c
===================================================================
RCS file: /cvs/src/src/bfd/elf32-arm.c,v
retrieving revision 1.219
diff -u -p -r1.219 elf32-arm.c
--- bfd/elf32-arm.c	19 Jan 2010 03:49:43 -0000	1.219
+++ bfd/elf32-arm.c	2 Feb 2010 17:26:35 -0000
@@ -12942,7 +12942,9 @@ arm_map_one_stub (struct bfd_hash_entry 
   return TRUE;
 }
 
-/* Output mapping symbols for linker generated sections.  */
+/* Output mapping symbols for linker generated sections,
+   and for those data-only sections that do not have a
+   $d.  */
 
 static bfd_boolean
 elf32_arm_output_arch_local_syms (bfd *output_bfd,
@@ -12957,6 +12959,7 @@ elf32_arm_output_arch_local_syms (bfd *o
   struct elf32_arm_link_hash_table *htab;
   bfd_vma offset;
   bfd_size_type size;
+  bfd *input_bfd;
 
   htab = elf32_arm_hash_table (info);
   check_use_blx (htab);
@@ -12965,6 +12968,32 @@ elf32_arm_output_arch_local_syms (bfd *o
   osi.info = info;
   osi.func = func;
 
+  /* Add a $d mapping symbol to data-only sections that
+     don't have any mapping symbol.  This may result in (harmless) redundant
+     mapping symbols.  */
+  for (input_bfd = info->input_bfds;
+       input_bfd != NULL;
+       input_bfd = input_bfd->link_next)
+    {
+      if ((input_bfd->flags & (BFD_LINKER_CREATED | HAS_SYMS)) == HAS_SYMS)
+	for (osi.sec = input_bfd->sections;
+	     osi.sec != NULL;
+	     osi.sec = osi.sec->next)
+	  {
+	    if (osi.sec->output_section != NULL
+		&& (osi.sec->flags & (SEC_HAS_CONTENTS | SEC_LINKER_CREATED))
+		   == SEC_HAS_CONTENTS
+		&& get_arm_elf_section_data (osi.sec) != NULL
+		&& get_arm_elf_section_data (osi.sec)->mapcount == 0)
+	      {
+		osi.sec_shndx = _bfd_elf_section_from_bfd_section
+		  (output_bfd, osi.sec->output_section);
+		if (osi.sec_shndx != (int)SHN_BAD)
+		  elf32_arm_output_map_sym (&osi, ARM_MAP_DATA, 0);
+	      }
+	  }
+    }
+
   /* ARM->Thumb glue.  */
   if (htab->arm_glue_size > 0)
     {
Index: ld/testsuite/ld-arm/arm-elf.exp
===================================================================
RCS file: /cvs/src/src/ld/testsuite/ld-arm/arm-elf.exp,v
retrieving revision 1.69
diff -u -p -r1.69 arm-elf.exp
--- ld/testsuite/ld-arm/arm-elf.exp	1 Feb 2010 10:33:16 -0000	1.69
+++ ld/testsuite/ld-arm/arm-elf.exp	2 Feb 2010 17:26:35 -0000
@@ -241,6 +241,9 @@ set armelftests {
     {"Relocation boundaries" "-defsym x=0 -defsym y=0 -defsym _start=0" "" {reloc-boundaries.s}
      {{objdump -s reloc-boundaries.d}}
      "reloc-boundaries"}
+    {"Data only mapping symbols" "-T data-only-map.ld -Map map" "" {data-only-map.s}
+     {{objdump -dr data-only-map.d}}
+     "data-only-map"}
 }
 
 run_ld_link_tests $armelftests
Index: ld/testsuite/ld-arm/data-only-map.d
===================================================================
RCS file: ld/testsuite/ld-arm/data-only-map.d
diff -N ld/testsuite/ld-arm/data-only-map.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-arm/data-only-map.d	2 Feb 2010 17:26:35 -0000
@@ -0,0 +1,13 @@
+
+[^:]*:     file format elf32-littlearm
+
+
+Disassembly of section \.text:
+
+00000000 <_start>:
+   0:	eb01 0002 	add\.w	r0, r1, r2
+   4:	eb010002 	\.word	0xeb010002
+   8:	eb01 0002 	add\.w	r0, r1, r2
+   c:	eb01 0200 	add\.w	r2, r1, r0
+  10:	eb010002 	\.word	0xeb010002
+  14:	eb010002 	\.word	0xeb010002
Index: ld/testsuite/ld-arm/data-only-map.ld
===================================================================
RCS file: ld/testsuite/ld-arm/data-only-map.ld
diff -N ld/testsuite/ld-arm/data-only-map.ld
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-arm/data-only-map.ld	2 Feb 2010 17:26:35 -0000
@@ -0,0 +1,16 @@
+/* Script for ld testsuite */
+OUTPUT_ARCH(arm)
+ENTRY(_start)
+SECTIONS
+{
+  .text :
+  {
+    *(.text)
+    *(.after1)
+    *(.after2)
+    *(.after3)
+    *(.after4)
+    *(.after5)
+  } =0
+}
+
Index: ld/testsuite/ld-arm/data-only-map.s
===================================================================
RCS file: ld/testsuite/ld-arm/data-only-map.s
diff -N ld/testsuite/ld-arm/data-only-map.s
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-arm/data-only-map.s	2 Feb 2010 17:26:35 -0000
@@ -0,0 +1,20 @@
+.syntax unified
+.thumb
+.global _start
+_start:
+add.w r0, r1, r2
+
+.section .after1
+.word 0xeb010002
+
+.section .after2
+add.w r0, r1, r2
+
+.section .after3
+add.w r2, r1, r0
+
+.section .after4
+.word 0xeb010002
+
+.section .after5
+.word 0xeb010002

Re: [PATCH] Linker fix for data-only sections

[Nick Clifton]

Hi Daniel,

> The attached patch attempts to fix this issue by always adding a $d at
> the beginning of data sections that don't have a mapping symbol.

I may be missing something here, but where in your patch do you check to 
make sure that the section is a data section and not a code section ?

Cheers
   Nick

Re: [PATCH] Linker fix for data-only sections

[Daniel Gutson]

Nick Clifton wrote:
> Hi Daniel,
> 
>> The attached patch attempts to fix this issue by always adding a $d at
>> the beginning of data sections that don't have a mapping symbol.
> 
> I may be missing something here, but where in your patch do you check to 
> make sure that the section is a data section and not a code section ?
> 

Hi Nick,
	thanks for reading the patch; maybe there's a confusion: ARM sections 
may contain both code and data, ruled by the mapping symbols, so 
(according to the EABI) we don't care if the section happens to have the 
executable attribute.

I originally checked for the BFD section data flags, but the 'data-only' 
flag is not useful here.

	Daniel.

-- 
Daniel Gutson
CodeSourcery
www.codesourcery.com

Re: [PATCH] Linker fix for data-only sections

[Nick Clifton]

Hi Daniel,

>> I may be missing something here, but where in your patch do you check
>> to make sure that the section is a data section and not a code section ?

> ARM sections may contain both code and data, ruled by the mapping symbols,
> so (according to the EABI) we don't care if the section happens to have the
> executable attribute.

Doh - I had forgotten that.  Of course this only applies to EABI 
conformant ARM binaries.  So maybe you ought to check for non-eabi 
targets and handle them appropriately ?

> I originally checked for the BFD section data flags, but the 'data-only'
> flag is not useful here.

But there is a SEC_CODE flag which is present in code sections and 
absent in data sections.

Cheers
   Nick

Re: [PATCH] Linker fix for data-only sections

[Daniel Gutson]

Hi Nick,

Nick Clifton wrote:
> Hi Daniel,
> 
>>> I may be missing something here, but where in your patch do you check
>>> to make sure that the section is a data section and not a code section ?
> 
>> ARM sections may contain both code and data, ruled by the mapping 
>> symbols,
>> so (according to the EABI) we don't care if the section happens to 
>> have the
>> executable attribute.
> 
> Doh - I had forgotten that.  Of course this only applies to EABI 
> conformant ARM binaries.  So maybe you ought to check for non-eabi 
> targets and handle them appropriately ?

Could you please ellaborate? AFAIK, this is ARM (not only EABI), please 
let me know.

> 
>> I originally checked for the BFD section data flags, but the 'data-only'
>> flag is not useful here.
> 
> But there is a SEC_CODE flag which is present in code sections and 
> absent in data sections.

Yes, I also checked for that too, but (IIUC) I don't think it's relevant 
checking for it due to the same reason.

    Daniel.

-- 
Daniel Gutson
CodeSourcery
www.codesourcery.com

Re: [PATCH] Linker fix for data-only sections

[Nick Clifton]

Hi Daniel,

>>>> I may be missing something here, but where in your patch do you check
>>>> to make sure that the section is a data section and not a code
>>>> section ?

>>> ARM sections may contain both code and data, ruled by the mapping
>>> symbols, so (according to the EABI) we don't care if the section
>>> happens to have the executable attribute.
>>
>> Doh - I had forgotten that. Of course this only applies to EABI
>> conformant ARM binaries. So maybe you ought to check for non-eabi
>> targets and handle them appropriately ?
>
> Could you please ellaborate? AFAIK, this is ARM (not only EABI), please
> let me know.

What I meant was that the EABI specifies that a section without mapping 
symbols should be treated as if it contained data, regardless of any 
attributes that might apply to that section.  (I think that this is 
correct, I do not actually have the text of the EABI in front of me).

But for arm-elf binaries, or arm-wince, or arm-pe, the EABI is not being 
used and in these cases it would make intuitive sense that sections 
without mapping symbols but with a contains-executable-code attribute of 
some kind should be treated as if they contained code.

Does this make sense ?

Cheers
   Nick

Re: [PATCH] Linker fix for data-only sections

[Paul Brook]

> But for arm-elf binaries, or arm-wince, or arm-pe, the EABI is not being
> used and in these cases it would make intuitive sense that sections
> without mapping symbols but with a contains-executable-code attribute of
> some kind should be treated as if they contained code.

I don't but that argument. AFAIK arm-pe doesn't have mapping symbols at all - 
it certainly shouldn't be effected by anything in elf32-arm.c.

Even for arm-elf I find your argument somewhat sketchy. If you don't have 
correct mapping symbols on you input objects then I find it hard to believe 
you care about mapping symbols in the result.

IMO assuming data is the only sane option. "Contains code" isn't a useful 
statement on ARM. You need to know the exact combination of ARM, Thumb and 
inline Data, which is why mapping symbols exist in the first place.

If you really want to try and do something sensible with legacy objects 
lacking mapping symbols you'd probably want to base it on function symbol 
types. Even then I wouldn't expect to get useful results in anything other 
than trivial cases.

Paul

Re: [PATCH] Linker fix for data-only sections

[Nick Clifton]

Hi Paul,

> IMO assuming data is the only sane option. "Contains code" isn't a useful
> statement on ARM. You need to know the exact combination of ARM, Thumb and
> inline Data, which is why mapping symbols exist in the first place.

Hmm, fair enough.  In which case please consider your patch approved.

Cheers
   Nick