[PATCH] PR32219, weak hidden reference segfault

[PATCH] PR32219, weak hidden reference segfault

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]

Hi, with reference to the old dicussion on PR 32219:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32219

It seems that a patch was submitted to put the DECL_WEAK check before
the visibility check, but that patch was never approved or applied, due
to concerns in the wording of surrounding comments:
http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00666.html

That patch does solve the segfault in the PR, and happens to also solve
the other weak-hidden + section-anchor issue Nathan's other patch solves
(simply because it works the same way by rejecting DECL_WEAK inside
binds_local_p).

However, my own testing of the PR patch on recent trunk indicates a
regression: it filters out the TLS wrapper functions for C++11
thread_local variables, causing them to be called by @PLT, and failing a
few tests that check for this.

Looking into the generated code for:

extern void weakfun() __attribute__((weak,visibility("hidden")));
void foo ()
{
  if (weakfun) weakfun();
}

Under -O1 -m32 -fPIC, the address load and test will look like:

(insn 5 2 6 2 (set (reg/f:SI 60)
   (plus:SI (reg:SI 3 bx)
      (const:SI (unspec:SI [
        (symbol_ref/i:SI ("f") [flags 0x43]  <function_decl f>)
                    ] UNSPEC_GOTOFF)))) {*leasi}
     (expr_list:REG_EQUAL (symbol_ref/i:SI ("f") <function_decl f>)
        (nil)))

(insn 6 5 7 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/f:SI 60)
            (const_int 0 [0]))) p.c:8 3 {*cmpsi_ccno_1}
     (nil))

(jump_insn 7 6 8 2 ...

However, the logic currently used in rtlanal.c:nonzero_address_p() only
test for "PIC-reg + <constant_p>", instead of more sophisticated
checking to expose the wrapped weak symbol_ref, thus confusing CSE to
eliminate the needed test.

The DECL_WEAK test upward movement in binds_local_p() works because when
bound non-local, x86 expand turns the address load into (mem (plus
(const (unspec ... UNSPEC_GOT)))) form, with the MEM helping to avoid
the above case.

Attached is a patch to make the nonzero_address_p() work for this case,
bootstrapped and tested on i686-linux without regressions.

I have the impression from the PR32219 discussion, that the solution
should be to make all weak-hidden-undefined symbols as potentially
non-local.  However, I don't think that is needed, no? As long as the
linker added zero value is in the same module, weak hidden semantics
should remain just the same...

Thanks,
Chung-Lin

2013-05-09  Chung-Lin Tang  <cltang@codesourcery.com>

	PR target/32219
	* rtlanal.c (nonzero_address_p): Robustify checking by look
        recursively into PIC constant offsets and (CONST (UNSPEC ...))
	expressions.

[-- Attachment #2: pr32219.patch --]
[-- Type: text/plain, Size: 1302 bytes --]

Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 198735)
+++ rtlanal.c	(working copy)
@@ -387,13 +387,22 @@ nonzero_address_p (const_rtx x)
       return false;
 
     case CONST:
-      return nonzero_address_p (XEXP (x, 0));
+      {
+	rtx base, offset;
+	/* Peel away any constant offsets from the base symbol.  */
+	split_const (CONST_CAST_RTX (x), &base, &offset);
+	return nonzero_address_p (base);
+      }
 
+    case UNSPEC:
+      /* Reach for a contained symbol.  */
+      return nonzero_address_p (XVECEXP (x, 0, 0));
+
     case PLUS:
       /* Handle PIC references.  */
       if (XEXP (x, 0) == pic_offset_table_rtx
 	       && CONSTANT_P (XEXP (x, 1)))
-	return true;
+	return nonzero_address_p (XEXP (x, 1));
       return false;
 
     case PRE_MODIFY:
Index: testsuite/gcc.dg/visibility-21.c
===================================================================
--- testsuite/gcc.dg/visibility-21.c	(revision 0)
+++ testsuite/gcc.dg/visibility-21.c	(revision 0)
@@ -0,0 +1,12 @@
+/* PR target/32219 */
+/* { dg-do run } */
+/* { dg-require-visibility "" } */
+/* { dg-options "-O1 -fPIC" { target fpic } } */
+
+extern void f() __attribute__((weak,visibility("hidden")));
+int main()
+{
+  if (f)
+    f();
+  return 0;
+}

Re: [PATCH] PR32219, weak hidden reference segfault

[Bernhard Reutner-Fischer]

On Thu, May 09, 2013 at 05:52:26PM +0800, Chung-Lin Tang wrote:

>2013-05-09  Chung-Lin Tang  <cltang@codesourcery.com>
>
>	PR target/32219
>	* rtlanal.c (nonzero_address_p): Robustify checking by look
>        recursively into PIC constant offsets and (CONST (UNSPEC ...))
>	expressions.

>Index: rtlanal.c
>===================================================================
>--- rtlanal.c	(revision 198735)
>+++ rtlanal.c	(working copy)
>@@ -387,13 +387,22 @@ nonzero_address_p (const_rtx x)
>       return false;
> 
>     case CONST:
>-      return nonzero_address_p (XEXP (x, 0));
>+      {
>+	rtx base, offset;
>+	/* Peel away any constant offsets from the base symbol.  */
>+	split_const (CONST_CAST_RTX (x), &base, &offset);
>+	return nonzero_address_p (base);
>+      }
> 
>+    case UNSPEC:
>+      /* Reach for a contained symbol.  */
>+      return nonzero_address_p (XVECEXP (x, 0, 0));
>+
>     case PLUS:
>       /* Handle PIC references.  */
>       if (XEXP (x, 0) == pic_offset_table_rtx
> 	       && CONSTANT_P (XEXP (x, 1)))
>-	return true;
>+	return nonzero_address_p (XEXP (x, 1));
>       return false;
> 
>     case PRE_MODIFY:
>Index: testsuite/gcc.dg/visibility-21.c
>===================================================================
>--- testsuite/gcc.dg/visibility-21.c	(revision 0)
>+++ testsuite/gcc.dg/visibility-21.c	(revision 0)

Please put this into gcc.dg/torture/ instead to make sure it works on
all optimization levels.

>@@ -0,0 +1,12 @@
>+/* PR target/32219 */
>+/* { dg-do run } */
>+/* { dg-require-visibility "" } */
>+/* { dg-options "-O1 -fPIC" { target fpic } } */

I do not remember offhand if the whole test should
dg-require-effective-target fpic
>+
>+extern void f() __attribute__((weak,visibility("hidden")));
>+int main()
>+{
>+  if (f)
>+    f();
>+  return 0;
>+}

Thanks for taking care of this PR!
Bernhard

Re: [PATCH] PR32219, weak hidden reference segfault

[Richard Sandiford]

Chung-Lin Tang <cltang@codesourcery.com> writes:
> +    case UNSPEC:
> +      /* Reach for a contained symbol.  */
> +      return nonzero_address_p (XVECEXP (x, 0, 0));

I don't think this is safe.  UNSPECs really are unspecified :-),
so we can't assume that (unspec X) is nonzero simply because X is.

Richard

Re: [PATCH] PR32219, weak hidden reference segfault

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On 13/5/10 6:37 PM, Richard Sandiford wrote:
> Chung-Lin Tang <cltang@codesourcery.com> writes:
>> +    case UNSPEC:
>> +      /* Reach for a contained symbol.  */
>> +      return nonzero_address_p (XVECEXP (x, 0, 0));
> 
> I don't think this is safe.  UNSPECs really are unspecified :-),
> so we can't assume that (unspec X) is nonzero simply because X is.

Attached is a modified patch (not yet tested but just for demonstration)
with a more specific test, hopefully regarded as more safe.

The point is in recognizing (const (unspec [symbol] XYZ)) offsets in PIC
references, which seems quite idiomatic across all targets by now.

I would suggest that this probably means there should be a new, more
specific construct in RTL to represent relocation values of this kind,
instead of (const (unspec)) serving an unofficial role; possibly some
real support for reasoning about PIC references could also be considered.

Chung-Lin


[-- Attachment #2: pr32219-2.patch --]
[-- Type: text/plain, Size: 659 bytes --]

Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 198923)
+++ rtlanal.c	(working copy)
@@ -393,7 +393,15 @@ nonzero_address_p (const_rtx x)
       /* Handle PIC references.  */
       if (XEXP (x, 0) == pic_offset_table_rtx
 	       && CONSTANT_P (XEXP (x, 1)))
-	return true;
+	{
+	  rtx offset = XEXP (x, 1);
+	  if (GET_CODE (offset) == CONST
+	      && GET_CODE (XEXP (offset, 0)) == UNSPEC
+	      && GET_CODE (XVECEXP (XEXP (offset, 0), 0, 0)) == SYMBOL_REF)
+	    return nonzero_address_p (XVECEXP (XEXP (offset, 0), 0, 0));
+
+	  return true;
+	}
       return false;
 
     case PRE_MODIFY:

Re: [PATCH] PR32219, weak hidden reference segfault

[Richard Sandiford]

Chung-Lin Tang <cltang@codesourcery.com> writes:
> On 13/5/10 6:37 PM, Richard Sandiford wrote:
>> Chung-Lin Tang <cltang@codesourcery.com> writes:
>>> +    case UNSPEC:
>>> +      /* Reach for a contained symbol.  */
>>> +      return nonzero_address_p (XVECEXP (x, 0, 0));
>> 
>> I don't think this is safe.  UNSPECs really are unspecified :-),
>> so we can't assume that (unspec X) is nonzero simply because X is.
>
> Attached is a modified patch (not yet tested but just for demonstration)
> with a more specific test, hopefully regarded as more safe.
>
> The point is in recognizing (const (unspec [symbol] XYZ)) offsets in PIC
> references, which seems quite idiomatic across all targets by now.

I agree this is safer.  However, there used to be some ports that
use (plus pic_offset_table_rtx (symbol_ref X)) -- i.e. without an unspec --
to represent a PIC reference to X.  That always seemed semantically wrong,
since you're not actually adding the address of X and the PIC register,
but the patch wouldn't handle that case correctly.

An alternative might be to remove the pic_offset_table_rtx case altogether
and rely on targetm.delegitimize_address instead.  FWIW, I'd prefer that
if it works, but it's not me you need to convince. :-)

> I would suggest that this probably means there should be a new, more
> specific construct in RTL to represent relocation values of this kind,
> instead of (const (unspec)) serving an unofficial role; possibly some
> real support for reasoning about PIC references could also be considered.

Yeah, maybe we could try to introduce some target-independent knowledge
of certain reloc types, a bit like the generic BFD_RELOC_*s in bfd.

Thanks,
Richard

Re: [PATCH] PR32219, weak hidden reference segfault

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

On 13/5/15 8:12 PM, Richard Sandiford wrote:
> Chung-Lin Tang <cltang@codesourcery.com> writes:
>> On 13/5/10 6:37 PM, Richard Sandiford wrote:
>>> Chung-Lin Tang <cltang@codesourcery.com> writes:
>>>> +    case UNSPEC:
>>>> +      /* Reach for a contained symbol.  */
>>>> +      return nonzero_address_p (XVECEXP (x, 0, 0));
>>>
>>> I don't think this is safe.  UNSPECs really are unspecified :-),
>>> so we can't assume that (unspec X) is nonzero simply because X is.
>>
>> Attached is a modified patch (not yet tested but just for demonstration)
>> with a more specific test, hopefully regarded as more safe.
>>
>> The point is in recognizing (const (unspec [symbol] XYZ)) offsets in PIC
>> references, which seems quite idiomatic across all targets by now.
> 
> I agree this is safer.  However, there used to be some ports that
> use (plus pic_offset_table_rtx (symbol_ref X)) -- i.e. without an unspec --
> to represent a PIC reference to X.  That always seemed semantically wrong,
> since you're not actually adding the address of X and the PIC register,
> but the patch wouldn't handle that case correctly.

Well I can't help those targets then, but at least nothing will be
changed for them by this patch. It will just continue to return 'true'.

> An alternative might be to remove the pic_offset_table_rtx case altogether
> and rely on targetm.delegitimize_address instead.  FWIW, I'd prefer that
> if it works, but it's not me you need to convince. :-)

Like we discussed below, I think the direction should be towards making
things more machine-independent, rather then pushing more into the backend.

>> I would suggest that this probably means there should be a new, more
>> specific construct in RTL to represent relocation values of this kind,
>> instead of (const (unspec)) serving an unofficial role; possibly some
>> real support for reasoning about PIC references could also be considered.
> 
> Yeah, maybe we could try to introduce some target-independent knowledge
> of certain reloc types, a bit like the generic BFD_RELOC_*s in bfd.


FWIW, I've ran tests on the newer patch on i686-linux, with no
regressions. Testcase has been moved to gcc.dg/torture by recommendation
of Bernhard. If any of the RTL maintainers can give an eye of merciful
approval, this old PR could be resolved :)

Thanks,
Chung-Lin


[-- Attachment #2: pr32219-3.patch --]
[-- Type: text/plain, Size: 1130 bytes --]

Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 199474)
+++ rtlanal.c	(working copy)
@@ -393,7 +393,15 @@ nonzero_address_p (const_rtx x)
       /* Handle PIC references.  */
       if (XEXP (x, 0) == pic_offset_table_rtx
 	       && CONSTANT_P (XEXP (x, 1)))
-	return true;
+	{
+	  rtx offset = XEXP (x, 1);
+	  if (GET_CODE (offset) == CONST
+	      && GET_CODE (XEXP (offset, 0)) == UNSPEC
+	      && GET_CODE (XVECEXP (XEXP (offset, 0), 0, 0)) == SYMBOL_REF)
+	    return nonzero_address_p (XVECEXP (XEXP (offset, 0), 0, 0));
+
+	  return true;
+	}
       return false;
 
     case PRE_MODIFY:
Index: testsuite/gcc.dg/torture/pr32219.c
===================================================================
--- testsuite/gcc.dg/torture/pr32219.c	(revision 0)
+++ testsuite/gcc.dg/torture/pr32219.c	(revision 0)
@@ -0,0 +1,12 @@
+/* PR target/32219 */
+/* { dg-do run } */
+/* { dg-require-visibility "" } */
+/* { dg-options "-fPIC" { target fpic } } */
+
+extern void f() __attribute__((weak,visibility("hidden")));
+int main()
+{
+  if (f)
+    f();
+  return 0;
+}

Re: [PATCH] PR32219, weak hidden reference segfault

[Bernhard Reutner-Fischer]

ping, CCing middle-end maintainers for review.

On 31 May 2013 10:13, Chung-Lin Tang <cltang@codesourcery.com> wrote:
> On 13/5/15 8:12 PM, Richard Sandiford wrote:
>> Chung-Lin Tang <cltang@codesourcery.com> writes:
>>> On 13/5/10 6:37 PM, Richard Sandiford wrote:
>>>> Chung-Lin Tang <cltang@codesourcery.com> writes:
>>>>> +    case UNSPEC:
>>>>> +      /* Reach for a contained symbol.  */
>>>>> +      return nonzero_address_p (XVECEXP (x, 0, 0));
>>>>
>>>> I don't think this is safe.  UNSPECs really are unspecified :-),
>>>> so we can't assume that (unspec X) is nonzero simply because X is.
>>>
>>> Attached is a modified patch (not yet tested but just for demonstration)
>>> with a more specific test, hopefully regarded as more safe.
>>>
>>> The point is in recognizing (const (unspec [symbol] XYZ)) offsets in PIC
>>> references, which seems quite idiomatic across all targets by now.
>>
>> I agree this is safer.  However, there used to be some ports that
>> use (plus pic_offset_table_rtx (symbol_ref X)) -- i.e. without an unspec --
>> to represent a PIC reference to X.  That always seemed semantically wrong,
>> since you're not actually adding the address of X and the PIC register,
>> but the patch wouldn't handle that case correctly.
>
> Well I can't help those targets then, but at least nothing will be
> changed for them by this patch. It will just continue to return 'true'.
>
>> An alternative might be to remove the pic_offset_table_rtx case altogether
>> and rely on targetm.delegitimize_address instead.  FWIW, I'd prefer that
>> if it works, but it's not me you need to convince. :-)
>
> Like we discussed below, I think the direction should be towards making
> things more machine-independent, rather then pushing more into the backend.
>
>>> I would suggest that this probably means there should be a new, more
>>> specific construct in RTL to represent relocation values of this kind,
>>> instead of (const (unspec)) serving an unofficial role; possibly some
>>> real support for reasoning about PIC references could also be considered.
>>
>> Yeah, maybe we could try to introduce some target-independent knowledge
>> of certain reloc types, a bit like the generic BFD_RELOC_*s in bfd.
>
>
> FWIW, I've ran tests on the newer patch on i686-linux, with no
> regressions. Testcase has been moved to gcc.dg/torture by recommendation
> of Bernhard. If any of the RTL maintainers can give an eye of merciful
> approval, this old PR could be resolved :)
>
> Thanks,
> Chung-Lin
>

Re: [PATCH] PR32219, weak hidden reference segfault [PING]

[Chung-Lin Tang]

Ping again?

On 13/6/11 5:20 PM, Bernhard Reutner-Fischer wrote:
> ping, CCing middle-end maintainers for review.
> 
> On 31 May 2013 10:13, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>> On 13/5/15 8:12 PM, Richard Sandiford wrote:
>>> Chung-Lin Tang <cltang@codesourcery.com> writes:
>>>> On 13/5/10 6:37 PM, Richard Sandiford wrote:
>>>>> Chung-Lin Tang <cltang@codesourcery.com> writes:
>>>>>> +    case UNSPEC:
>>>>>> +      /* Reach for a contained symbol.  */
>>>>>> +      return nonzero_address_p (XVECEXP (x, 0, 0));
>>>>>
>>>>> I don't think this is safe.  UNSPECs really are unspecified :-),
>>>>> so we can't assume that (unspec X) is nonzero simply because X is.
>>>>
>>>> Attached is a modified patch (not yet tested but just for demonstration)
>>>> with a more specific test, hopefully regarded as more safe.
>>>>
>>>> The point is in recognizing (const (unspec [symbol] XYZ)) offsets in PIC
>>>> references, which seems quite idiomatic across all targets by now.
>>>
>>> I agree this is safer.  However, there used to be some ports that
>>> use (plus pic_offset_table_rtx (symbol_ref X)) -- i.e. without an unspec --
>>> to represent a PIC reference to X.  That always seemed semantically wrong,
>>> since you're not actually adding the address of X and the PIC register,
>>> but the patch wouldn't handle that case correctly.
>>
>> Well I can't help those targets then, but at least nothing will be
>> changed for them by this patch. It will just continue to return 'true'.
>>
>>> An alternative might be to remove the pic_offset_table_rtx case altogether
>>> and rely on targetm.delegitimize_address instead.  FWIW, I'd prefer that
>>> if it works, but it's not me you need to convince. :-)
>>
>> Like we discussed below, I think the direction should be towards making
>> things more machine-independent, rather then pushing more into the backend.
>>
>>>> I would suggest that this probably means there should be a new, more
>>>> specific construct in RTL to represent relocation values of this kind,
>>>> instead of (const (unspec)) serving an unofficial role; possibly some
>>>> real support for reasoning about PIC references could also be considered.
>>>
>>> Yeah, maybe we could try to introduce some target-independent knowledge
>>> of certain reloc types, a bit like the generic BFD_RELOC_*s in bfd.
>>
>>
>> FWIW, I've ran tests on the newer patch on i686-linux, with no
>> regressions. Testcase has been moved to gcc.dg/torture by recommendation
>> of Bernhard. If any of the RTL maintainers can give an eye of merciful
>> approval, this old PR could be resolved :)
>>
>> Thanks,
>> Chung-Lin
>>

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Chung-Lin Tang]

Ping.

On 2013/6/20 03:01 PM, Chung-Lin Tang wrote:
> Ping again?
> 
> On 13/6/11 5:20 PM, Bernhard Reutner-Fischer wrote:
>> ping, CCing middle-end maintainers for review.
>>
>> On 31 May 2013 10:13, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>>> On 13/5/15 8:12 PM, Richard Sandiford wrote:
>>>> Chung-Lin Tang <cltang@codesourcery.com> writes:
>>>>> On 13/5/10 6:37 PM, Richard Sandiford wrote:
>>>>>> Chung-Lin Tang <cltang@codesourcery.com> writes:
>>>>>>> +    case UNSPEC:
>>>>>>> +      /* Reach for a contained symbol.  */
>>>>>>> +      return nonzero_address_p (XVECEXP (x, 0, 0));
>>>>>>
>>>>>> I don't think this is safe.  UNSPECs really are unspecified :-),
>>>>>> so we can't assume that (unspec X) is nonzero simply because X is.
>>>>>
>>>>> Attached is a modified patch (not yet tested but just for demonstration)
>>>>> with a more specific test, hopefully regarded as more safe.
>>>>>
>>>>> The point is in recognizing (const (unspec [symbol] XYZ)) offsets in PIC
>>>>> references, which seems quite idiomatic across all targets by now.
>>>>
>>>> I agree this is safer.  However, there used to be some ports that
>>>> use (plus pic_offset_table_rtx (symbol_ref X)) -- i.e. without an unspec --
>>>> to represent a PIC reference to X.  That always seemed semantically wrong,
>>>> since you're not actually adding the address of X and the PIC register,
>>>> but the patch wouldn't handle that case correctly.
>>>
>>> Well I can't help those targets then, but at least nothing will be
>>> changed for them by this patch. It will just continue to return 'true'.
>>>
>>>> An alternative might be to remove the pic_offset_table_rtx case altogether
>>>> and rely on targetm.delegitimize_address instead.  FWIW, I'd prefer that
>>>> if it works, but it's not me you need to convince. :-)
>>>
>>> Like we discussed below, I think the direction should be towards making
>>> things more machine-independent, rather then pushing more into the backend.
>>>
>>>>> I would suggest that this probably means there should be a new, more
>>>>> specific construct in RTL to represent relocation values of this kind,
>>>>> instead of (const (unspec)) serving an unofficial role; possibly some
>>>>> real support for reasoning about PIC references could also be considered.
>>>>
>>>> Yeah, maybe we could try to introduce some target-independent knowledge
>>>> of certain reloc types, a bit like the generic BFD_RELOC_*s in bfd.
>>>
>>>
>>> FWIW, I've ran tests on the newer patch on i686-linux, with no
>>> regressions. Testcase has been moved to gcc.dg/torture by recommendation
>>> of Bernhard. If any of the RTL maintainers can give an eye of merciful
>>> approval, this old PR could be resolved :)
>>>
>>> Thanks,
>>> Chung-Lin
>>>
> 

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Diego Novillo]

On Sun, Jul 14, 2013 at 2:08 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
> Ping.

Could you please repost the patch with its description?  This thread
is sufficiently old and noisy that I'm not even sure what the patch
does nor why.


Thanks.  Diego.

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Bernhard Reutner-Fischer]

On 14 July 2013 19:43, Diego Novillo <dnovillo@google.com> wrote:
> On Sun, Jul 14, 2013 at 2:08 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>> Ping.
>
> Could you please repost the patch with its description?  This thread
> is sufficiently old and noisy that I'm not even sure what the patch
> does nor why.

Chung-Lin Tang, can you regtest and repost the patch please?

TIA,
>
>
> Thanks.  Diego.

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Chung-Lin Tang]

On 13/8/1 5:16 PM, Bernhard Reutner-Fischer wrote:
> On 14 July 2013 19:43, Diego Novillo <dnovillo@google.com> wrote:
>> On Sun, Jul 14, 2013 at 2:08 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>>> Ping.
>>
>> Could you please repost the patch with its description?  This thread
>> is sufficiently old and noisy that I'm not even sure what the patch
>> does nor why.
> 
> Chung-Lin Tang, can you regtest and repost the patch please?
> 
> TIA,

I'll re-explain the patch later as Diego has requested, maybe this weekend.

Thanks,
Chung-Lin

>>
>>
>> Thanks.  Diego.

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

On 13/7/15 1:43 AM, Diego Novillo wrote:
> Could you please repost the patch with its description?  This thread
> is sufficiently old and noisy that I'm not even sure what the patch
> does nor why.

Taking the same example in my first post:

extern void weakfun() __attribute__((weak,visibility("hidden")));
void foo ()
{
  if (weakfun) weakfun();
}

Under -O1 -m32 -fPIC, the address load and test will look like:

(insn 5 2 6 2 (set (reg/f:SI 60)
   (plus:SI (reg:SI 3 bx)
      (const:SI (unspec:SI [
        (symbol_ref/i:SI ("f") [flags 0x43]  <function_decl f>)
                    ] UNSPEC_GOTOFF)))) {*leasi}
     (expr_list:REG_EQUAL (symbol_ref/i:SI ("f") <function_decl f>)
        (nil)))

(insn 6 5 7 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/f:SI 60)
            (const_int 0 [0]))) p.c:8 3 {*cmpsi_ccno_1}
     (nil))

(jump_insn 7 6 8 2 ...


Under -fPIC, the code in rtlanal.c:nonzero_address_p() does not properly
recognize the "PIC-reg + <constant>" form of load as a weak symbol; it
returns 'true' immediately after seeing the pic-reg indexing, and does
not test the wrapped symbol for DECL_WEAK.

My patch slightly modifies the test to look into the wrapped symbol when
seeing a "PIC-reg + <unspec-constant>" form, which I believe has become
the idiomatic form in GCC for such PIC addresses. Richard Sandiford's
concerns were that, UNSPEC really is unspecified, and this might be
overassuming its semantics, plus some targets may not be really
following the idiomatic use.

My final take at the time was, for targets that do follow the common
PIC-reg+const-unspec form, this patch solves the problem, while for
other targets, nothing is changed.

I have re-tested the patch on i686-linux, with clean results. Please see
if this patch can be accepted into trunk (patch attached again for
convenience).

Thanks,
Chung-Lin

2013-08-04  Chung-Lin Tang  <cltang@codesourcery.com>

	PR target/32219
	* rtlanal.c (nonzero_address_p): Robustify checking by look
        recursively into PIC constant offsets and (CONST (UNSPEC ...))
	expressions.


[-- Attachment #2: pr32219.patch --]
[-- Type: text/plain, Size: 1130 bytes --]

Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 201473)
+++ rtlanal.c	(working copy)
@@ -393,7 +393,15 @@ nonzero_address_p (const_rtx x)
       /* Handle PIC references.  */
       if (XEXP (x, 0) == pic_offset_table_rtx
 	       && CONSTANT_P (XEXP (x, 1)))
-	return true;
+	{
+	  rtx offset = XEXP (x, 1);
+	  if (GET_CODE (offset) == CONST
+	      && GET_CODE (XEXP (offset, 0)) == UNSPEC
+	      && GET_CODE (XVECEXP (XEXP (offset, 0), 0, 0)) == SYMBOL_REF)
+	    return nonzero_address_p (XVECEXP (XEXP (offset, 0), 0, 0));
+
+	  return true;
+	}
       return false;
 
     case PRE_MODIFY:
Index: testsuite/gcc.dg/torture/pr32219.c
===================================================================
--- testsuite/gcc.dg/torture/pr32219.c	(revision 0)
+++ testsuite/gcc.dg/torture/pr32219.c	(revision 0)
@@ -0,0 +1,12 @@
+/* PR target/32219 */
+/* { dg-do run } */
+/* { dg-require-visibility "" } */
+/* { dg-options "-fPIC" { target fpic } } */
+
+extern void f() __attribute__((weak,visibility("hidden")));
+int main()
+{
+  if (f)
+    f();
+  return 0;
+}

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Bernhard Reutner-Fischer]

On 4 August 2013 17:14:36 Chung-Lin Tang <cltang@codesourcery.com> wrote:
> On 13/7/15 1:43 AM, Diego Novillo wrote:
> > Could you please repost the patch with its description?  This thread
> > is sufficiently old and noisy that I'm not even sure what the patch
> > does nor why.
>
> Taking the same example in my first post:
>
> extern void weakfun() __attribute__((weak,visibility("hidden")));
> void foo ()
> {
>   if (weakfun) weakfun();
> }
>
> Under -O1 -m32 -fPIC, the address load and test will look like:
>
> (insn 5 2 6 2 (set (reg/f:SI 60)
>    (plus:SI (reg:SI 3 bx)
>       (const:SI (unspec:SI [
>         (symbol_ref/i:SI ("f") [flags 0x43]  <function_decl f>)
>                     ] UNSPEC_GOTOFF)))) {*leasi}
>      (expr_list:REG_EQUAL (symbol_ref/i:SI ("f") <function_decl f>)
>         (nil)))
>
> (insn 6 5 7 2 (set (reg:CCZ 17 flags)
>         (compare:CCZ (reg/f:SI 60)
>             (const_int 0 [0]))) p.c:8 3 {*cmpsi_ccno_1}
>      (nil))
>
> (jump_insn 7 6 8 2 ...
>
>
> Under -fPIC, the code in rtlanal.c:nonzero_address_p() does not properly
> recognize the "PIC-reg + <constant>" form of load as a weak symbol; it
> returns 'true' immediately after seeing the pic-reg indexing, and does
> not test the wrapped symbol for DECL_WEAK.
>
> My patch slightly modifies the test to look into the wrapped symbol when
> seeing a "PIC-reg + <unspec-constant>" form, which I believe has become
> the idiomatic form in GCC for such PIC addresses. Richard Sandiford's
> concerns were that, UNSPEC really is unspecified, and this might be
> overassuming its semantics, plus some targets may not be really
> following the idiomatic use.
>
> My final take at the time was, for targets that do follow the common
> PIC-reg+const-unspec form, this patch solves the problem, while for
> other targets, nothing is changed.
>
> I have re-tested the patch on i686-linux, with clean results. Please see
> if this patch can be accepted into trunk (patch attached again for
> convenience).
>
> Thanks,
> Chung-Lin
>
> 2013-08-04  Chung-Lin Tang  <cltang@codesourcery.com>
>
> 	PR target/32219
> 	* rtlanal.c (nonzero_address_p): Robustify checking by look
>         recursively into PIC constant offsets and (CONST (UNSPEC ...))
> 	expressions.
>
An alternative was to change default_binds_local_1() to reorder the weak 
check, worked at least for me, back then:
http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00665.html
But without the comment or at least a comment that explains it more 
accurately...

Whatever works to get this fixed :)
Thanks


Sent with AquaMail for Android
http://www.aqua-mail.com

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Mike Stump]

On Aug 4, 2013, at 8:14 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
> On 13/7/15 1:43 AM, Diego Novillo wrote:
>> Could you please repost the patch with its description?  This thread
>> is sufficiently old and noisy that I'm not even sure what the patch
>> does nor why.
> 
> Taking the same example in my first post:
> 
>  Under -fPIC, the code in rtlanal.c:nonzero_address_p() does not properly
> recognize the "PIC-reg + <constant>" form of load as a weak symbol; it
> returns 'true' immediately after seeing the pic-reg indexing, and does
> not test the wrapped symbol for DECL_WEAK.

So, I can't help but think that others would say that looking into an unspec is by nature, the wrong way to do it, unless that code is in the port.

I think the followup from Bernhard points to a better solution, though the wording in the comment was objectionable.  Merely say that the symbol, if weak and not defined, is then not local.

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Mike Stump]

[ sorry for the dup ]

On Aug 4, 2013, at 8:14 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:

> On 13/7/15 1:43 AM, Diego Novillo wrote:
>> Could you please repost the patch with its description?  This thread
>> is sufficiently old and noisy that I'm not even sure what the patch
>> does nor why.
> 
> Taking the same example in my first post:

> Under -fPIC, the code in rtlanal.c:nonzero_address_p() does not properly
> recognize the "PIC-reg + <constant>" form of load as a weak symbol; it
> returns 'true' immediately after seeing the pic-reg indexing, and does
> not test the wrapped symbol for DECL_WEAK.

So, I can't help but think that others would say that looking into an unspec is by nature, the wrong way to do it, unless that code is in the port.

I think the followup from Bernhard points to a better solution, though the wording in the comment was objectionable.  Merely say that the symbol, if weak and not defined, is then not local.

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Chung-Lin Tang]

On 13/8/5 10:06 PM, Mike Stump wrote:
> On Aug 4, 2013, at 8:14 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>> On 13/7/15 1:43 AM, Diego Novillo wrote:
>>> Could you please repost the patch with its description?  This thread
>>> is sufficiently old and noisy that I'm not even sure what the patch
>>> does nor why.
>>
>> Taking the same example in my first post:
>>
>>  Under -fPIC, the code in rtlanal.c:nonzero_address_p() does not properly
>> recognize the "PIC-reg + <constant>" form of load as a weak symbol; it
>> returns 'true' immediately after seeing the pic-reg indexing, and does
>> not test the wrapped symbol for DECL_WEAK.
> 
> So, I can't help but think that others would say that looking into an unspec is by nature, the wrong way to do it, unless that code is in the port.
> 
> I think the followup from Bernhard points to a better solution, though the wording in the comment was objectionable.  Merely say that the symbol, if weak and not defined, is then not local.

When I last tested that patch which moves the DECL_WEAK check, the
testcases for C++ TLS wrappers fail. I don't remember the fine details,
but effectively it filters out the TLS wrappers from being treated
locally, causing them to be called through @PLT, and regressing on some
tests specifically checking for that...

The UNSPEC interpretation here is fairly restricted, FWIW. Earlier talk
on this thread also mentioned that maybe specific RTL constructs for
reasoning about PIC addresses should be introduced, rather than common
idiomatic pattern, though that may be a long shot for now.

Chung-Lin

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Mike Stump]

On Aug 5, 2013, at 7:15 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
> On 13/8/5 10:06 PM, Mike Stump wrote:
>> On Aug 4, 2013, at 8:14 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>>> On 13/7/15 1:43 AM, Diego Novillo wrote:
>>>> Could you please repost the patch with its description?  This thread
>>>> is sufficiently old and noisy that I'm not even sure what the patch
>>>> does nor why.
>>> 
>>> Taking the same example in my first post:
>>> 
>>> Under -fPIC, the code in rtlanal.c:nonzero_address_p() does not properly
>>> recognize the "PIC-reg + <constant>" form of load as a weak symbol; it
>>> returns 'true' immediately after seeing the pic-reg indexing, and does
>>> not test the wrapped symbol for DECL_WEAK.
>> 
>> So, I can't help but think that others would say that looking into an unspec is by nature, the wrong way to do it, unless that code is in the port.
>> 
>> I think the followup from Bernhard points to a better solution, though the wording in the comment was objectionable.  Merely say that the symbol, if weak and not defined, is then not local.
> 
> When I last tested that patch which moves the DECL_WEAK check, the
> testcases for C++ TLS wrappers fail. I don't remember the fine details,
> but effectively it filters out the TLS wrappers from being treated
> locally, causing them to be called through @PLT, and regressing on some
> tests specifically checking for that…

Hum…  I wonder if there is a TLS predicate one can mix in to the check that is appropriate.

> The UNSPEC interpretation here is fairly restricted, FWIW. Earlier talk
> on this thread also mentioned that maybe specific RTL constructs for
> reasoning about PIC addresses should be introduced, rather than common
> idiomatic pattern, though that may be a long shot for now.

specifying the unspecified, make is specified, and calling it unspec, would seem to be wrong.

The right approach, long term, is to have address forms all specified and documented and a port merely can use the forms they are interested in.  pic is one of those things that should be bumped up, unspec is kinda silly.

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Chung-Lin Tang]

On 13/8/5 下午10:24, Mike Stump wrote:
> On Aug 5, 2013, at 7:15 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>> On 13/8/5 10:06 PM, Mike Stump wrote:
>>> On Aug 4, 2013, at 8:14 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>>>> On 13/7/15 1:43 AM, Diego Novillo wrote:
>>>>> Could you please repost the patch with its description?  This thread
>>>>> is sufficiently old and noisy that I'm not even sure what the patch
>>>>> does nor why.
>>>>
>>>> Taking the same example in my first post:
>>>>
>>>> Under -fPIC, the code in rtlanal.c:nonzero_address_p() does not properly
>>>> recognize the "PIC-reg + <constant>" form of load as a weak symbol; it
>>>> returns 'true' immediately after seeing the pic-reg indexing, and does
>>>> not test the wrapped symbol for DECL_WEAK.
>>>
>>> So, I can't help but think that others would say that looking into an unspec is by nature, the wrong way to do it, unless that code is in the port.
>>>
>>> I think the followup from Bernhard points to a better solution, though the wording in the comment was objectionable.  Merely say that the symbol, if weak and not defined, is then not local.
>>
>> When I last tested that patch which moves the DECL_WEAK check, the
>> testcases for C++ TLS wrappers fail. I don't remember the fine details,
>> but effectively it filters out the TLS wrappers from being treated
>> locally, causing them to be called through @PLT, and regressing on some
>> tests specifically checking for that…
> 
> Hum…  I wonder if there is a TLS predicate one can mix in to the check that is appropriate.
> 
>> The UNSPEC interpretation here is fairly restricted, FWIW. Earlier talk
>> on this thread also mentioned that maybe specific RTL constructs for
>> reasoning about PIC addresses should be introduced, rather than common
>> idiomatic pattern, though that may be a long shot for now.
> 
> specifying the unspecified, make is specified, and calling it unspec, would seem to be wrong.
> 
> The right approach, long term, is to have address forms all specified and documented and a port merely can use the forms they are interested in.  pic is one of those things that should be bumped up, unspec is kinda silly.

Yes, that's what I meant. e.g. instead of using (const (unspec...)), new
RTL operators for specifying the common forms of relocations (including
those used PIC) should be defined; this will involve changing lots of
backends, so should be aimed in the long term.

Chung-Lin

Re: [PATCH] PR32219, weak hidden reference segfault [PING^2]

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
> On Aug 4, 2013, at 8:14 AM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>> On 13/7/15 1:43 AM, Diego Novillo wrote:
>>> Could you please repost the patch with its description?  This thread
>>> is sufficiently old and noisy that I'm not even sure what the patch
>>> does nor why.
>> 
>> Taking the same example in my first post:
>
>> Under -fPIC, the code in rtlanal.c:nonzero_address_p() does not properly
>> recognize the "PIC-reg + <constant>" form of load as a weak symbol; it
>> returns 'true' immediately after seeing the pic-reg indexing, and does
>> not test the wrapped symbol for DECL_WEAK.
>
> So, I can't help but think that others would say that looking into an
> unspec is by nature, the wrong way to do it, unless that code is in the
> port.

Yeah.  I didn't want to reply again for fear of prolonging the thread
even more, but that's why I'd suggested removing:

      /* Handle PIC references.  */
      if (XEXP (x, 0) == pic_offset_table_rtx
	       && CONSTANT_P (XEXP (x, 1)))
	return true;

altogether and using targetm.delegitimize_address instead.  I think that's
our current interface for dealing with this kind of thing.

Thanks,
Richard