[PATCH] binutils/objcopy: keep relocation while renaming a section with explicit flags

[PATCH] binutils/objcopy: keep relocation while renaming a section with explicit flags

[Patrick Monnerat]

When explicit flags are listed in a --rename-section option, they
override the input section's flags.
There is no way to list the "reloc" flag explicitly, thus the original
one must be kept. Failure to do so causes the section's relocation to
be dropped.
This patch merges the original SEC_RELOC flag within the renamed section
flags. It also introduces a test case for it.
---
 binutils/objcopy.c                                  |  1 +
 binutils/testsuite/binutils-all/objcopy.exp         |  2 ++
 binutils/testsuite/binutils-all/rename-section-01.d | 12 ++++++++++++
 3 files changed, 15 insertions(+)
 create mode 100644 binutils/testsuite/binutils-all/rename-section-01.d

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index d886e3ae343..f195e4d875e 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4052,6 +4052,7 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
   if (new_name != name)
     {
       name = new_name;
+      flags |= bfd_section_flags (isection) & SEC_RELOC;
       flags = check_new_section_flags (flags, obfd, name);
     }
 
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index 5871d431eea..aebfdb2090b 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -1448,3 +1448,5 @@ if { [istarget *-*-cygwin] || [istarget *-*-mingw*] } {
 if { ![is_xcoff_format] } {
     objcopy_test "pr25662" $src executable "" $ldflags
 }
+
+run_dump_test "rename-section-01"
diff --git a/binutils/testsuite/binutils-all/rename-section-01.d b/binutils/testsuite/binutils-all/rename-section-01.d
new file mode 100644
index 00000000000..b0282fc9552
--- /dev/null
+++ b/binutils/testsuite/binutils-all/rename-section-01.d
@@ -0,0 +1,12 @@
+#PROG: objcopy
+#name: objcopy rename-section with flags - keep relocation
+#source: needed-by-reloc.s
+#objcopy: --rename-section .data=rodata,contents,alloc,load,readonly
+#objdump: -r
+
+.*: +file format .*
+
+RELOCATION RECORDS FOR \[rodata\]:
+OFFSET +TYPE +VALUE
+0+ +[^ ]+ +foo
+
-- 
2.38.1

Re: [PATCH] binutils/objcopy: keep relocation while renaming a section with explicit flags

[Alan Modra]

On Sat, Nov 12, 2022 at 01:44:41PM +0100, Patrick Monnerat via Binutils wrote:
> When explicit flags are listed in a --rename-section option, they
> override the input section's flags.
> There is no way to list the "reloc" flag explicitly, thus the original
> one must be kept. Failure to do so causes the section's relocation to
> be dropped.
> This patch merges the original SEC_RELOC flag within the renamed section
> flags. It also introduces a test case for it.

Thanks for working on this, but I'm going to apply a different patch.
SEC_RELOC is an odd flag, mostly redundant.  I think it's better to
handle setting or clearing the flag in BFD.  The testcase needed
tweaking too.  Some targets don't support named sections or have
different names for .data, and some targets reduce the reloc symbol
"foo" to a section symbol.

bfd/
	* reloc.c (_bfd_generic_set_reloc): Set/clear SEC_RELOC depending
	on reloc count.
	* elf64-sparc.c (elf64_sparc_set_reloc): Likewise.
binutils/
	* objcopy.c (copy_relocations_in_section): Remove now unnecessary
	clearing of SEC_RELOC.
	* testsuite/binutils-all/rename-section-01.d: New test.
	* testsuite/binutils-all/objcopy.exp: Run it.
gas/
	* write.c (size_seg): Remove unneccesary twiddle of SEC_RELOC.
	(write_relocs): Likewise.  Always call bfd_set_reloc.

diff --git a/bfd/elf64-sparc.c b/bfd/elf64-sparc.c
index fb4483dcd17..c6d0d3e5b0a 100644
--- a/bfd/elf64-sparc.c
+++ b/bfd/elf64-sparc.c
@@ -322,6 +322,10 @@ elf64_sparc_set_reloc (bfd *abfd ATTRIBUTE_UNUSED,
 {
   asect->orelocation = location;
   canon_reloc_count (asect) = count;
+  if (count != 0)
+    asect->flags |= SEC_RELOC;
+  else
+    asect->flags &= ~SEC_RELOC;
 }
 
 /* Write out the relocs.  */
diff --git a/bfd/reloc.c b/bfd/reloc.c
index 89b6f7fd352..6446acc7a30 100644
--- a/bfd/reloc.c
+++ b/bfd/reloc.c
@@ -8706,6 +8706,10 @@ _bfd_generic_set_reloc (bfd *abfd ATTRIBUTE_UNUSED,
 {
   section->orelocation = relptr;
   section->reloc_count = count;
+  if (count != 0)
+    section->flags |= SEC_RELOC;
+  else
+    section->flags &= ~SEC_RELOC;
 }
 
 /*
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index d886e3ae343..3d886240ce1 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4331,10 +4331,7 @@ copy_relocations_in_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
     }
 
   if (relsize == 0)
-    {
-      bfd_set_reloc (obfd, osection, NULL, 0);
-      osection->flags &= ~SEC_RELOC;
-    }
+    bfd_set_reloc (obfd, osection, NULL, 0);
   else
     {
       if (isection->orelocation != NULL)
@@ -4377,8 +4374,6 @@ copy_relocations_in_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
 	}
 
       bfd_set_reloc (obfd, osection, relcount == 0 ? NULL : relpp, relcount);
-      if (relcount == 0)
-	osection->flags &= ~SEC_RELOC;
     }
 }
 
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index 5871d431eea..aebfdb2090b 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -1448,3 +1448,5 @@ if { [istarget *-*-cygwin] || [istarget *-*-mingw*] } {
 if { ![is_xcoff_format] } {
     objcopy_test "pr25662" $src executable "" $ldflags
 }
+
+run_dump_test "rename-section-01"
diff --git a/binutils/testsuite/binutils-all/rename-section-01.d b/binutils/testsuite/binutils-all/rename-section-01.d
new file mode 100644
index 00000000000..21d901326c6
--- /dev/null
+++ b/binutils/testsuite/binutils-all/rename-section-01.d
@@ -0,0 +1,14 @@
+#PROG: objcopy
+#name: objcopy rename-section with flags - keep relocation
+#source: needed-by-reloc.s
+#objcopy: --rename-section .data=myrodata,contents,alloc,load,readonly
+#objdump: -r
+#notarget: alpha*-*-*vms* rx-*-elf [is_som_format] [is_aout_format]
+
+.*: +file format .*
+
+#...
+RELOCATION RECORDS FOR .*myrodata.*:
+OFFSET +TYPE +VALUE
+0+ .*
+#pass
diff --git a/gas/write.c b/gas/write.c
index 1c1b8104222..3014f687949 100644
--- a/gas/write.c
+++ b/gas/write.c
@@ -579,7 +579,6 @@ size_seg (bfd *abfd ATTRIBUTE_UNUSED, asection *sec, void *xxx ATTRIBUTE_UNUSED)
   if (size > 0 && ! seginfo->bss)
     flags |= SEC_HAS_CONTENTS;
 
-  flags &= ~SEC_RELOC;
   x = bfd_set_section_flags (sec, flags);
   gas_assert (x);
 
@@ -1385,13 +1384,7 @@ write_relocs (bfd *abfd ATTRIBUTE_UNUSED, asection *sec,
   }
 #endif
 
-  if (n)
-    {
-      flagword flags = bfd_section_flags (sec);
-      flags |= SEC_RELOC;
-      bfd_set_section_flags (sec, flags);
-      bfd_set_reloc (stdoutput, sec, relocs, n);
-    }
+  bfd_set_reloc (stdoutput, sec, n ? relocs : NULL, n);
 
 #ifdef SET_SECTION_RELOCS
   SET_SECTION_RELOCS (sec, relocs, n);

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] binutils/objcopy: keep relocation while renaming a section with explicit flags

[Alan Modra]

For now, xfail the new test.  Some header/aux-header rewriting is
required at the very least.

	* testsuite/binutils-all/rename-section-01.d: xfail xcoff.

diff --git a/binutils/testsuite/binutils-all/rename-section-01.d b/binutils/testsuite/binutils-all/rename-section-01.d
index 21d901326c6..4bde93a2e0b 100644
--- a/binutils/testsuite/binutils-all/rename-section-01.d
+++ b/binutils/testsuite/binutils-all/rename-section-01.d
@@ -4,6 +4,7 @@
 #objcopy: --rename-section .data=myrodata,contents,alloc,load,readonly
 #objdump: -r
 #notarget: alpha*-*-*vms* rx-*-elf [is_som_format] [is_aout_format]
+#xfail: [is_xcoff_format]
 
 .*: +file format .*
 

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] binutils/objcopy: keep relocation while renaming a section with explicit flags

[Patrick Monnerat]

Thanks for your objcopy fix Alan. It works for me.

On 11/14/22 04:14, Alan Modra via Binutils wrote:
> For now, xfail the new test.  Some header/aux-header rewriting is
> required at the very least.
>
> 	* testsuite/binutils-all/rename-section-01.d: xfail xcoff.
>
> diff --git a/binutils/testsuite/binutils-all/rename-section-01.d b/binutils/testsuite/binutils-all/rename-section-01.d
> index 21d901326c6..4bde93a2e0b 100644
> --- a/binutils/testsuite/binutils-all/rename-section-01.d
> +++ b/binutils/testsuite/binutils-all/rename-section-01.d
> @@ -4,6 +4,7 @@
>   #objcopy: --rename-section .data=myrodata,contents,alloc,load,readonly
>   #objdump: -r
>   #notarget: alpha*-*-*vms* rx-*-elf [is_som_format] [is_aout_format]
> +#xfail: [is_xcoff_format]
>   
>   .*: +file format .*
>   
>