Bug 12804 - Incremental link tests failed
Summary: Incremental link tests failed
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gold (show other bugs)
Version: 2.22
: P2 normal
Target Milestone: ---
Assignee: Cary Coutant
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-25 13:13 UTC by H.J. Lu
Modified: 2011-06-10 15:33 UTC (History)
1 user (show)

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


Attachments
Input binaries (20.32 KB, application/octet-stream)
2011-06-08 21:15 UTC, H.J. Lu
Details
Fix test case so "v2" is in .data section before and after incremental update (299 bytes, patch)
2011-06-09 00:51 UTC, Cary Coutant
Details | Diff
New input binaries (20.34 KB, application/octet-stream)
2011-06-09 12:53 UTC, H.J. Lu
Details
Fix to record uncompressed size of input sections when doing incremental link (767 bytes, patch)
2011-06-09 18:01 UTC, Cary Coutant
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description H.J. Lu 2011-05-25 13:13:18 UTC
On Linux/x86-64, I got

make[7]: *** [incremental_test_2] Error 1
make[7]: *** [incremental_test_3] Error 1
make[7]: *** [incremental_test_4] Error 1
make[6]: *** [check-am] Error 2

Linux/ia32 seems OK.
Comment 1 Cary Coutant 2011-05-25 17:47:37 UTC
Can you attach the file testsuite/test-suite.log? What version of gcc are you configured with?
Comment 2 H.J. Lu 2011-05-25 18:29:53 UTC
(In reply to comment #1)
> Can you attach the file testsuite/test-suite.log? What version of gcc are you
> configured with?

[hjl@gnu-6 gold]$ find -name "*.log"
./bootstrap-test.log
./config.log
./test-suite.log
./bootstrap-test-r.log
[hjl@gnu-6 gold]$ cat ./test-suite.log
================================
   gold 0.1: ./test-suite.log   
================================

All 2 tests passed.  

.. contents:: :depth: 2

[hjl@gnu-6 gold]$ 

g++ -W -Wall    -Werror -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -fmerge-constants -g -O2   -o gc_comdat_test -Bgcctestdir/ -Wl,--gc-sections gc_comdat_test_1.o gc_comdat_test_2.o
gcctestdir/ld: error: two_file_test_2.o: multiple definition of 't1_2()'
gcctestdir/ld: two_file_test_tmp.o: previous definition here
gcctestdir/ld: error: two_file_test_2.o: multiple definition of 't1a()'
gcctestdir/ld: two_file_test_tmp.o: previous definition here
gcctestdir/ld: error: two_file_test_2.o: multiple definition of 'f10()'
gcctestdir/ld: two_file_test_tmp.o: previous definition here
gcctestdir/ld: error: two_file_test_2.o: multiple definition of 'f11b(int (*)())'
gcctestdir/ld: two_file_test_tmp.o: previous definition here
gcctestdir/ld: error: two_file_test_2.o: multiple definition of 'f12()'
gcctestdir/ld: two_file_test_tmp.o: previous definition here
gcctestdir/ld: error: two_file_test_2.o: multiple definition of 'f13()'
gcctestdir/ld: two_file_test_tmp.o: previous definition here
gcctestdir/ld: error: two_file_test_2.og++ -W -Wall    -Werror -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -fmerge-constants -g -O2   -o gc_tls_test -Bgcctestdir/ -Wl,--gc-sections gc_tls_test.o
: multiple definition of 'f14()'
....
I am using gcc 4.5 from Fedora 14.
Comment 3 Cary Coutant 2011-05-25 19:01:19 UTC
OK, I think the problem is that I'm out of patch space because some small COMDAT functions are being included incorrectly. COMDAT handling is incremental patch 16, which is next up, and I think that'll fix this.

Having some extra patch space added to the output file during --incremental-full would also have fixed this, but it really would have masked the problem rather than fixed it. That's one reason I'm not yet adding extra patch space. That's in patch 22, the next to be posted.
Comment 4 Cary Coutant 2011-05-26 18:53:13 UTC
HJ, now that patch 16 is committed, can you check to see if these failures are gone? Before the patch, in my configuration (gcc 4.4), the tests were clean at -O0 -g, but incremental_test_4 wouldn't build at -O2 because of the "out of patch space" error due to not eliminating COMDATs properly. After patch 16, I get clean testsuite runs in both modes.

If your configuration still has problems, it's probably simplest for me to disable those tests until patch 22 (add extra patch space) goes in.
Comment 5 H.J. Lu 2011-05-27 11:55:39 UTC
I still got

g++ -W -Wall    -Werror -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -fmerge-constants -g -O2   -o incremental_test_4 -Wl,--incremental-full -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp.o two_file_test_main.o
gcctestdir/ld: error: two_file_test_2.o: multiple definition of 't1_2()'
gcctestdir/ld: two_file_test_tmp.o: previous definition here
gcctestdir/ld: error: two_file_test_2.o: multiple definition of 't1a()'
gcctestdir/ld: two_file_test_tmp.o: previous definition here
gcctestdir/ld: error: two_file_test_2.o: multiple definition of 'f10()'
gcctestdir/ld: two_file_test_tmp.o: previous definition here
....
Comment 6 Cary Coutant 2011-05-27 17:39:43 UTC
(In reply to comment #5)
> I still got
> 
> g++ -W -Wall    -Werror -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
> -fmerge-constants -g -O2   -o incremental_test_4 -Wl,--incremental-full
> -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp.o
> two_file_test_main.o
> gcctestdir/ld: error: two_file_test_2.o: multiple definition of 't1_2()'
> gcctestdir/ld: two_file_test_tmp.o: previous definition here
> gcctestdir/ld: error: two_file_test_2.o: multiple definition of 't1a()'
> gcctestdir/ld: two_file_test_tmp.o: previous definition here
> gcctestdir/ld: error: two_file_test_2.o: multiple definition of 'f10()'
> gcctestdir/ld: two_file_test_tmp.o: previous definition here
> ....

This doesn't make sense to me -- two_file_test_2.o isn't mentioned on the link line at all, and the linker shouldn't be looking at it. At this step in the Makefile rule, two_file_test_tmp.o should be a copy of two_file_test_2_v1.o, so it does make sense that two_file_test_2.o would have duplicate symbols, but I don't see how it's being linked given that command.

I thought maybe I forgot to check in the regenerated testsuite/Makefile.in, but I checked and it looks OK in cvs. Did your build somehow skip regenerating testsuite/Makefile?

-cary
Comment 7 H.J. Lu 2011-05-27 20:09:13 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > I still got
> > 
> > g++ -W -Wall    -Werror -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
> > -fmerge-constants -g -O2   -o incremental_test_4 -Wl,--incremental-full
> > -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp.o
> > two_file_test_main.o
> > gcctestdir/ld: error: two_file_test_2.o: multiple definition of 't1_2()'
> > gcctestdir/ld: two_file_test_tmp.o: previous definition here
> > gcctestdir/ld: error: two_file_test_2.o: multiple definition of 't1a()'
> > gcctestdir/ld: two_file_test_tmp.o: previous definition here
> > gcctestdir/ld: error: two_file_test_2.o: multiple definition of 'f10()'
> > gcctestdir/ld: two_file_test_tmp.o: previous definition here
> > ....
> 
> This doesn't make sense to me -- two_file_test_2.o isn't mentioned on the link
> line at all, and the linker shouldn't be looking at it. At this step in the
> Makefile rule, two_file_test_tmp.o should be a copy of two_file_test_2_v1.o, so
> it does make sense that two_file_test_2.o would have duplicate symbols, but I
> don't see how it's being linked given that command.
> 
> I thought maybe I forgot to check in the regenerated testsuite/Makefile.in, but
> I checked and it looks OK in cvs. Did your build somehow skip regenerating
> testsuite/Makefile?
> 

I didn't do anything special. Can you try to build binutils
with GCC 4.5 on Linux/x86-64?
Comment 8 cvs-commit@gcc.gnu.org 2011-05-29 17:24:09 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	ian@sourceware.org	2011-05-29 17:24:05

Modified files:
	gold           : ChangeLog 
	gold/testsuite : Makefile.am Makefile.in 

Log message:
	PR gold/12804
	* testsuite/Makefile.am: Use different file name for two_file_test
	temporary file for each incremental test.
	* testsuite/Makefile.in: Rebuild.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gold/ChangeLog.diff?cvsroot=src&r1=1.740&r2=1.741
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gold/testsuite/Makefile.am.diff?cvsroot=src&r1=1.160&r2=1.161
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gold/testsuite/Makefile.in.diff?cvsroot=src&r1=1.169&r2=1.170
Comment 9 Ian Lance Taylor 2011-05-29 17:26:48 UTC
The problem comes from running "make -j check".  It should now be fixed.  If you have an existing build directory, it may be necessary to remove gold/testsuite/two* and gold/testsuite/incremental* in order for the files to be rebuilt correctly.
Comment 10 H.J. Lu 2011-05-30 21:20:49 UTC
I still got

gcctestdir/ld: fatal error: out of patch space; relink with --incremental-full
collect2: ld returned 1 exit status
make[7]: *** [incremental_test_4] Error 1

with "make -j8 check" on Intel Core i7 870/64bit Fedora 14.
Comment 11 Ian Lance Taylor 2011-05-31 00:05:06 UTC
You're right, there is still a problem.  For me it works fine the first time I run the testsuite.  However, if I "touch gold/ld-new" and run "make check" again, it fails.
Comment 12 Ian Lance Taylor 2011-05-31 04:25:29 UTC
Sorry, I was testing with an old tree.  I actually do not see a problem any more.  H.J., do you see a problem in a fresh build directory?
Comment 13 H.J. Lu 2011-05-31 04:48:31 UTC
(In reply to comment #12)
> Sorry, I was testing with an old tree.  I actually do not see a problem any
> more.  H.J., do you see a problem in a fresh build directory?

Yes, that is in a fresh build directory on x86-64 Fedora 14.
Comment 14 H.J. Lu 2011-06-08 13:00:01 UTC
On Fedora/15, I still got

g++ -W -Wall    -Werror -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -fmerge-constants -g -O2   -o incremental_test_4 -Wl,--incremental-update,--incremental-base=incremental_test_4.base -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp_4.o two_file_test_main.o
gcctestdir/ld: fatal error: out of patch space; relink with --incremental-full
collect2: ld returned 2 exit status
Comment 15 Cary Coutant 2011-06-08 17:18:20 UTC
> g++ -W -Wall    -Werror -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
> -fmerge-constants -g -O2   -o incremental_test_4
> -Wl,--incremental-update,--incremental-base=incremental_test_4.base
> -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp_4.o
> two_file_test_main.o
> gcctestdir/ld: fatal error: out of patch space; relink with --incremental-full
> collect2: ld returned 2 exit status

Do you have the object files incremental_test_4.base,
two_file_test_1.o, two_file_test_1b.o, two_file_test_tmp_4.o, and
two_file_test_main.o still lying around? Can you send them to me or
attach them to this PR?

-cary
Comment 16 H.J. Lu 2011-06-08 21:15:28 UTC
Created attachment 5774 [details]
Input binaries
Comment 17 Cary Coutant 2011-06-09 00:51:02 UTC
Created attachment 5775 [details]
Fix test case so "v2" is in .data section before and after incremental update

I think this is the problem: In two_file_test_2_v1.cc, I changed the initialization for v2 from 456 to 0, which moved it from .data to .bss. As a result, the incremental update didn't have room in .data when it tried to link in two_file_test-2.cc as a replacement, where v2 is in .data. This patch changes the initial value to 1, which keeps it in .data.
Comment 18 cvs-commit@gcc.gnu.org 2011-06-09 00:51:42 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	ccoutant@sourceware.org	2011-06-09 00:51:39

Modified files:
	gold           : ChangeLog 
	gold/testsuite : two_file_test_2_v1.cc 

Log message:
	PR gold/12804
	* testsuite/two_file_test_2_v1.cc: Change initialization of
	v2 to keep it in .data.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gold/ChangeLog.diff?cvsroot=src&r1=1.750&r2=1.751
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gold/testsuite/two_file_test_2_v1.cc.diff?cvsroot=src&r1=1.1&r2=1.2
Comment 19 H.J. Lu 2011-06-09 12:53:44 UTC
Created attachment 5777 [details]
New input binaries

It still doesn't work.
Comment 20 Cary Coutant 2011-06-09 18:01:07 UTC
Created attachment 5781 [details]
Fix to record uncompressed size of input sections when doing incremental link

With compressed debug sections in the input file, the linker was storing the compressed size of the section in the incremental info instead of the uncompressed size. This patch fixes that, and also adds a check to disallow the use of the linker's --compress-debug-sections option when doing incremental linking.
Comment 21 cvs-commit@gcc.gnu.org 2011-06-09 18:18:47 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	ccoutant@sourceware.org	2011-06-09 18:18:44

Modified files:
	gold           : ChangeLog gold.cc object.cc 

Log message:
	PR gold/12804
	* gold/gold.cc (queue_initial_tasks): Warn if --incremental is
	used with --compress-debug-sections.
	* gold/object.cc (Sized_relobj_file::do_layout): Report
	uncompressed size of compressed input sections.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gold/ChangeLog.diff?cvsroot=src&r1=1.751&r2=1.752
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gold/gold.cc.diff?cvsroot=src&r1=1.92&r2=1.93
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gold/object.cc.diff?cvsroot=src&r1=1.141&r2=1.142
Comment 22 H.J. Lu 2011-06-10 15:33:29 UTC
Fixed.