Bug 64860 - [5/6 Regression] multiple definition of typeinfo in 5.0 (4.9.2 works)
Summary: [5/6 Regression] multiple definition of typeinfo in 5.0 (4.9.2 works)
Status: RESOLVED DUPLICATE of bug 67548
Alias: None
Product: gcc
Classification: Unclassified
Component: lto (show other bugs)
Version: 5.0
: P2 normal
Target Milestone: 5.3
Assignee: Jan Hubicka
Keywords: lto, wrong-code
Depends on:
Reported: 2015-01-29 15:09 UTC by Franz Sirl
Modified: 2015-11-26 03:16 UTC (History)
3 users (show)

See Also:
Known to work: 4.9.2
Known to fail:
Last reconfirmed: 2015-02-01 00:00:00

Simple testcase. Use "make" to reproduce. (618 bytes, application/x-gzip)
2015-01-29 15:09 UTC, Franz Sirl
partial patch (1.49 KB, patch)
2015-03-23 01:21 UTC, Jan Hubicka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Franz Sirl 2015-01-29 15:09:35 UTC
Created attachment 34617 [details]
Simple testcase. Use "make" to reproduce.

While trying to construct a testcase for a 4.8 to 4.9 change of LTO linking behaviour, I stumbled over this (just re-tested with r220248):

g++-5 -flto -fuse-linker-plugin -O2 -Wall -Wextra -c file1.cpp
g++-5 -flto -fuse-linker-plugin -O2 -Wall -Wextra -c file2.cpp
gcc-5  -flto -fuse-linker-plugin -Wl,-r -nostdlib -o lib1.lib file1.o
gcc-5  -flto -fuse-linker-plugin -Wl,-r -nostdlib -o lib2.lib file2.o
g++-5 -flto -fuse-linker-plugin -O2 -o testexe lib1.lib lib2.lib
lib2.lib:(.rodata+0x0): multiple definition of `typeinfo name for CDialogBase'
lib1.lib:(.rodata+0x0): first defined here
collect2: error: ld returned 1 exit status

# g++-5 -v
Using built-in specs.
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info --mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64 --enable-languages=c,c++,fortran --with-gxx-include-dir=/usr/include/c++/5 --enable-ssp --disable-libssp --disable-libvtv --disable-plugin --with-bugurl=http://bugs.opensuse.org/ --with-pkgversion='SUSE Linux' --disable-libgcj --with-slibdir=/lib64 --with-system-zlib --enable-__cxa_atexit --enable-libstdcxx-allocator=new --disable-libstdcxx-pch --with-default-libstdcxx-abi=c++98 --enable-version-specific-runtime-libs --enable-linker-build-id --enable-linux-futex --program-suffix=-5 --without-system-libunwind --enable-multilib --with-arch-32=i586 --with-tune=generic --build=x86_64-suse-linux --host=x86_64-suse-linux
Thread model: posix
gcc version 5.0.0 20150129 (experimental) (SUSE Linux)

gcc-4.8.3 and 4.9.2 compile and link the same code just fine.
Comment 1 Richard Biener 2015-01-29 15:26:46 UTC
Comment 2 Jan Hubicka 2015-02-01 18:49:32 UTC
Comment 3 Jan Hubicka 2015-02-02 06:31:15 UTC
Hmm, interesting, I did not even think we support this ;)
I am not sure how much it makes sense to do incremntal linking like this with LTO because this limits LTO to incremental link only (it would perhaps make sense to produce LTO file after merigng in this case).

What changed since GCC 4.9 is that visibility pass is now more aggressive on bringing comdat locals.  In resolution file we have:
$ more *.res
file2.o 6
212 f6705eca5fa850e3 PREVAILING_DEF _ZN11CDialogBase11GetDocumentEv
217 f6705eca5fa850e3 PREVAILING_DEF dialog2
204 f6705eca5fa850e3 PREVAILING_DEF _ZTV11CDialogBase
232 f6705eca5fa850e3 PREVAILING_DEF _ZTS11CDialogBase
248 f6705eca5fa850e3 PREVAILING_DEF _ZTI11CDialogBase
259 f6705eca5fa850e3 UNDEF _ZTVN10__cxxabiv117__class_type_infoE

which makes us to believe that all the symbols are previaling ones for current DSO.  The trick it that incrementally this is not quite true. I suppose only way to make this work reliably is to modify:
static bool                                                                     
resolution_to_local_definition_p (enum ld_plugin_symbol_resolution resolution)  
  return (resolution == LDPR_PREVAILING_DEF                                     
          || resolution == LDPR_PREVAILING_DEF_IRONLY_EXP                       
          || resolution == LDPR_PREVAILING_DEF_IRONLY);                         
to exclude LDPR_PREVAILING_DEF and similarly in ipa.c.  This way we lose some optimization in non-incremental linking where we know that we are the only LTO unit in proram.

So perhaps doing this conditionally on -r? We will probably makes it funnier to produce testcases. Or we don't really care as it normally affects only cases where non-LTO and LTO objects are mixed.
Comment 4 Richard Biener 2015-03-18 12:53:50 UTC
Yes, -r should be honored by the appropriate resolution info (not _IR_ONLY).  Thus you have to treat that conservatively.

Otherwise you can create a testcase that fails with the final link and the
other strong definition in a shared library.
Comment 5 Jan Hubicka 2015-03-18 18:30:46 UTC
Well, the problem is that -r is the only case where we get LDPR_PREVAILING_DEF or IRONLY and the resulting symbol may be removed from the unit later.
We would need a new LDPR_PREVAILING_DEF_FOR_NOW or something like that to represent the incremental nature of linking here, or we need a global flag in GCC saying if incremental linking is supposed tohappen or not.
Comment 6 Jan Hubicka 2015-03-18 18:32:59 UTC
(just to explain bit more - the main difference between static and dynamic linking here is that dynamic linking never remove any definition and thus you can dissolve comdat groups)
Comment 7 Jan Hubicka 2015-03-20 20:42:06 UTC
I suppose proper fix is to make flag_incremental_linking and turn -r from Driver only.  Then we could revisit individual optimizations that do rely on the fact that static linking will not be re-done.
Comment 8 Jan Hubicka 2015-03-23 01:21:11 UTC
Created attachment 35100 [details]
partial patch

this is a patch that adds necessary checks into resolution code. Basically if static linking is anticipated, we can not derive much of useful info from PREVAILING_DEF/PREVAILING_DEF_IRONLY_EXP and must anticipate that static linking may change these and drop comdat groups.

THe catch is that someone needs to pass -r to lto1 that is not currently happening (I verified that if I hack opts.c to set incremental_linking to true the testcase works).

With -r option, I can imagine that lto-wrapper can do it, because we get:
COLLECT_GCC_OPTIONS='-c' '-fmath-errno' '-fsigned-zeros' '-ftrapping-math' '-fno-trapv' '-fno-openmp' '-fno-openacc' '-mtune=generic' '-march=x86-64' '-O2' '-B' './' '-O2' '-r' '-v' '-mtune=generic' '-march=x86-64' '-fltrans-output-list=/tmp/cc3o3UIr.ltrans.out' '-fwpa' '-fresolution=/tmp/cchYXuvF.res

I donot know hwere -r gets dropped though.  But the testcase uses -Wl,-r that may need LTO plugin extension?
Comment 9 Jakub Jelinek 2015-03-27 14:10:12 UTC
Lowering to P2 on Honza's request.
Comment 10 Jan Hubicka 2015-03-30 06:03:36 UTC
I filled in binutils PR for extension of plugin API https://sourceware.org/bugzilla/show_bug.cgi?id=18178
Comment 11 Jakub Jelinek 2015-04-22 12:00:41 UTC
GCC 5.1 has been released.
Comment 12 Richard Biener 2015-07-16 09:14:18 UTC
GCC 5.2 is being released, adjusting target milestone to 5.3.
Comment 13 Jan Hubicka 2015-11-26 03:16:59 UTC
This is another bug related to incremental linking and LTO.  I will redirect them all to PR67548. The testcase now works on mainline GCC in a sense that the duplicate is gone and we get missing typeinfo:
lib1.lib:<artificial>:typeinfo for CDialogBase: error: undefined reference to 'vtable for __cxxabiv1::__class_type_info'
/usr/bin/ld: the vtable symbol may be undefined because the class is missing its key function
collect2: error: ld returned 1 exit status

that seems intended.

*** This bug has been marked as a duplicate of bug 67548 ***