diff mbox

PR rtl-optimization/32219: optimizer causees wrong code in pic/hidden/weak symbol checking

Message ID CAMe9rOrFdRqFXxw-ASFasb8M+UaaT4NNKrWUZ+D-CjpFGvOuNA@mail.gmail.com
State New
Headers show

Commit Message

H.J. Lu Feb. 12, 2015, 6:58 p.m. UTC 
On Thu, Feb 12, 2015 at 10:16 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Feb 11, 2015 at 10:22 PM, Richard Henderson <rth@redhat.com> wrote:
>> On 02/10/2015 01:19 PM, Richard Henderson wrote:
>>> As an existing issue, I'm not sure why "specified" visibility is any different
>>> from unspecified visibility.  As far as I'm aware, the "specified" bit simply
>>> means that the decl doesn't inherit inherit visibility from the class, or from
>>> the command-line.  But once we're this far, the visibility actually applied to
>>> the symbol should be all that matters.
>>
>> The test is there to differentiate explicit visibility from that implied from
>> the command-line.  Without it, we assume hidden visibility for external symbols
>> too early, making the command-line option useless.  This is visible even in
>> building libgcc.
>>
>> I believe this set of patches does what we want, and cleans things up a bit in
>> the process.
>>
>>
>
> I tried them on Linux/x86-64.  They caused:
>
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
> scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
> scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
> scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
> scan-assembler-not _ZTW1i@PLT
>

I am testing this patch on top of yours.

Comments

Richard Henderson Feb. 12, 2015, 7:25 p.m. UTC | #1 
On 02/12/2015 10:58 AM, H.J. Lu wrote:
>    if (DECL_VISIBILITY_SPECIFIED (exp)
> +      && (resolved_locally
> +	  || !flag_pic

Yes, this essentially goes back to your original patch, which I claim still
conflates issues.

In particular, I believe mentioning flag_pic here is a serious error.

There are two problems that I see so far,

 (1) node->definition isn't set for this symbol.  This is because you
     only fixed varpool_node::finalize_decl, and not
     cgraph_node::finalize_function.

 (2) The weak test should probably be split into two pieces, like the
     visibility test: exclude undefined weak, include specified visibility,
     exclude non-dominant weak, exclude external, include implied visibility.


r~
diff mbox

Patch

From 3b516badec25151acd4a96fa4ef07b3f88e3f053 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 12 Feb 2015 10:55:55 -0800
Subject: [PATCH] A variable is local if specified by user

And it is resolved or defined locally, not compiling for PIC or not weak.
---
 gcc/varasm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/gcc/varasm.c b/gcc/varasm.c
index 9f79416..b14f2d3 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6838,15 +6838,21 @@  default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
 	resolved_locally = true;
     }
 
-  /* Undefined (or non-dominant) weak symbols are not defined locally.  */
-  if (DECL_WEAK (exp) && !resolved_locally)
-    return false;
-
-  /* A variable is local if the user has said explicitly that it will be.  */
+  /* A variable is local if the user has said explicitly that it will
+     be and it is resolved or defined locally, not compiling for PIC or
+     not weak.  */
   if (DECL_VISIBILITY_SPECIFIED (exp)
+      && (resolved_locally
+	  || !flag_pic
+	  || !DECL_EXTERNAL (exp)
+	  || !DECL_WEAK (exp))
       && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
     return true;
 
+  /* Undefined (or non-dominant) weak symbols are not defined locally.  */
+  if (DECL_WEAK (exp) && !resolved_locally)
+    return false;
+
   /* Variables defined outside this object might not be local.  */
   if (DECL_EXTERNAL (exp) && !resolved_locally)
     return false;
-- 
1.9.3