Bug 11133 - gc-sections deletes sections with __start/__stop reference
Summary: gc-sections deletes sections with __start/__stop reference
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.21
: P2 normal
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
: 3400 (view as bug list)
Depends on: 27495
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-05 01:23 UTC by Sriraman Tallam
Modified: 2021-03-01 19:19 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sriraman Tallam 2010-01-05 01:23:49 UTC
Here is how to reproduce. Using ld with binutils 2.19.

$ cat hello.cc
#include <stdio.h>
int main()
{
  printf("Hello World\n");
}
$ gcc-4.4.0 hello.cc -static -Wl,--gc-sections
$./a.out > hello.out
$ wc hello.out
0 0 0 hello.out

Using, --print-gc-sections shows that :

ld: removing unused section from '__libc_atexit' in file 'libc.a(genops.o)'
ld: removing unused section from '__libc_freeres_fn' in file
'libc.a(register-atfork.o)'
ld: removing unused section from '__libc_subfreeres' in file
'libc.a(register-atfork.o)'
ld: removing unused section from '__libc_freeres_fn' in file 'libc.a(dcigettext.o)'
...

Removing __libc_atexit was responsible for this behaviour.
Comment 1 H.J. Lu 2010-01-05 15:48:21 UTC
Both ld and gold fail with

---
[hjl@gnu-6 pr11133]$ cat test.c
struct obs_kernel_param {
  long a;
  long b;
  int c;
};

extern struct obs_kernel_param const __start__init_setup[];

#define __setup_param(A, B, C) \
  struct obs_kernel_param __setup_##A \
    __attribute__((__section__("_init_setup"))) = { A, B, C }

__setup_param(0x11, 0x2, 0x3);

int
_start ()
{
  return __start__init_setup != (struct obs_kernel_param *) 1;
}
[hjl@gnu-6 pr11133]$ make
cc    -c -o test.o test.c
./ld -o bar test.o
./ld --gc-sections -o foo test.o
test.o: In function `_start':
test.c:(.text+0x5): undefined reference to `__start__init_setup'
make: *** [foo] Error 1
[hjl@gnu-6 pr11133]$ 
---
Comment 2 H.J. Lu 2010-01-05 16:43:24 UTC
A smaller testcase:

---
[hjl@gnu-6 pr11133]$ cat test.c
extern const int *__start__foo;
int foo __attribute__((__section__("_foo"))) = 1;
int
_start ()
{
  return *__start__foo;
}
[hjl@gnu-6 pr11133]$ make
cc    -c -o test.o test.c
./ld -e _start -o bar test.o
./ld -e _start --gc-sections -o foo test.o
test.o: In function `_start':
test.c:(.text+0x7): undefined reference to `__start__foo'
make: *** [foo] Error 1
[hjl@gnu-6 pr11133]$ 
---
Comment 3 H.J. Lu 2010-01-05 17:32:54 UTC
A patch is posted at

http://sourceware.org/ml/binutils/2010-01/msg00063.html
Comment 4 Alan Modra 2010-01-06 13:51:32 UTC
I think this is a glibc bug.  There isn't any good reason why a reference to a
__start_section/__stop_section symbol in an output section should affect garbage
collection of input sections, except of course that it works around this glibc
--gc-sections problem.  I can imagine other situations where a user has a
reference to __start_section but wants the current linker behaviour.
Comment 5 Ian Lance Taylor 2010-01-06 14:06:06 UTC
At first glance I'm not sure I agree.  __start_section is special already
because it only works when the section has a special name.  Can you sketch out a
case where a program uses __start_section but still wants the individual input
sections to be garbage collected?
Comment 6 Mike Frysinger 2010-01-07 06:44:29 UTC
drepper is aware of the errors in glibc but is not interested in fixing it.  see
bug 3400.
Comment 7 Alan Modra 2010-01-07 11:23:42 UTC
Imagine someone who wants an array of functions (perhaps constructor like) or
some other object used in their app.  They set up weak references in a named
section, then access the array using the __start_* symbol.  The array itself
isn't referenced so references there don't cause the functions to be kept.

Oh..  My array won't be kept either.  I didn't think of that earlier..  Hmm, the
named section would need to be a debug section, and then it doesn't matter if
there is a change in the way __start_* affects --gc-sections.

So my concern that HJ's idea might affect some existing app is probably not
valid.  I still don't particularly like a reference to __start_somesection
meaning that we should keep all "somesection" input sections.  It would be
better if there were a more explicit way to mark input sections.  I also note
that HJ's patch is incorrect since marking a section with gc_mark and returning
NULL from gc_mark_hook will result in no mark phase recursion for that section.
Comment 8 H.J. Lu 2010-01-07 13:36:59 UTC
*** Bug 3400 has been marked as a duplicate of this bug. ***
Comment 9 H.J. Lu 2010-01-07 13:47:50 UTC
(In reply to comment #7)
> Imagine someone who wants an array of functions (perhaps constructor like) or
> some other object used in their app.  They set up weak references in a named
> section, then access the array using the __start_* symbol.  The array itself
> isn't referenced so references there don't cause the functions to be kept.
> Oh..  My array won't be kept either.  I didn't think of that earlier..  Hmm, the
> named section would need to be a debug section, and then it doesn't matter if
> there is a change in the way __start_* affects --gc-sections.
> 
> So my concern that HJ's idea might affect some existing app is probably not
> valid.  I still don't particularly like a reference to __start_somesection
> meaning that we should keep all "somesection" input sections.  It would be
> better if there were a more explicit way to mark input sections.  I also note
> that HJ's patch is incorrect since marking a section with gc_mark and returning
> NULL from gc_mark_hook will result in no mark phase recursion for that section.

I will try a different approach.

Comment 10 H.J. Lu 2010-01-07 14:03:42 UTC
(In reply to comment #7)
> 
> So my concern that HJ's idea might affect some existing app is probably not
> valid.  I still don't particularly like a reference to __start_somesection
> meaning that we should keep all "somesection" input sections.  It would be

Isn't it this bug all about?

> better if there were a more explicit way to mark input sections.  I also note
> that HJ's patch is incorrect since marking a section with gc_mark and returning
> NULL from gc_mark_hook will result in no mark phase recursion for that section.
> 

The updated patch is posted at

http://sourceware.org/ml/binutils/2010-01/msg00137.html
Comment 11 Sourceware Commits 2010-01-08 01:43:35 UTC
Subject: Bug 11133

CVSROOT:	/cvs/src
Module name:	src
Changes by:	hjl@sourceware.org	2010-01-08 01:43:23

Modified files:
	bfd            : ChangeLog elflink.c 
	ld/testsuite   : ChangeLog 
	ld/testsuite/ld-gc: gc.exp 
Added files:
	ld/testsuite/ld-gc: start.d start.s 

Log message:
	Set SEC_KEEP on section XXX for undefined __start_XXX/__stop_XXX
	
	bfd/
	
	2010-01-07  H.J. Lu  <hongjiu.lu@intel.com>
	
	PR ld/11133
	* elflink.c (_bfd_elf_gc_mark_hook): Check section XXX for
	undefined __start_XXX/__stop_XXX in all input files and set
	SEC_KEEP.
	
	ld/testsuite/
	
	2010-01-07  H.J. Lu  <hongjiu.lu@intel.com>
	
	PR ld/11133
	* ld-gc/gc.exp: Run start.
	
	* ld-gc/start.d: New.
	* ld-gc/start.s: Likewise.

Patches:
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/bfd/ChangeLog.diff?cvsroot=src&r1=1.4875&r2=1.4876
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/bfd/elflink.c.diff?cvsroot=src&r1=1.363&r2=1.364
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/ld/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1202&r2=1.1203
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/ld/testsuite/ld-gc/start.d.diff?cvsroot=src&r1=NONE&r2=1.1
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/ld/testsuite/ld-gc/start.s.diff?cvsroot=src&r1=NONE&r2=1.1
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/ld/testsuite/ld-gc/gc.exp.diff?cvsroot=src&r1=1.7&r2=1.8

Comment 12 H.J. Lu 2010-01-08 01:45:32 UTC
Fixed.
Comment 13 Sourceware Commits 2010-01-08 05:55:25 UTC
Subject: Bug 11133

CVSROOT:	/cvs/src
Module name:	src
Changes by:	amodra@sourceware.org	2010-01-08 05:55:11

Modified files:
	bfd            : ChangeLog elf32-cr16.c elf32-microblaze.c 
	                 elf64-ppc.c 

Log message:
	PR ld/11133
	* elf32-cr16.c (elf32_cr16_gc_mark_hook): Call _bfd_elf_gc_mark_hook.
	* elf32-microblaze.c (microblaze_elf_gc_mark_hook): Likewise.
	* elf64-ppc.c (ppc64_elf_gc_mark_hook): Likewise.

Patches:
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/bfd/ChangeLog.diff?cvsroot=src&r1=1.4877&r2=1.4878
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/bfd/elf32-cr16.c.diff?cvsroot=src&r1=1.12&r2=1.13
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/bfd/elf32-microblaze.c.diff?cvsroot=src&r1=1.3&r2=1.4
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/bfd/elf64-ppc.c.diff?cvsroot=src&r1=1.306&r2=1.307

Comment 14 Sourceware Commits 2015-10-22 19:25:10 UTC
The master branch has been updated by H.J. Lu <hjl@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=bba037e0aef1f3b17cc6cf6fd041ed6110cc375a

commit bba037e0aef1f3b17cc6cf6fd041ed6110cc375a
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Thu Oct 22 12:17:43 2015 -0700

    Always keep sections marked with SEC_KEEP
    
    SEC_KEEP check in elf_gc_sweep was missing in commit:
    
    commit bde6f3eb6dff94cea1d471e15c6154d55d49820f
    Author: H.J. Lu <hjl.tools@gmail.com>
    Date:   Fri Jan 8 01:43:23 2010 +0000
    
        Set SEC_KEEP on section XXX for undefined __start_XXX/__stop_XXX
    
        bfd/
    
        2010-01-07  H.J. Lu  <hongjiu.lu@intel.com>
    
          PR ld/11133
          * elflink.c (_bfd_elf_gc_mark_hook): Check section XXX for
          undefined __start_XXX/__stop_XXX in all input files and set
          SEC_KEEP.
    
    This patch adds SEC_KEEP check to elf_gc_sweep.
    
    bfd/
    
    	PR ld/19161
    	* elflink.c (elf_gc_sweep): Always keep sections marked with
    	SEC_KEEP.
    
    ld/testsuite/
    
    	PR ld/19161
    	* ld-gc/gc.exp: Run pr19161 test.
    	* ld-gc/pr19161-1.c: New file.
    	* ld-gc/pr19161-2.c: Likewise.
    	* ld-gc/pr19161.d: Likewise.
Comment 15 Sourceware Commits 2015-10-22 23:53:32 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1e7eae0dcc0e5473dda573b30107ffdd501b0d73

commit 1e7eae0dcc0e5473dda573b30107ffdd501b0d73
Author: Alan Modra <amodra@gmail.com>
Date:   Fri Oct 23 10:14:51 2015 +1030

    Re: Always keep sections marked with SEC_KEEP
    
    Revert bba037e0, it breaks the gc-sections model.  Instead fix the
    underlying problem which is that _bfd_elf_gc_mark_hook is too late to
    be setting SEC_KEEP.
    
    	PR ld/11133
    	PR ld/19161
    	* elflink.c (elf_gc_sweep): Revert last patch.
    	(_bfd_elf_gc_mark_hook): Don't set SEC_KEEP here.
Comment 16 Sourceware Commits 2015-10-23 11:57:34 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1cce69b9dc8c58884c3cc4a8928fb234294e6886

commit 1cce69b9dc8c58884c3cc4a8928fb234294e6886
Author: Alan Modra <amodra@gmail.com>
Date:   Fri Oct 23 22:23:05 2015 +1030

    Handle __start_* and __stop_* symbols in --gc-sections
    
    	PR ld/11133
    	PR ld/19161
    	PR ld/19167
    	* elflink.c (_bfd_elf_gc_mark_hook): Delete code handling __start_*
    	and __stop_* symbol refs.
    	(_bfd_elf_gc_mark_rsec): Add start_stop parameter.  Handle __start_*
    	and __stop_* symbol refs here..
    	(_bfd_elf_gc_mark_reloc): ..and here.
    	* elf-bfd.h (_bfd_elf_gc_mark_hook): Update prototype.
    	* elf-eh-frame.c (_bfd_elf_parse_eh_frame): Update
    	_bfd_elf_gc_mark_rsec call.