[RFC PATCH] bfd: Handle objcopy --only-keep-debug in _bfd_elf_init_private_section_data()

[RFC PATCH] bfd: Handle objcopy --only-keep-debug in _bfd_elf_init_private_section_data()

[Peilin Ye]

From: Peilin Ye <peilin.ye@bytedance.com>

objcopy's --only-keep-debug option has been broken for ELF files since
commit 8c803a2dd7d3 ("elf_backend_section_flags and
_bfd_elf_init_private_section_data"):

  1. binutils/objcopy.c:setup_section() marks non-debug sections as
     SHT_NOBITS, then calls bfd_copy_private_section_data(), see
     "mark_nobits";
  2. If ISEC and OSEC share the same section flags,
     bfd/elf.c:_bfd_elf_init_private_section_data() restores OSEC's
     section type back to ISEC's section type, effectively undoing
     "mark_nobits".

This makes objcopy's --only-keep-debug a no-op.  As an example, Linux's
scripts/package/builddeb script depends on this option to generate
debug-info-only kernel modules.

Handle this special case by checking OSEC's size in addition to its
type (similar to bfd/elf.c:elf_fake_sections()).  A non-debug section
marked as SHT_NOBITS by --only-keep-debug should have a non-zero size.

bfd/ChangeLog:
	* elf.c (_bfd_elf_init_private_section_data): Handle SHT_NOBITS
	  sections marked by objcopy --only-keep-debug

Reported-by: Omar Sandoval <osandov@osandov.com>
Fixes: 8c803a2dd7d3 ("elf_backend_section_flags and _bfd_elf_init_private_section_data")
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
Hi all,

We ran into this bug yesterday, sorry for sending this right before 2.38
release date.  This patch is manually tested only.  Which testsuite(s)
should I use to test this patch?  I browsed bfd/README and
bfd/doc/bfdint.texi but failed to figure out.  Please advise, thank you!

Thanks,
Peilin Ye
<peilin.ye@bytedance.com>

 bfd/elf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bfd/elf.c b/bfd/elf.c
index 14c2c7ba734d..0c0a3e0a870b 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -7887,7 +7887,7 @@ _bfd_elf_init_private_section_data (bfd *ibfd,
      SHF_MASKOS and SHF_MASKPROC.  */
   if (elf_section_type (osec) == SHT_PROGBITS
       || elf_section_type (osec) == SHT_NOTE
-      || elf_section_type (osec) == SHT_NOBITS)
+      || (elf_section_type (osec) == SHT_NOBITS && osec->size == 0))
     elf_section_type (osec) = SHT_NULL;
   /* For objcopy and relocatable link, copy the ELF section type from
      the input file if the BFD section flags are the same.  (If they
-- 
2.20.1

Re: [RFC PATCH] bfd: Handle objcopy --only-keep-debug in _bfd_elf_init_private_section_data()

[Nick Clifton]

Hi Peilin,

> objcopy's --only-keep-debug option has been broken for ELF files since
> commit 8c803a2dd7d3 ("elf_backend_section_flags and
> _bfd_elf_init_private_section_data"):
> 
>    1. binutils/objcopy.c:setup_section() marks non-debug sections as
>       SHT_NOBITS, then calls bfd_copy_private_section_data(), see
>       "mark_nobits";
>    2. If ISEC and OSEC share the same section flags,
>       bfd/elf.c:_bfd_elf_init_private_section_data() restores OSEC's
>       section type back to ISEC's section type, effectively undoing
>       "mark_nobits".
> 
> This makes objcopy's --only-keep-debug a no-op.  As an example, Linux's
> scripts/package/builddeb script depends on this option to generate
> debug-info-only kernel modules.
> 
> Handle this special case by checking OSEC's size in addition to its
> type (similar to bfd/elf.c:elf_fake_sections()).  A non-debug section
> marked as SHT_NOBITS by --only-keep-debug should have a non-zero size.

I have a feeling that this change is going to have wider repercussions
than simply fixing this problem.  It is going to affect those sections
created by elf_fake_sections() for example, and I bet that there will
be other scenarios that have not occurred to me.

Possibly a better fix would be to change objcopy instead so that it can
cope with the behaviour of bfd_copy_private_section_data().  For example,
please could you try out this patch and see if it fixes the problem ?


diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index d16d8ee67e4..14b76d8a65a 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4180,6 +4180,10 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
        err = _("failed to copy private data");
        goto loser;
      }
+  /* Since isec.flags == osec.flags, bfd_copy_private_section_data()
+     will reset the section type.  */
+  if (make_nobits)
+    elf_section_type (osection) = SHT_NOBITS;

    /* All went well.  */
    return;


> This patch is manually tested only.  Which testsuite(s)
> should I use to test this patch? 

Please run the binutils testsuite.  ie:

   cd <build-dir>/binutils
   make check

Cheers
   Nick

Re: [RFC PATCH] bfd: Handle objcopy --only-keep-debug in _bfd_elf_init_private_section_data()

[Alan Modra]

Thanks for the clear analysis and patch.  I think I prefer not to copy
the hack I added in elf_fake_sections when it ought to work to simply
do the following:

	* objcopy.c (setup_section): Act on make_nobits after calling
	bfd_copy_private_section_data.

Do you have a testcase we could add to the binutils testsuite?

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index d16d8ee67e4..d53aa5c6000 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4085,9 +4085,6 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
       goto loser;
     }
 
-  if (make_nobits)
-    elf_section_type (osection) = SHT_NOBITS;
-
   size = bfd_section_size (isection);
   size = bfd_convert_section_size (ibfd, isection, obfd, size);
   if (copy_byte >= 0)
@@ -4181,6 +4178,9 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
       goto loser;
     }
 
+  if (make_nobits)
+    elf_section_type (osection) = SHT_NOBITS;
+
   /* All went well.  */
   return;
 


-- 
Alan Modra
Australia Development Lab, IBM

[PATCH v2] objcopy: Fix --only-keep-debug

[Peilin Ye]

From: Peilin Ye <peilin.ye@bytedance.com>

objcopy's --only-keep-debug option has been broken for ELF files since
commit 8c803a2dd7d3 ("elf_backend_section_flags and
_bfd_elf_init_private_section_data"):

  1. binutils/objcopy.c:setup_section() marks non-debug OSECs as
     SHT_NOBITS, then calls bfd_copy_private_section_data(), see
     "mark_nobits";
  2. If ISEC and OSEC share the same section flags,
     bfd/elf.c:_bfd_elf_init_private_section_data() restores OSEC's
     section type back to ISEC's section type, effectively undoing
     "mark_nobits".

This makes objcopy's --only-keep-debug a no-op.  As an example, Linux's
scripts/package/builddeb script depends on this option to generate
debug-info-only kernel modules.

As suggested by Nick and Alan, fix --only-keep-debug by marking
non-debug OSECs as SHT_NOBITS after calling
bfd_copy_private_section_data() in setup_section().

binutils/ChangeLog:
	* objcopy.c (setup_section): For --only-keep-debug, mark
	  non-debug output sections as SHT_NOBITS after calling
	  bfd_copy_private_section_data()

Reported-by: Omar Sandoval <osandov@osandov.com>
Fixes: 8c803a2dd7d3 ("elf_backend_section_flags and _bfd_elf_init_private_section_data")
Suggested-by: Alan Modra <amodra@gmail.com>
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
change since v1:
	- fix objcopy instead of touching _bfd_elf_init_private_section_data()
	  (suggested by Nick and Alan)

 binutils/objcopy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index d16d8ee67e4d..d53aa5c6000f 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4085,9 +4085,6 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
       goto loser;
     }
 
-  if (make_nobits)
-    elf_section_type (osection) = SHT_NOBITS;
-
   size = bfd_section_size (isection);
   size = bfd_convert_section_size (ibfd, isection, obfd, size);
   if (copy_byte >= 0)
@@ -4181,6 +4178,9 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
       goto loser;
     }
 
+  if (make_nobits)
+    elf_section_type (osection) = SHT_NOBITS;
+
   /* All went well.  */
   return;
 
-- 
2.20.1

Re: [RFC PATCH] bfd: Handle objcopy --only-keep-debug in _bfd_elf_init_private_section_data()

[Peilin Ye]

Hi Nick and Alan,

Thanks for the suggestion!  I tried both approaches but wasn't sure
which one is better.

I've tested v2 with the binutils testsuite.

On Fri, Jan 28, 2022 at 11:22:36PM +1030, Alan Modra wrote:
> Do you have a testcase we could add to the binutils testsuite?

Not yet, I just noticed that we have a
keep_debug_symbols_and_test_copy() test in objcopy.exp, I can take a
look later today if needed.

Thanks,
Peilin Ye
<peilin.ye@bytedance.com>

Re: [RFC PATCH] bfd: Handle objcopy --only-keep-debug in _bfd_elf_init_private_section_data()

[Peilin Ye]

Hi Alan,

On Fri, Jan 28, 2022 at 11:22:36PM +1030, Alan Modra wrote:
> Do you have a testcase we could add to the binutils testsuite?

I just added a check [1] to
objcopy.exp:keep_debug_symbols_and_test_copy(), but unfortunately it is
not testing anything:

It seems that --only-keep-debug is broken for Linux kernel .ko modules,
but it works just fine on tmpdir/striprog (built from testprog.c) used
by this testsuite...

I will take another look tomorrow and try to figure out what is so
special about Linux modules, then come up with a better test.  Maybe we
should also mention this in the commit message of the fix.

Thanks,
Peilin Ye
<peilin.ye@bytedance.com>

[1]

diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index bb80c1f0b554..1ad90a776c48 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -890,6 +890,8 @@ proc strip_executable_with_saving_a_symbol { prog flags test1 test2 } {
 # Test keeping only debug symbols of an executable

 proc keep_debug_symbols_and_test_copy { prog1 flags1 test1 prog2 flags2 test2 } {
+    global READELF
+
     remote_file build delete tmpdir/striprog
     remote_download build tmpdir/copyprog tmpdir/striprog
     if [is_remote host] {
@@ -898,12 +900,45 @@ proc keep_debug_symbols_and_test_copy { prog1 flags1 test1 prog2 flags2 test2 }
 	set copyfile tmpdir/striprog
     }

+    set non_debug_sections {}
+    if [is_elf_format] {
+        set got [binutils_run $READELF "-S --wide ${copyfile}"]
+        while {[regexp \
+               {[^a-zA-Z]+([a-zA-Z0-9_\.]+)[ \t]+([A-Z]+)[ \t]+[0-9a-f]+ [0-9a-f]+ [0-9a-f]+ [0-9]+[ \t]+([A-Z]*)[ \t]+[0-9]+(.*)} \
+               $got all name type flag rest]} {
+            if {$type != "NOTE" && [regexp {[AG]} $flag]} {
+                lappend non_debug_sections $name
+            }
+            set got $rest
+        }
+    }
+
     set exec_output [binutils_run $prog1 "$flags1 ${copyfile}"]
     if ![string equal "" $exec_output] {
 	fail $test1
 	return
     }
-    pass $test1
+
+    set fails 0
+    if [is_elf_format] {
+        set got [binutils_run $READELF "-S --wide ${copyfile}"]
+        while {[regexp \
+               {[^a-zA-Z]+([a-zA-Z0-9_\.]+)[ \t]+([A-Z]+)[ \t]+[0-9a-f]+ [0-9a-f]+ [0-9a-f]+ [0-9]+[ \t]+[A-Z]*[ \t]+[0-9]+(.*)} \
+               $got all name type rest]} {
+            if {[lsearch -exact $non_debug_sections $name] >= 0 && $type != "NOBITS"} {
+                set fails 1
+                send_log "Expected SHT_NOBITS type for .$name\n"
+            }
+            set got $rest
+        }
+    }
+
+    if {$fails == 0} {
+        pass $test1
+    } else {
+        fail $test1
+        return
+    }

     set exec_output [binutils_run $prog2 "$flags2 ${copyfile}"]
     if ![string equal "" $exec_output] {

Re: [RFC PATCH] bfd: Handle objcopy --only-keep-debug in _bfd_elf_init_private_section_data()

[Peilin Ye]

On Sat, Jan 29, 2022 at 01:15:40AM -0800, Peilin Ye wrote:
> It seems that --only-keep-debug is broken for Linux kernel .ko modules,
> but it works just fine on tmpdir/striprog (built from testprog.c) used
> by this testsuite...

I figured this out; it's because --only-keep-debug is only broken for
ELF relocatables.

For dynamically linked and statically linked ELF executables, their
non-debug output sections' type is changed 3 times:

  1. In setup_section() "make_nobits", set to SHT_NOBITS;
  2. Then in _bfd_elf_init_private_section_data(), restored back to input
     section's type, as we already discussed;
  3. Finally in assign_file_positions_for_load_sections(), they are set
     once again to SHT_NOBITS:

	for (i = 0; i < m->count; i++)
		if ((m->sections[i]->flags & (SEC_LOAD | SEC_HAS_CONTENTS)) == 0)
	      	/* If we aren't making room for this section, then
		   it must be SHT_NOBITS regardless of what we've
		   set via struct bfd_elf_special_section.  */
			elf_section_type (m->sections[i]) = SHT_NOBITS;
							    ^^^^^^^^^^

For a complete call graph:

copy_object()
  [...]
    setup_section()				/* 1. set to SHT_NOBITS */
      bfd_copy_private_section_data()
        _bfd_elf_copy_private_section_data()
          _bfd_elf_init_private_section_data()	/* 2. restore to ISEC's type */
  [...]
    copy_section()
      bfd_set_section_contents()
        _bfd_elf_set_section_contents()
          _bfd_elf_compute_section_file_positions()
            assign_file_positions_except_relocs()
              if (EXEC_P or DYNAMIC)		/* ELF executables? e.g. tmpdir/striprog */
                assign_file_positions_for_load_sections()	/* 3. set again to SHT_NOBITS */
              else				/* ELF relocatables? e.g. Linux modules */

Frankly, I don't semantically understand what this code is trying to do
here yet.  Since an ELF relocatable is neither EXEC_P nor DYNAMIC,
assign_file_positions_for_load_sections() is not called, therefore its
non-debug OSECs remain the old (ISEC's) type.

In conclusion, I think currently --only-keep-debug is only broken for
ELF relocatables, although I'm not 100% sure.

I will add this information in v3's commit message, and come up with a
better testsuite (probably test this on a .o file or something).

Thanks,
Peilin Ye
<peilin.ye@bytedance.com>

[PATCH v3 1/2] objcopy: Fix --only-keep-debug for ELF relocatables

[Peilin Ye]

From: Peilin Ye <peilin.ye@bytedance.com>

objcopy's --only-keep-debug option has been broken for ELF relocatables
since commit 8c803a2dd7d3 ("elf_backend_section_flags and
_bfd_elf_init_private_section_data"):

  1. binutils/objcopy.c:setup_section() marks non-debug OSECs as
     SHT_NOBITS, then calls bfd_copy_private_section_data(), see
     "mark_nobits";
  2. If ISEC and OSEC share the same section flags,
     bfd/elf.c:_bfd_elf_init_private_section_data() restores OSEC's
     section type back to ISEC's section type, effectively undoing
     "mark_nobits".

This makes objcopy's --only-keep-debug a no-op for ELF relocatables.  As
an example, Linux's scripts/package/builddeb script depends on this
option to generate debug-info-only kernel (.ko) modules.

As suggested by Nick and Alan, fix --only-keep-debug by marking
non-debug OSECs as SHT_NOBITS after calling
bfd_copy_private_section_data() in setup_section().

binutils/ChangeLog:
	* objcopy.c (setup_section): For --only-keep-debug, mark
	  non-debug output sections as SHT_NOBITS after calling
	  bfd_copy_private_section_data()

Reported-by: Omar Sandoval <osandov@osandov.com>
Fixes: 8c803a2dd7d3 ("elf_backend_section_flags and _bfd_elf_init_private_section_data")
Suggested-by: Alan Modra <amodra@gmail.com>
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
change since v2:
    - mention ELF relocatables in commit message

change since v1:
	- fix objcopy instead of touching _bfd_elf_init_private_section_data()
	  (suggested by Nick and Alan)

 binutils/objcopy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index d16d8ee67e4d..d53aa5c6000f 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4085,9 +4085,6 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
       goto loser;
     }
 
-  if (make_nobits)
-    elf_section_type (osection) = SHT_NOBITS;
-
   size = bfd_section_size (isection);
   size = bfd_convert_section_size (ibfd, isection, obfd, size);
   if (copy_byte >= 0)
@@ -4181,6 +4178,9 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
       goto loser;
     }
 
+  if (make_nobits)
+    elf_section_type (osection) = SHT_NOBITS;
+
   /* All went well.  */
   return;
 
-- 
2.20.1

[PATCH v3 2/2] binutils/testsuite: Test --only-keep-debug for ELF relocatables

[Peilin Ye]

From: Peilin Ye <peilin.ye@bytedance.com>

Recently we fixed a bug in objcopy's --only-keep-debug option for ELF
relocatables.  Add a test for it.

binutils/ChangeLog:
    * testsuite/binutils-all/objcopy.exp
      (keep_debug_symbols_for_elf_relocatable): Test --only-keep-debug
      marks non-debug output sections as NOBITS for ELF relocatables

Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
Hi all,

Please review, thanks!

 binutils/testsuite/binutils-all/objcopy.exp | 57 +++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index bb80c1f0b554..3dbb512b69cf 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -967,6 +967,60 @@ proc keep_debug_symbols_and_check_links { prog flags test } {
     }
 }
 
+# For ELF relocatables, test that --only-keep-debug
+# marks non-debug output sections as NOBITS.
+
+proc keep_debug_symbols_for_elf_relocatable { prog flags test } {
+    global srcdir
+    global subdir
+    global READELF
+
+    if { [target_compile $srcdir/$subdir/testprog.c tmpdir/testprog.o object debug] != "" } {
+        untested $test
+        return
+    }
+
+    if [is_remote host] {
+        set relocatable [remote_download host tmpdir/testprog.o]
+    } else {
+        set relocatable tmpdir/testprog.o
+    }
+
+    set non_debug_sections {}
+    set got [binutils_run $READELF "-S --wide ${relocatable}"]
+    while { [regexp \
+            {[^a-zA-Z]+([a-zA-Z0-9_\.]+)[ \t]+([A-Z]+)[ \t]+[0-9a-f]+ [0-9a-f]+ [0-9a-f]+ [0-9]+[ \t]+([A-Z]*)[ \t]+[0-9]+(.*)} \
+            $got all name type flag rest] } {
+        if { $type != "NOTE" && [regexp {[AG]} $flag] } {
+            lappend non_debug_sections $name
+        }
+        set got $rest
+    }
+
+    set exec_output [binutils_run $prog "$flags ${relocatable}"]
+    if ![string equal "" $exec_output] {
+        fail $test
+        return
+    }
+
+    set fails 0
+    set got [binutils_run $READELF "-S --wide ${relocatable}"]
+    while { [regexp \
+            {[^a-zA-Z]+([a-zA-Z0-9_\.]+)[ \t]+([A-Z]+)[ \t]+[0-9a-f]+ [0-9a-f]+ [0-9a-f]+ [0-9]+[ \t]+[A-Z]*[ \t]+[0-9]+(.*)} \
+            $got all name type rest] } {
+        if { [lsearch -exact $non_debug_sections $name] >= 0 && $type != "NOBITS" } {
+            set fails 1
+            send_log "Expected SHT_NOBITS type for .$name\n"
+        }
+        set got $rest
+    }
+    if { $fails == 1 } {
+        fail $test
+        return
+    }
+
+    pass $test
+}
 
 set test1 "simple objcopy of executable"
 set test1r "run objcopy of executable"
@@ -978,6 +1032,7 @@ set test4 "keep only debug data"
 set test5 "simple objcopy of debug data"
 if [is_elf_format] {
     set test6 "NOBITS sections retain sh_link field"
+    set test7 "--only-keep-debug for ELF relocatables"
 }
 
 switch [copy_setup] {
@@ -1008,6 +1063,7 @@ switch [copy_setup] {
 					 "$OBJCOPY" "$OBJCOPYFLAGS" "$test5"
 	if [is_elf_format] {
 	    keep_debug_symbols_and_check_links "$STRIP" "--only-keep-debug $STRIPFLAGS" "$test6"
+	    keep_debug_symbols_for_elf_relocatable "$STRIP" "--only-keep-debug $STRIPFLAGS" "$test7"
 	}
     }
     "0" {
@@ -1018,6 +1074,7 @@ switch [copy_setup] {
 					 "$OBJCOPY" "$OBJCOPYFLAGS" "$test5"
 	if [is_elf_format] {
 	    keep_debug_symbols_and_check_links "$STRIP" "--only-keep-debug $STRIPFLAGS" "$test6"
+	    keep_debug_symbols_for_elf_relocatable "$STRIP" "--only-keep-debug $STRIPFLAGS" "$test7"
 	}
     }
 }
-- 
2.20.1