Bug 65780 - [5 Regression] Uninitialized common handling in executables
Summary: [5 Regression] Uninitialized common handling in executables
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 5.0
: P1 normal
Target Milestone: 5.0
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-15 22:29 UTC by Jakub Jelinek
Modified: 2015-05-11 07:14 UTC (History)
9 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2015-04-16 00:00:00


Attachments
A patch (1.21 KB, patch)
2015-04-16 02:38 UTC, H.J. Lu
Details | Diff
An updated patch (1.23 KB, patch)
2015-04-16 03:22 UTC, H.J. Lu
Details | Diff
A different patch (1.22 KB, patch)
2015-04-16 04:32 UTC, H.J. Lu
Details | Diff
A new patch (1.41 KB, patch)
2015-04-16 11:48 UTC, H.J. Lu
Details | Diff
A patch (2.12 KB, patch)
2015-04-16 12:33 UTC, H.J. Lu
Details | Diff
A patch (2.14 KB, patch)
2015-04-16 13:08 UTC, H.J. Lu
Details | Diff
A patch with updated comments (2.14 KB, patch)
2015-04-16 13:16 UTC, H.J. Lu
Details | Diff
The final patch (2.68 KB, patch)
2015-04-16 16:08 UTC, H.J. Lu
Details | Diff
The final patch (2.71 KB, patch)
2015-04-16 18:33 UTC, H.J. Lu
Details | Diff
The final patch with variable name change and updated comments (2.79 KB, patch)
2015-04-16 22:02 UTC, H.J. Lu
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2015-04-15 22:29:45 UTC
int optopt;

int
main ()
{
  optopt = 4;
  return 0;
}

fails to link with GCC 5 on powerpc64{,le}-linux:
gcc -O2 -o a a.c
/usr/bin/ld: /tmp/cckV8RvX.o: In function `main':
a.c:(.text.startup+0x6): unresolvable R_PPC64_TOC16_HA against `optopt@@GLIBC_2.3'
/usr/bin/ld: a.c:(.text.startup+0xe): unresolvable R_PPC64_TOC16_LO against `optopt@@GLIBC_2.3'
/usr/bin/ld: final link failed: Nonrepresentable section on output
collect2: error: ld returned 1 exit status

The problem seems to be that targetm.binds_local_p now returns true for !shlib
DECL_COMMON with NULL (or error_mark_node) DECL_INITIAL.
Looking at the difference between 4.9 and 5, there is:
  if (defined_locally && weak_dominate && !shlib)
    resolved_locally = true;
and finally (similar to 4.9)
  /* Uninitialized COMMON variable may be unified with symbols
     resolved from other modules.  */
  if (DECL_COMMON (exp)
      && !resolved_locally
      && (DECL_INITIAL (exp) == NULL
          || (!in_lto_p && DECL_INITIAL (exp) == error_mark_node)))
    return false;
Looking at what x86_64-linux linker does in this case, assuming that an unitialized common binds locally in the executable if it is defined there works fine, but clearly not on ppc*.  Either it is a linker bug there, or we for powerpc* (some other targets too) should treat the uninitialized DECL_COMMON defined_locally && weak_dominate && !shlib differently - not set resolved_locally in that case?
Can people try other targets?  The testcase relies on optopt being an integer variable defined in libc shared library, if that is not the case, create a shared library with that symbol and link it to the binary.
Comment 1 James Cowgill 2015-04-15 23:01:53 UTC
The testcase fails on mipsel-linux-gnu with:

$ gcc-5 test.c
/usr/bin/ld: non-dynamic relocations refer to dynamic symbol optopt@@GLIBC_2.0
/usr/bin/ld: failed to set dynamic section sizes: Bad value
collect2: error: ld returned 1 exit status

It works with 4.9.
Comment 2 H.J. Lu 2015-04-15 23:13:15 UTC
Please provide the output of "readelf -sW a.o" to verify
if optopt is COMMON:

[hjl@gnu-6 gcc]$ ./xgcc -B./ -O2 -c /tmp/a.c 
[hjl@gnu-6 gcc]$ readelf -sW a.o 

Symbol table '.symtab' contains 12 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS a.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT    1 
     3: 0000000000000000     0 SECTION LOCAL  DEFAULT    2 
     4: 0000000000000000     0 SECTION LOCAL  DEFAULT    3 
     5: 0000000000000000     0 SECTION LOCAL  DEFAULT    4 
     6: 0000000000000000     0 SECTION LOCAL  DEFAULT    5 
     7: 0000000000000000     0 SECTION LOCAL  DEFAULT    8 
     8: 0000000000000000     0 SECTION LOCAL  DEFAULT    9 
     9: 0000000000000000     0 SECTION LOCAL  DEFAULT    7 
    10: 0000000000000000    13 FUNC    GLOBAL DEFAULT    5 main
    11: 0000000000000004     4 OBJECT  GLOBAL DEFAULT  COM optopt
[hjl@gnu-6 gcc]$
Comment 3 H.J. Lu 2015-04-15 23:41:11 UTC
Also does -fno-common make a difference?
Comment 4 Alan Modra 2015-04-16 00:37:14 UTC
optopt is common, and from a linker perspective I believe the correct resolution of a common symbol in the executable and a strong definition in a shared library, is the shared library symbol.  Therefore the change to binds_local_p is wrong.

What's going on here is that HJ has made changes for x86 to always copy variables defined in a shared library into the executable's .dynbss if referenced by the executable.  This is necessary to support non-PIC without text relocations, but isn't necessary for PIC.  It is certainly possible to do this for PIC but I think only x86 does so.
Comment 5 H.J. Lu 2015-04-16 02:10:44 UTC
I can reproduce it with binutils 2.24 on x86-64:

[hjl@gnu-tools-1 gcc]$ ./xgcc -B./ -fPIE -pie /tmp/a.c 
/export/build/gnu/binutils/release/usr/local/bin/ld: /tmp/ccazj1RF.o: relocation R_X86_64_PC32 against undefined symbol `optopt@@GLIBC_2.2.5' can not be used when making a shared object; recompile with -fPIC
/export/build/gnu/binutils/release/usr/local/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status
[hjl@gnu-tools-1 gcc]$
Comment 6 H.J. Lu 2015-04-16 02:38:49 UTC
Created attachment 35325 [details]
A patch

Please try this.
Comment 7 H.J. Lu 2015-04-16 03:22:42 UTC
Created attachment 35326 [details]
An updated patch

Copy reloc in PIE only works for x86-64.  It isn't allowed for -m32.
Comment 8 Alan Modra 2015-04-16 03:28:46 UTC
Changing the summary back to as it was.  -fPIE is not necessary to show this bug on powerpc64 (or mips by the look of comment #1).
Comment 9 H.J. Lu 2015-04-16 04:32:14 UTC
Created attachment 35327 [details]
A different patch

On x86, this issue only shows up with PIE. Here is a different
patch to treat common symbol defined locally only if the backend
passes true common_maybe_local.  For x86-64, it is true only if
HAVE_LD_PIE_COPYRELOC is 1.  For i386, it is always false.  If
we aren't building PIE, common_maybe_local is true or false
doesn't make a difference for x86 since the common symbol is
always referenced normally with copy reloc.  For PIE on x86-64,
common symbol is local only if HAVE_LD_PIE_COPYRELOC is 1.
Comment 10 Jakub Jelinek 2015-04-16 07:33:26 UTC
(In reply to H.J. Lu from comment #9)
> Created attachment 35327 [details]
> A different patch
> 
> On x86, this issue only shows up with PIE. Here is a different
> patch to treat common symbol defined locally only if the backend
> passes true common_maybe_local.  For x86-64, it is true only if
> HAVE_LD_PIE_COPYRELOC is 1.  For i386, it is always false.  If
> we aren't building PIE, common_maybe_local is true or false
> doesn't make a difference for x86 since the common symbol is
> always referenced normally with copy reloc.  For PIE on x86-64,
> common symbol is local only if HAVE_LD_PIE_COPYRELOC is 1.

+
+  /* For common symbol, it is defined locally only if common_maybe_local
+     is true.  */
+  bool defined_locally = (!DECL_EXTERNAL (exp)
+			  && (!DECL_COMMON (exp) || common_maybe_local));

I think better would be:
  bool uninited_common = (DECL_COMMON (exp)
                          && (DECL_INITIAL (exp) == NULL
                              || (!in_lto_p && DECL_INITIAL (exp) == error_mark_node)));
  /* For common symbol, it is defined locally only if common_maybe_local
     is true.  */
  bool defined_locally = (!DECL_EXTERNAL (exp) && (!uninited_common || common_maybe_local));
...
and then use
  /* Uninitialized COMMON variable may be unified with symbols
     resolved from other modules.  */
  if (uninited_common && !resolved_locally)
    return false;
Comment 11 Jakub Jelinek 2015-04-16 07:46:08 UTC
And, shouldn't common_maybe_local for i?86/x86_64 be
  !flag_pic || (TARGET_64BIT && HAVE_LD_PIE_COPYRELOC != 0)
?  What about other targets that are known to generate COPY relocations in this case for non-PIE executables?  Should they pass !flag_pic?
Perhaps there should be a generic default_binds_local_p* entry point that passes
!flag_pic as common_maybe_local, that those targets could (after maintainers test it properly with various vintage linkers?) use as their TARGET_BINDS_LOCAL_P ?
Clearly rs6000 should pass always false though, perhaps many others too.
Comment 12 Jakub Jelinek 2015-04-16 11:09:17 UTC
I've tried the #c0 testcase with gcc 5.1 rc and binutils 2.25 on various linux architectures.  On armv7hl, x86_64, s390 and s390x no errors are reported for both normal executable and PIE, for i686 PIE link fails, normal works, for powerpc64{,le} expectedly both normal and PIE link fail, aarch64 I'm still waiting for results.
Comment 13 H.J. Lu 2015-04-16 11:12:53 UTC
(In reply to Jakub Jelinek from comment #11)
> And, shouldn't common_maybe_local for i?86/x86_64 be
>   !flag_pic || (TARGET_64BIT && HAVE_LD_PIE_COPYRELOC != 0)
> ?  What about other targets that are known to generate COPY relocations in
> this case for non-PIE executables?  Should they pass !flag_pic?
> Perhaps there should be a generic default_binds_local_p* entry point that
> passes
> !flag_pic as common_maybe_local, that those targets could (after maintainers
> test it properly with various vintage linkers?) use as their
> TARGET_BINDS_LOCAL_P ?

Check flag_pic isn't necessary.  For non-PIC, the same code sequence
and relocation are used to access defined and undefined symbols, common
or not.
Comment 14 H.J. Lu 2015-04-16 11:19:58 UTC
(In reply to Jakub Jelinek from comment #10)
> (In reply to H.J. Lu from comment #9)
> > Created attachment 35327 [details]
> > A different patch
> > 
> > On x86, this issue only shows up with PIE. Here is a different
> > patch to treat common symbol defined locally only if the backend
> > passes true common_maybe_local.  For x86-64, it is true only if
> > HAVE_LD_PIE_COPYRELOC is 1.  For i386, it is always false.  If
> > we aren't building PIE, common_maybe_local is true or false
> > doesn't make a difference for x86 since the common symbol is
> > always referenced normally with copy reloc.  For PIE on x86-64,
> > common symbol is local only if HAVE_LD_PIE_COPYRELOC is 1.
> 
> +
> +  /* For common symbol, it is defined locally only if common_maybe_local
> +     is true.  */
> +  bool defined_locally = (!DECL_EXTERNAL (exp)
> +			  && (!DECL_COMMON (exp) || common_maybe_local));
> 
> I think better would be:
>   bool uninited_common = (DECL_COMMON (exp)
>                           && (DECL_INITIAL (exp) == NULL
>                               || (!in_lto_p && DECL_INITIAL (exp) ==
> error_mark_node)));
>   /* For common symbol, it is defined locally only if common_maybe_local
>      is true.  */
>   bool defined_locally = (!DECL_EXTERNAL (exp) && (!uninited_common ||
> common_maybe_local));
> ...
> and then use
>   /* Uninitialized COMMON variable may be unified with symbols
>      resolved from other modules.  */
>   if (uninited_common && !resolved_locally)
>     return false;

What does initialized COMMON look like to linker?  If it is marked
as COMMON symbol to linker, it will be treated the same as
uninitialized COMMON symbol.
Comment 15 H.J. Lu 2015-04-16 11:40:23 UTC
(In reply to Jakub Jelinek from comment #10)
> (In reply to H.J. Lu from comment #9)
> > Created attachment 35327 [details]
> > A different patch
> > 
> > On x86, this issue only shows up with PIE. Here is a different
> > patch to treat common symbol defined locally only if the backend
> > passes true common_maybe_local.  For x86-64, it is true only if
> > HAVE_LD_PIE_COPYRELOC is 1.  For i386, it is always false.  If
> > we aren't building PIE, common_maybe_local is true or false
> > doesn't make a difference for x86 since the common symbol is
> > always referenced normally with copy reloc.  For PIE on x86-64,
> > common symbol is local only if HAVE_LD_PIE_COPYRELOC is 1.
> 
> +
> +  /* For common symbol, it is defined locally only if common_maybe_local
> +     is true.  */
> +  bool defined_locally = (!DECL_EXTERNAL (exp)
> +			  && (!DECL_COMMON (exp) || common_maybe_local));
> 
> I think better would be:
>   bool uninited_common = (DECL_COMMON (exp)
>                           && (DECL_INITIAL (exp) == NULL
>                               || (!in_lto_p && DECL_INITIAL (exp) ==
> error_mark_node)));
>   /* For common symbol, it is defined locally only if common_maybe_local
>      is true.  */
>   bool defined_locally = (!DECL_EXTERNAL (exp) && (!uninited_common ||
> common_maybe_local));
> ...
> and then use
>   /* Uninitialized COMMON variable may be unified with symbols
>      resolved from other modules.  */
>   if (uninited_common && !resolved_locally)
>     return false;

Can we find a tectase with initialized COMMON variable and compile
it as PIE?
Comment 16 Jakub Jelinek 2015-04-16 11:41:32 UTC
(In reply to H.J. Lu from comment #13)
> Check flag_pic isn't necessary.  For non-PIC, the same code sequence
> and relocation are used to access defined and undefined symbols, common
> or not.

What do you mean by is not necessary?  Without that, you'll return false from targetm.binds_local_p for DECL_COMMON in the testcase say on i686-linux, or if you have old linker.
Comment 17 Jakub Jelinek 2015-04-16 11:44:02 UTC
(In reply to H.J. Lu from comment #15)
> Can we find a tectase with initialized COMMON variable and compile
> it as PIE?

I don't know where initialized DECL_COMMON could come from, but this spot certainly isn't the only one that is counting with that, e.g. when deciding what section to use comm_section is used only if it is bbs_initializer_p, etc.
At least for GCC 5, I'd strongly prefer small provably correct changes at this point.
Comment 18 H.J. Lu 2015-04-16 11:46:25 UTC
(In reply to Jakub Jelinek from comment #10)
> (In reply to H.J. Lu from comment #9)
> > Created attachment 35327 [details]
> > A different patch
> > 
> > On x86, this issue only shows up with PIE. Here is a different
> > patch to treat common symbol defined locally only if the backend
> > passes true common_maybe_local.  For x86-64, it is true only if
> > HAVE_LD_PIE_COPYRELOC is 1.  For i386, it is always false.  If
> > we aren't building PIE, common_maybe_local is true or false
> > doesn't make a difference for x86 since the common symbol is
> > always referenced normally with copy reloc.  For PIE on x86-64,
> > common symbol is local only if HAVE_LD_PIE_COPYRELOC is 1.
> 
> +
> +  /* For common symbol, it is defined locally only if common_maybe_local
> +     is true.  */
> +  bool defined_locally = (!DECL_EXTERNAL (exp)
> +			  && (!DECL_COMMON (exp) || common_maybe_local));
> 
> I think better would be:
>   bool uninited_common = (DECL_COMMON (exp)
>                           && (DECL_INITIAL (exp) == NULL
>                               || (!in_lto_p && DECL_INITIAL (exp) ==
> error_mark_node)));
>   /* For common symbol, it is defined locally only if common_maybe_local
>      is true.  */
>   bool defined_locally = (!DECL_EXTERNAL (exp) && (!uninited_common ||
> common_maybe_local));
> ...
> and then use
>   /* Uninitialized COMMON variable may be unified with symbols
>      resolved from other modules.  */
>   if (uninited_common && !resolved_locally)
>     return false;

Here is a testcase:

---
int optopt = 5;
int optopt;

int
main ()
{
  optopt = 4;
  return 0;
}
----
~
Comment 19 H.J. Lu 2015-04-16 11:48:31 UTC
Created attachment 35330 [details]
A new patch
Comment 20 H.J. Lu 2015-04-16 11:53:00 UTC
(In reply to Jakub Jelinek from comment #16)
> (In reply to H.J. Lu from comment #13)
> > Check flag_pic isn't necessary.  For non-PIC, the same code sequence
> > and relocation are used to access defined and undefined symbols, common
> > or not.
> 
> What do you mean by is not necessary?  Without that, you'll return false
> from targetm.binds_local_p for DECL_COMMON in the testcase say on
> i686-linux, or if you have old linker.

I guess it won't hurt.
Comment 21 Jakub Jelinek 2015-04-16 12:12:09 UTC
I've repeated my test on the various architectures, this time with additional readelf -Ws test | grep optopt
if the link succeeds.  And indeed, x86_64 with recent linker is the only one where optopt is defined, rather than SHN_UNDEF in the PIE.  armv7hl, s390, s390x, i686 and x86_64 with old linker all have optopt defined in the binary for normal executable (!flag_pic) and SHN_UNDEF for PIE.

Thus, based on this I'd say that i386 backend should pass
!flag_pic || (TARGET_64BIT && HAVE_LD_PIE_COPYRELOC != 0)
to the new param (in ix86_binds_local_p).
Then, perhaps you should make default_binds_local_p_3 also non-static and declared in output.h, ix86_binds_local_p should perhaps use it directly, and
default_binds_local_p_2 should have just a single argument, so that arm and s390 backends (dunno, maybe aarch64 and a few others too) could use it directly as their TARGET_BINDS_LOCAL_P definition.  default_binds_local_p_2 would then call
default_binds_local_p_3 with
exp, flag_shlib != 0, true, false, !flag_pic
arguments.  And obviously all the two (default_binds_local_p{,2}) should have better documentation on how they differ.
Comment 22 H.J. Lu 2015-04-16 12:33:45 UTC
Created attachment 35331 [details]
A patch

I am testing this.
Comment 23 Jakub Jelinek 2015-04-16 12:41:46 UTC
(In reply to H.J. Lu from comment #22)
> Created attachment 35331 [details]
> A patch
> 
> I am testing this.

+static bool
+ix86_binds_local_p (const_tree exp)
+{
+  return default_binds_local_p_3 (exp, flag_shlib != 0, true, false,
+				  (!flag_pic
+				   || (TARGET_64BIT
+				       && HAVE_LD_PIE_COPYRELOC != 0)));
+}

shouldn't the 4th argument be true?  At least before this patch, i386 backend was the only one that passed true as extern_protected_data, but after this patch you never pass true to that parameter, making it dead again.

Also, a typo:

lcoal -> local
Comment 24 H.J. Lu 2015-04-16 13:08:09 UTC
Created attachment 35332 [details]
A patch
Comment 25 Jakub Jelinek 2015-04-16 13:10:45 UTC
Comment on attachment 35332 [details]
A patch

An non-external
shouldn't this be
A non-external
?
Other than that LGTM, but I'd prefer another pair of eyes on this.
Comment 26 H.J. Lu 2015-04-16 13:16:20 UTC
Created attachment 35333 [details]
A patch with updated comments
Comment 27 Jakub Jelinek 2015-04-16 15:36:30 UTC
(In reply to H.J. Lu from comment #26)
> Created attachment 35333 [details]
> A patch with updated comments

Found a couple of issues, here is incremental diff, mostly formatting improvements,
and in the case of default_binds_local_p_2 (right now unused, hopefully incrementally used
by arm, s390 and perhaps other backends later), passing true to common_maybe_local unconditionally,
when only in non-PIE binaries (thus !flag_pic) it works fine.

--- gcc/varasm.c
+++ gcc/varasm.c
@@ -6811,8 +6811,7 @@
 
 bool
 default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
-			 bool extern_protected_data,
-			 bool common_maybe_local)
+			 bool extern_protected_data, bool common_maybe_local)
 {
   /* A non-decl is an entry in the constant pool.  */
   if (!DECL_P (exp))
@@ -6902,8 +6901,7 @@
 bool
 default_binds_local_p (const_tree exp)
 {
-  return default_binds_local_p_3 (exp, flag_shlib != 0, true, false,
-				  false);
+  return default_binds_local_p_3 (exp, flag_shlib != 0, true, false, false);
 }
 
 /* Similar to default_binds_local_p, but common symbol may be local.  */
@@ -6912,14 +6910,13 @@
 default_binds_local_p_2 (const_tree exp)
 {
   return default_binds_local_p_3 (exp, flag_shlib != 0, true, false,
-				  true);
+				  !flag_pic);
 }
 
 bool
 default_binds_local_p_1 (const_tree exp, int shlib)
 {
-  return default_binds_local_p_3 (exp, shlib != 0, false, false,
-				  false);
+  return default_binds_local_p_3 (exp, shlib != 0, false, false, false);
 }
 
 /* Return true when references to DECL must bind to current definition in
Comment 28 H.J. Lu 2015-04-16 16:08:56 UTC
Created attachment 35335 [details]
The final patch
Comment 29 Jeffrey A. Law 2015-04-16 18:14:32 UTC
Oh how I wish all this stuff was better documented, both in the GCC sources and at a higher level in some linkers/loaders documentation.

ix86_binds_local_p needs a function comment.  I think with a function comment, no comments would be needed in the function body.

I think it's also advisable to get a function comment for the default_binds_local* that don't currently have one. 

What I'm trying to wrap my head around is what "defined_locally" really means.  Is it a "must" or "maybe" property when it gets set in default_binds_local_p_3?

If it's a must property, then "common_maybe_local" seems mis-named and/or mis-used (the former seems most likely to me).

If it's a maybe property, then why is a common uninit local filtered out?  At this point we don't know if a common uninit local will be defined locally or not, so it's a maybe.

I can probably go forward with an approval or specific change recommendations that would lead to approval once someone can tell me if defined_locally is a maybe or must property.
Comment 30 H.J. Lu 2015-04-16 18:33:40 UTC
Created attachment 35336 [details]
The final patch
Comment 31 H.J. Lu 2015-04-16 18:36:41 UTC
(In reply to Jeffrey A. Law from comment #29)
> Oh how I wish all this stuff was better documented, both in the GCC sources
> and at a higher level in some linkers/loaders documentation.
> 
> ix86_binds_local_p needs a function comment.  I think with a function
> comment, no comments would be needed in the function body.

Updated.

> I think it's also advisable to get a function comment for the
> default_binds_local* that don't currently have one. 

That can be done in a flow-up patch.

> What I'm trying to wrap my head around is what "defined_locally" really
> means.  Is it a "must" or "maybe" property when it gets set in
> default_binds_local_p_3?
> 
> If it's a must property, then "common_maybe_local" seems mis-named and/or
> mis-used (the former seems most likely to me).
> 
> If it's a maybe property, then why is a common uninit local filtered out? 
> At this point we don't know if a common uninit local will be defined locally
> or not, so it's a maybe.
> 
> I can probably go forward with an approval or specific change
> recommendations that would lead to approval once someone can tell me if
> defined_locally is a maybe or must property.

A common symbol in PIC will never be local, for any targets, unless
it has hidden/internal visibility.
Comment 32 Jeffrey A. Law 2015-04-16 19:20:27 UTC
HJ, you're missing my point.  I need to understand the meaning of the variable defined_locally to move forward with the patch.  Is it a must property (ie, if true, the symbol is always defined locally), or is it a may property (ie, if set, the symbol may be defined locally, but might also be defined externally).

Much of the correctness of the patch hinges on that question.
Comment 33 H.J. Lu 2015-04-16 19:28:03 UTC
(In reply to Jeffrey A. Law from comment #32)
> HJ, you're missing my point.  I need to understand the meaning of the
> variable defined_locally to move forward with the patch.  Is it a must
> property (ie, if true, the symbol is always defined locally), or is it a may
> property (ie, if set, the symbol may be defined locally, but might also be
> defined externally).
> 
> Much of the correctness of the patch hinges on that question.

There are 2 variables, defined_locally and resolved_locally.
A symbol can be defined locally, but not resolved locally,
depending on PIC and/or visibility.
Comment 34 Jakub Jelinek 2015-04-16 21:03:10 UTC
(In reply to Jeffrey A. Law from comment #29)
> What I'm trying to wrap my head around is what "defined_locally" really
> means.  Is it a "must" or "maybe" property when it gets set in
> default_binds_local_p_3?

I'd say it is a "must" property, the decl must be defined locally.
A common var is of course a corner case, because uninitialized common var "maybe" defined locally if it doesn't have a strong definition elsewhere.
And, for executables (normal and PIE), if the linker arranges for the definition to be present (through COPY relocation) in the executable, it "must" be defined locally too, even if it has a strong definition in a dependent shared library.

> If it's a must property, then "common_maybe_local" seems mis-named and/or
> mis-used (the former seems most likely to me).

I'd say mis-named; so what about common_local_p, with a comment describing it - if the linker can guarantee that an uninited common symbol in the executable will still be defined (through COPY relocation) in the executable, then this flag should be set.
Comment 35 H.J. Lu 2015-04-16 22:02:20 UTC
Created attachment 35341 [details]
The final patch with variable name change and updated comments
Comment 36 Jeffrey A. Law 2015-04-17 16:12:07 UTC
Patch in c#35 is approved.
Comment 37 hjl@gcc.gnu.org 2015-04-17 16:23:57 UTC
Author: hjl
Date: Fri Apr 17 16:23:24 2015
New Revision: 222184

URL: https://gcc.gnu.org/viewcvs?rev=222184&root=gcc&view=rev
Log:
Properly handle uninitialized common symbol

Uninitialized common symbol behavior in executables is target and linker
dependent.  default_binds_local_p_3 is made public and updated to take an
argument to indicate if the linker can guarantee that an uninitialized
common symbol in the executable will still be defined (through COPY
relocation) in the executable.  If common symbol is local to executable,
default_binds_local_p_3 will treat non-external variable as defined
locally.  default_binds_local_p_2 is changed to treat common symbol as
local for non-PIE binaries.

For i386, common symbol is local only for non-PIE binaries.  For x86-64,
common symbol is local only for non-PIE binaries or linker supports copy
reloc in PIE binaries.  If a target treats common symbol as local only
for non-PIE binaries, it can define TARGET_BINDS_LOCAL_P as
default_binds_local_p_2.

gcc/

	PR target/65780
	* output.h (default_binds_local_p_3): New.
	* varasm.c (default_binds_local_p_3): Make it public.  Take an
	argument to indicate if common symbol may be local.  If common
	symbol may be local, treat non-external variable as defined
	locally.
	(default_binds_local_p_2): Pass !flag_pic to default_binds_local_p_3.
	(default_binds_local_p_1): Pass false to default_binds_local_p_3.
	* config/i386/i386.c (ix86_binds_local_p): New.
	(TARGET_BINDS_LOCAL_P): Replace default_binds_local_p_2 with
	ix86_binds_local_p.

gcc/testsuite/

	PR target/65780
	* gcc.dg/pr65780-1.c: New test.
	* gcc.dg/pr65780-2.c: Likewise.
	* gcc.target/i386/pr32219-9.c: Likewise.
	* gcc.target/i386/pr32219-1.c (xxx): Make it initialized common
	symbol.
	* gcc.target/i386/pr64317.c (c): Initialize.

Added:
    trunk/gcc/testsuite/gcc.dg/pr65780-1.c
    trunk/gcc/testsuite/gcc.dg/pr65780-2.c
    trunk/gcc/testsuite/gcc.target/i386/pr32219-9.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/output.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/i386/pr32219-1.c
    trunk/gcc/testsuite/gcc.target/i386/pr64317.c
    trunk/gcc/varasm.c
Comment 38 H.J. Lu 2015-04-17 16:26:29 UTC
Fixed on trunk so far.
Comment 39 Jakub Jelinek 2015-04-17 16:30:03 UTC
Please commit it to the branch too, I'll do another RC tomorrow.
Comment 40 hjl@gcc.gnu.org 2015-04-17 16:36:54 UTC
Author: hjl
Date: Fri Apr 17 16:36:22 2015
New Revision: 222185

URL: https://gcc.gnu.org/viewcvs?rev=222185&root=gcc&view=rev
Log:
Properly handle uninitialized common symbol

Uninitialized common symbol behavior in executables is target and linker
dependent.  default_binds_local_p_3 is made public and updated to take an
argument to indicate if the linker can guarantee that an uninitialized
common symbol in the executable will still be defined (through COPY
relocation) in the executable.  If common symbol is local to executable,
default_binds_local_p_3 will treat non-external variable as defined
locally.  default_binds_local_p_2 is changed to treat common symbol as
local for non-PIE binaries.

For i386, common symbol is local only for non-PIE binaries.  For x86-64,
common symbol is local only for non-PIE binaries or linker supports copy
reloc in PIE binaries.  If a target treats common symbol as local only
for non-PIE binaries, it can define TARGET_BINDS_LOCAL_P as
default_binds_local_p_2.

gcc/

	PR target/65780
	* output.h (default_binds_local_p_3): New.
	* varasm.c (default_binds_local_p_3): Make it public.  Take an
	argument to indicate if common symbol may be local.  If common
	symbol may be local, treat non-external variable as defined
	locally.
	(default_binds_local_p_2): Pass !flag_pic to default_binds_local_p_3.
	(default_binds_local_p_1): Pass false to default_binds_local_p_3.
	* config/i386/i386.c (ix86_binds_local_p): New.
	(TARGET_BINDS_LOCAL_P): Replace default_binds_local_p_2 with
	ix86_binds_local_p.

gcc/testsuite/

	PR target/65780
	* gcc.dg/pr65780-1.c: New test.
	* gcc.dg/pr65780-2.c: Likewise.
	* gcc.target/i386/pr32219-9.c: Likewise.
	* gcc.target/i386/pr32219-1.c (xxx): Make it initialized common
	symbol.
	* gcc.target/i386/pr64317.c (c): Initialize.

Added:
    branches/gcc-5-branch/gcc/testsuite/gcc.dg/pr65780-1.c
    branches/gcc-5-branch/gcc/testsuite/gcc.dg/pr65780-2.c
    branches/gcc-5-branch/gcc/testsuite/gcc.target/i386/pr32219-9.c
Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/config/i386/i386.c
    branches/gcc-5-branch/gcc/output.h
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
    branches/gcc-5-branch/gcc/testsuite/gcc.target/i386/pr32219-1.c
    branches/gcc-5-branch/gcc/testsuite/gcc.target/i386/pr64317.c
    branches/gcc-5-branch/gcc/varasm.c
Comment 41 H.J. Lu 2015-04-17 16:37:42 UTC
Fixed for 5.1.
Comment 42 Dominique d'Humieres 2015-04-17 21:13:53 UTC
Revision r222184 breaks bootstrap on x86_64-apple-darwin14:

/opt/gcc/build_w/./prev-gcc/xg++ -B/opt/gcc/build_w/./prev-gcc/ -B/opt/gcc/gcc6w/x86_64-apple-darwin14.3.0/bin/ -nostdinc++ -B/opt/gcc/build_w/prev-x86_64-apple-darwin14.3.0/libstdc++-v3/src/.libs -B/opt/gcc/build_w/prev-x86_64-apple-darwin14.3.0/libstdc++-v3/libsupc++/.libs  -I/opt/gcc/build_w/prev-x86_64-apple-darwin14.3.0/libstdc++-v3/include/x86_64-apple-darwin14.3.0  -I/opt/gcc/build_w/prev-x86_64-apple-darwin14.3.0/libstdc++-v3/include  -I/opt/gcc/work/libstdc++-v3/libsupc++ -L/opt/gcc/build_w/prev-x86_64-apple-darwin14.3.0/libstdc++-v3/src/.libs -L/opt/gcc/build_w/prev-x86_64-apple-darwin14.3.0/libstdc++-v3/libsupc++/.libs -c  -DIN_GCC_FRONTEND -DIN_GCC_FRONTEND -g -O2   -gtoggle -DIN_GCC    -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror -fno-common  -DHAVE_CONFIG_H -I. -Icp -I../../work/gcc -I../../work/gcc/cp -I../../work/gcc/../include -I./../intl -I../../work/gcc/../libcpp/include -I/opt/mp-new/include  -I../../work/gcc/../libdecnumber -I../../work/gcc/../libdecnumber/dpd -I../libdecnumber -I../../work/gcc/../libbacktrace -I/opt/mp-new/include  -o cp/optimize.o -MT cp/optimize.o -MMD -MP -MF cp/.deps/optimize.TPo ../../work/gcc/cp/optimize.c
../../work/gcc/config/i386/i386.c:51747:1: error: 'bool ix86_binds_local_p(const_tree)' defined but not used [-Werror=unused-function]
 ix86_binds_local_p (const_tree exp)
 ^
cc1plus: all warnings being treated as errors
Makefile:2071: recipe for target 'i386.o' failed
make[3]: *** [i386.o] Error 1

Bootstrap is restored if I comment the block

static bool
ix86_binds_local_p (const_tree exp)
{
  return default_binds_local_p_3 (exp, flag_shlib != 0, true, true,
                                  (!flag_pic
                                   || (TARGET_64BIT
                                       && HAVE_LD_PIE_COPYRELOC != 0)));
}
Comment 43 hjl@gcc.gnu.org 2015-04-17 21:54:54 UTC
Author: hjl
Date: Fri Apr 17 21:54:22 2015
New Revision: 222201

URL: https://gcc.gnu.org/viewcvs?rev=222201&root=gcc&view=rev
Log:
Don't define ix86_binds_local_p for MacOS nor Windows

	PR target/65780
	* config/i386/i386.c (ix86_binds_local_p): Define only if
	TARGET_MACHO and TARGET_DLLIMPORT_DECL_ATTRIBUTES are false.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
Comment 44 hjl@gcc.gnu.org 2015-04-17 21:55:37 UTC
Author: hjl
Date: Fri Apr 17 21:55:05 2015
New Revision: 222202

URL: https://gcc.gnu.org/viewcvs?rev=222202&root=gcc&view=rev
Log:
Don't define ix86_binds_local_p for MacOS nor Windows

	PR target/65780
	* config/i386/i386.c (ix86_binds_local_p): Define only if
	TARGET_MACHO and TARGET_DLLIMPORT_DECL_ATTRIBUTES are false.

Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/config/i386/i386.c
Comment 45 Jack Howarth 2015-04-17 23:16:22 UTC
Is this considered fixed now for 5.1?
Comment 46 Jakub Jelinek 2015-04-18 05:36:16 UTC
I think so.
Comment 47 Stupachenko Evgeny 2015-04-21 14:14:31 UTC
The patch caused significant regressions (see below) on spec2000 INT benchmarks compiled with options “-fPIE -pie -O2 -ffast-math -mfpmath=sse -m32 -march=corei7-avx” or “-fPIE -pie -O2 -ffast-math -mfpmath=sse -m32 -march=slm”.

Sandybridge:
164.gzip    1635.0000  1602.0000 -2.01% 
197.parser  2159.0000  2117.0000 -1.94% 
254.gap     2909.0000  2886.0000 -0.79% 
256.bzip2   2232.0000  2154.0000 -3.49%

Silvermont:
164.gzip     964.0000   945.0000 -1.97% 
197.parser   951.0000   940.0000 -1.15% 
254.gap     1328.0000  1296.0000 -2.40% 
256.bzip2    952.0000   898.0000 -5.67%
Comment 48 Jakub Jelinek 2015-04-21 14:18:01 UTC
(In reply to Stupachenko Evgeny from comment #47)
> The patch caused significant regressions (see below) on spec2000 INT
> benchmarks compiled with options “-fPIE -pie -O2 -ffast-math -mfpmath=sse
> -m32 -march=corei7-avx” or “-fPIE -pie -O2 -ffast-math -mfpmath=sse -m32
> -march=slm”.
> 
> Sandybridge:
> 164.gzip    1635.0000  1602.0000 -2.01% 
> 197.parser  2159.0000  2117.0000 -1.94% 
> 254.gap     2909.0000  2886.0000 -0.79% 
> 256.bzip2   2232.0000  2154.0000 -3.49%
> 
> Silvermont:
> 164.gzip     964.0000   945.0000 -1.97% 
> 197.parser   951.0000   940.0000 -1.15% 
> 254.gap     1328.0000  1296.0000 -2.40% 
> 256.bzip2    952.0000   898.0000 -5.67%

That is the price to pay for correctness.  If you want to avoid that penalty, I'd say change ld.bfd as well as ld.gold on i?86 to use COPY relocations for PIEs like x86_64 has been changed some time ago, then when configured against improved linker and with small patch to gcc you can recover not just that cost, but some more on top of that.
Comment 49 H.J. Lu 2015-04-21 14:40:23 UTC
(In reply to Stupachenko Evgeny from comment #47)
> The patch caused significant regressions (see below) on spec2000 INT
> benchmarks compiled with options “-fPIE -pie -O2 -ffast-math -mfpmath=sse
> -m32 -march=corei7-avx” or “-fPIE -pie -O2 -ffast-math -mfpmath=sse -m32
> -march=slm”.
> 
> Sandybridge:
> 164.gzip    1635.0000  1602.0000 -2.01% 
> 197.parser  2159.0000  2117.0000 -1.94% 
> 254.gap     2909.0000  2886.0000 -0.79% 
> 256.bzip2   2232.0000  2154.0000 -3.49%
> 
> Silvermont:
> 164.gzip     964.0000   945.0000 -1.97% 
> 197.parser   951.0000   940.0000 -1.15% 
> 254.gap     1328.0000  1296.0000 -2.40% 
> 256.bzip2    952.0000   898.0000 -5.67%

As Jakub pointed out, this commit is for correctness.  Please open
a new bug report to optimize undefined/common symbol access in PIE
if linker supports copy reloc in PIE:

https://sourceware.org/bugzilla/show_bug.cgi?id=18289
Comment 50 Jakub Jelinek 2015-05-11 07:09:36 UTC
Author: jakub
Date: Mon May 11 07:09:04 2015
New Revision: 222992

URL: https://gcc.gnu.org/viewcvs?rev=222992&root=gcc&view=rev
Log:
	PR target/65780
	* config/s390/linux.h (TARGET_BINDS_LOCAL_P): Define to
	default_binds_local_p_2.
	* config/arm/linux-elf.h (TARGET_BINDS_LOCAL_P): Likewise.
	* config/aarch64/aarch64-linux.h (TARGET_BINDS_LOCAL_P): Likewise.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/aarch64/aarch64-linux.h
    trunk/gcc/config/arm/linux-elf.h
    trunk/gcc/config/s390/linux.h
Comment 51 Jakub Jelinek 2015-05-11 07:14:42 UTC
Author: jakub
Date: Mon May 11 07:14:10 2015
New Revision: 222993

URL: https://gcc.gnu.org/viewcvs?rev=222993&root=gcc&view=rev
Log:
	PR target/65780
	* config/s390/linux.h (TARGET_BINDS_LOCAL_P): Define to
	default_binds_local_p_2.
	* config/arm/linux-elf.h (TARGET_BINDS_LOCAL_P): Likewise.
	* config/aarch64/aarch64-linux.h (TARGET_BINDS_LOCAL_P): Likewise.

Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/config/aarch64/aarch64-linux.h
    branches/gcc-5-branch/gcc/config/arm/linux-elf.h
    branches/gcc-5-branch/gcc/config/s390/linux.h