Bug 67548 - [5 Regression] LTO drops weak binding with "ld -r"
Summary: [5 Regression] LTO drops weak binding with "ld -r"
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: lto (show other bugs)
Version: 6.0
: P2 normal
Target Milestone: 6.0
Assignee: Jan Hubicka
URL:
Keywords:
: 64860 (view as bug list)
Depends on: 68662
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-11 12:16 UTC by H.J. Lu
Modified: 2017-10-10 14:56 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2015-09-11 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description H.J. Lu 2015-09-11 12:16:50 UTC
[hjl@gnu-6 lto]$ cat test.c
extern int x;

void foobar (void) { x--; }
[hjl@gnu-6 lto]$ cat test-weak.c
#include <stdio.h>

int x;

__attribute__((weak))
void foobar (void) { x++; }

int main (void)
{
  foobar ();
  if (x == -1)
    printf ("OK\n");
  return 0;
}
[hjl@gnu-6 lto]$ make
gcc  -flto -O2    -c -o test-weak.o test-weak.c
gcc  -flto -O2  -nostdlib -r -o test-intermediate.o test-weak.o
readelf -sW test-intermediate.o | grep foobar
    10: 0000000000000000     8 FUNC    GLOBAL DEFAULT    1 foobar
gcc  -flto -O2    -c -o test.o test.c
gcc  -flto -O2  test-intermediate.o test.o -o prog
test.o (symbol from plugin): In function `foobar':
(.text+0x0): multiple definition of `foobar'
test-intermediate.o:(.text+0x0): first defined here
/tmp/cciS2u59.ltrans0.ltrans.o: In function `foobar':
<artificial>:(.text+0x0): multiple definition of `foobar'
test-intermediate.o:(.text+0x0): first defined here
collect2: error: ld returned 1 exit status
Makefile:11: recipe for target 'prog' failed
make: *** [prog] Error 1
[hjl@gnu-6 lto]$
Comment 1 H.J. Lu 2015-09-11 12:27:36 UTC
It was caused by r210592.
Comment 2 H.J. Lu 2015-09-11 12:31:27 UTC
This may be a linker bug if it tells GCC that a weak symbol is
prevailing with "ld -r".
Comment 3 H.J. Lu 2015-09-11 16:39:56 UTC
We have

enum ld_plugin_symbol_resolution
{
  LDPR_UNKNOWN = 0,

  /* Symbol is still undefined at this point.  */
  LDPR_UNDEF,

  /* This is the prevailing definition of the symbol, with references from
     regular object code.  */
  LDPR_PREVAILING_DEF,

  /* This is the prevailing definition of the symbol, with no
     references from regular objects.  It is only referenced from IR
     code.  */
  LDPR_PREVAILING_DEF_IRONLY,

  /* This definition was pre-empted by a definition in a regular
     object file.  */
  LDPR_PREEMPTED_REG,

  /* This definition was pre-empted by a definition in another IR file.  */
  LDPR_PREEMPTED_IR,

  /* This symbol was resolved by a definition in another IR file.  */
  LDPR_RESOLVED_IR,

  /* This symbol was resolved by a definition in a regular object
     linked into the main executable.  */
  LDPR_RESOLVED_EXEC,

  /* This symbol was resolved by a definition in a shared object.  */
  LDPR_RESOLVED_DYN,

  /* This is the prevailing definition of the symbol, with no
     references from regular objects.  It is only referenced from IR
     code, but the symbol is exported and may be referenced from
     a dynamic object (not seen at link time).  */
  LDPR_PREVAILING_DEF_IRONLY_EXP
};

None of them is applicable to a weakdef with "ld -r".
Comment 4 H.J. Lu 2015-09-11 16:49:33 UTC
It is wrong to clear DECL_WEAK:

DECL_WEAK (next->decl) = false;
Comment 5 Jan Hubicka 2015-09-12 16:04:12 UTC
> None of them is applicable to a weakdef with "ld -r".
Yep, GCC simply assumes that incremental linking is not going to happen and it makes strong assumptions
about visibility in static&PIC binaries.
To support incremental linking we need a way to pass this information to GCC. Either by extra codes or
by informing the plugin that linking is incremental.
See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64860

Honza
Comment 6 H.J. Lu 2015-09-12 17:02:58 UTC
(In reply to Jan Hubicka from comment #5)
> > None of them is applicable to a weakdef with "ld -r".
> Yep, GCC simply assumes that incremental linking is not going to happen and
> it makes strong assumptions
> about visibility in static&PIC binaries.
> To support incremental linking we need a way to pass this information to
> GCC. Either by extra codes or
> by informing the plugin that linking is incremental.
> See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64860
> 
> Honza

Linker passes this

/* The type of output file being generated by the linker.  */

enum ld_plugin_output_file_type
{
  LDPO_REL,
  LDPO_EXEC,
  LDPO_DYN,
  LDPO_PIE
};

to GCC plug-in.  But this information is never used by GCC plug-in.
Comment 7 Richard Biener 2015-09-14 11:29:05 UTC
I thought we do nothing in an incremental link but concat the LTO input sections?
That's why we put that $ID suffix on the section names.  So maybe with plugin
auto-loading (and thus slim objects?) the LTO plugin shouldn't claim any files
with LDPO_REL mode.
Comment 8 Jan Hubicka 2015-11-18 20:56:43 UTC
There are two incremental link implementations:

 1) HJ's binutils will concat the sections, 
 2) mainline binutils will invoke GCC via plugin and produce .o file with final assembly.
The correct implementation IMO is 
 3) invoke GCC via plugin and produce the merge LTO file (i.e. cut the process before WPA optimization starts and stream out everything again).

3) would be only way to also support fat LTO files.  Sadly we do not have plugin API interface for 2) or 3).  2) would be relatively easy - we only need to make GCC know it does incremental linking and assume that every symbol can be bound externally.  I did not find way how to get this situation detected from the plugin/wrapper though and I am not sure how useful it is in practice given that it won't give you whole program optimization.
Comment 9 Jan Hubicka 2015-11-18 21:03:11 UTC
Aha, missed HJ's comment. If we have the info on type of file, this ought to be relatively easy to fix.  I will look into it.
Comment 10 Jan Hubicka 2015-11-25 23:05:39 UTC
Author: hubicka
Date: Wed Nov 25 23:05:07 2015
New Revision: 230915

URL: https://gcc.gnu.org/viewcvs?rev=230915&root=gcc&view=rev
Log:

	PR lto/67548
	* lto-plugin.c (linker_output, linker_output_set): New statics.
	(all_symbols_read_handler): Add -flinker-output option.
	(onload): Record linker_output info.

	* ipa-visibility.c (cgraph_externally_visible_p,
	varpool_node::externally_visible_p): When doing incremental linking,
	hidden symbols may be still used later.
	(update_visibility_by_resolution_info): Do not drop weak during
	incremental link.
	(function_and_variable_visibility): Fix formating.
	* flag-types.h (lto_linker_output): Declare.
	* common.opt 9flag_incremental_link): New flag.

	* lto-lang.c (lto_post_options): Process flag_lto_linker_output.
	* lang.opt (lto_linker_output): New enum.
	(flinker_output): New flag.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/common.opt
    trunk/gcc/flag-types.h
    trunk/gcc/ipa-visibility.c
    trunk/gcc/lto/ChangeLog
    trunk/gcc/lto/lang.opt
    trunk/gcc/lto/lto-lang.c
    trunk/lto-plugin/ChangeLog
    trunk/lto-plugin/lto-plugin.c
Comment 11 Jan Hubicka 2015-11-26 03:16:59 UTC
*** Bug 64860 has been marked as a duplicate of this bug. ***
Comment 12 Jan Hubicka 2015-12-03 04:42:03 UTC
Fixed on trunk so far
Comment 13 Richard Biener 2015-12-04 10:43:14 UTC
GCC 5.3 is being released, adjusting target milestone.
Comment 14 Tejas Belagod 2015-12-16 22:34:22 UTC
Author: belagod
Date: Wed Dec 16 22:33:51 2015
New Revision: 231724

URL: https://gcc.gnu.org/viewcvs?rev=231724&root=gcc&view=rev
Log:
	Backport from Mainline
        PR lto/67548
        * lto-plugin.c (linker_output, linker_output_set): New statics.
        (all_symbols_read_handler): Add -flinker-output option.
        (onload): Record linker_output info.
    
        * ipa-visibility.c (cgraph_externally_visible_p,
        varpool_node::externally_visible_p): When doing incremental linking,
        hidden symbols may be still used later.
        (update_visibility_by_resolution_info): Do not drop weak during
        incremental link.
        (function_and_variable_visibility): Fix formating.
        * flag-types.h (lto_linker_output): Declare.
        * common.opt 9flag_incremental_link): New flag.
    
        * lto-lang.c (lto_post_options): Process flag_lto_linker_output.
        * lang.opt (lto_linker_output): New enum.
        (flinker_output): New flag.


Added:
    branches/ARM/embedded-5-branch/gcc/lto/ChangeLog.arm
    branches/ARM/embedded-5-branch/lto-plugin/ChangeLog.arm
Modified:
    branches/ARM/embedded-5-branch/gcc/ChangeLog.arm
    branches/ARM/embedded-5-branch/gcc/common.opt
    branches/ARM/embedded-5-branch/gcc/flag-types.h
    branches/ARM/embedded-5-branch/gcc/ipa-visibility.c
    branches/ARM/embedded-5-branch/gcc/lto/lang.opt
    branches/ARM/embedded-5-branch/gcc/lto/lto-lang.c
    branches/ARM/embedded-5-branch/lto-plugin/lto-plugin.c
Comment 15 Richard Biener 2016-06-03 10:03:30 UTC
GCC 5.4 is being released, adjusting target milestone.
Comment 16 Jakub Jelinek 2017-10-10 13:51:55 UTC
GCC 5 branch has been closed, should be fixed in GCC 6 and later.