Bug 12001 - Linker includes archive members when symbols therein have already been defined
Summary: Linker includes archive members when symbols therein have already been defined
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.21
: P2 normal
Target Milestone: ---
Assignee: Nick Clifton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-09 20:55 UTC by Mark Mitchell
Modified: 2011-01-02 16:04 UTC (History)
3 users (show)

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

Attachments
Add early definition of --defsym symbols. (1.11 KB, patch)
2010-10-28 13:42 UTC, Nick Clifton 		Details | Diff
scan pending assignments before deciding to include an archive member (959 bytes, patch)
2010-11-02 08:38 UTC, Nick Clifton 		Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Mitchell 2010-09-09 20:55:28 UTC 
In this scenario:

f.c
===
void f() {}

main.c
======
int main () { f(); }

$ gcc -c f.c
$ ar cr libf.a f.o
$ gcc main.c -Wl,--defsym=f=4 -L. -lf

the linker pulls in f.o from libf.a, even though f has been explicitly defined
on the command line.

As described here:

http://sourceware.org/ml/binutils/2010-09/msg00110.html

the consensus is that this is a defect in GNU LD and in Gold.
Comment 1 Nick Clifton 2010-10-28 13:42:29 UTC 
Created attachment 5095 [details]
Add early definition of --defsym symbols.
Comment 2 Nick Clifton 2010-10-28 13:47:43 UTC 
Hi Mark,

  I have uploaded a possible patch to LD for this problem.  I assume that a similar patch to GOLD will also be possible, but I have not looked into this yet.  

  In the course of checking the patch I found one linker testcase that broke: default-script2.  Looking at the test, it seems to me that the patched linker is actually correct in this instance.  The test uses a linker script that looks like this:

  _START = DEFINED(_START) ? _START : 0x9000000;
  SECTIONS
  {
    . = _START;
    .text : {*(.text)}
    /DISCARD/ : {*(*)}
  }

It then checks to see that the .text symbol has been given an address of 0x90000000 despite the fact that "-defsym _START=0x8000000" has been passed on the command line.  The patched linker sets the address to 0x80000000, which I think is right.

  Does anyone have any comments before I look at creating a patch for GOLD ?

Cheers
  Nick
Comment 3 Mark Mitchell 2010-10-28 15:27:07 UTC 
On 10/28/2010 8:48 AM, nickc at redhat dot com wrote:

>   In the course of checking the patch I found one linker testcase that broke:
> default-script2.

I agree.

I'm disturbed that there's any evidence that people were relying on the
old behavior, but it just seems wrong:

This:

>   _START = DEFINED(_START) ? _START : 0x9000000;

in combination with --defsym _START=... should result in a defined
_START.  IMO, the most logical interpretation of --defsym is that the
linker operate "as if" such definitions occurred at the top of the
linker script, in the order they appeared on the command line, so that
the script is actually:

  _START = 0x80000000;
  _START = DEFINED(_START) ? _START : 0x9000000;

And, in that case, of course, _START is defined.
Comment 4 Nick Clifton 2010-11-02 08:38:21 UTC 
Created attachment 5104 [details]
scan pending assignments before deciding to include an archive member
Comment 5 Nick Clifton 2010-11-02 08:39:26 UTC 
Hi Guys,

  OK, I have uploaded a patch for GOLD as well.  Assuming that nobody has any objections to either patch I plan to apply them at the end of the week.

Cheers
  Nick
Comment 6 Ian Lance Taylor 2010-11-02 13:15:43 UTC 
The gold patch is OK except that the new name() function should return "const string&" rather than "string".

Thanks.
Comment 7 Nick Clifton 2010-11-03 17:19:18 UTC 
Hi Mark,

  I have checked both patches in, so the fix should appear in the next release of the binutils.

Cheers
  Nick
Comment 8 Mark Mitchell 2010-11-03 17:47:36 UTC 
Nick --

Thanks for fixing this!

-- Mark
Comment 9 Sourceware Commits 2010-12-20 06:27:15 UTC 
CVSROOT:	/cvs/src
Module name:	src
Changes by:	amodra@sourceware.org	2010-12-20 06:27:11

Modified files:
	ld             : ChangeLog ldexp.c ldexp.h ldlang.c 
	ld/testsuite   : ChangeLog 
	ld/testsuite/ld-scripts: default-script2.d 

Log message:
	PR ld/12001
	Revert 2010-11-03 Nick Clifton
	* ldlang.c (ldlang_def_chain_list): Delete.
	(insert_defined, ldlang_add_def, lang_place_defineds): Delete.
	(lang_process): Don't call lang_place_defineds.
	(lang_add_assignment): Don't do anything special for --defsym.
	
	* ldexp.h (struct ldexp_control): Add uses_defined.
	(exp_fold_tree_no_dot): Declare.
	* ldexp.c (exp_fold_tree): Clear uses_defined.
	(exp_fold_tree_no_dot): Likewise.  Make global.
	(fold_name <DEFINED>): Set uses_defined.
	(exp_fold_tree_1 <etree_assign>): Define symbol during first phase
	even when the value being assigned isn't valid.
	* ldlang.c (open_input_bfds): Process assignment statements.
	(lang_process): Bump lang_statement_iteration.
	(scan_for_self_assignment): Formatting.
	(print_assignment): Style.
	
	testsuite/
	* ld-scripts/default-script2.d: Revert 2010-11-03 change.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/ld/ChangeLog.diff?cvsroot=src&r1=1.2256&r2=1.2257
http://sourceware.org/cgi-bin/cvsweb.cgi/src/ld/ldexp.c.diff?cvsroot=src&r1=1.87&r2=1.88
http://sourceware.org/cgi-bin/cvsweb.cgi/src/ld/ldexp.h.diff?cvsroot=src&r1=1.23&r2=1.24
http://sourceware.org/cgi-bin/cvsweb.cgi/src/ld/ldlang.c.diff?cvsroot=src&r1=1.353&r2=1.354
http://sourceware.org/cgi-bin/cvsweb.cgi/src/ld/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1338&r2=1.1339
http://sourceware.org/cgi-bin/cvsweb.cgi/src/ld/testsuite/ld-scripts/default-script2.d.diff?cvsroot=src&r1=1.4&r2=1.5