Patchwork PR32219, weak hidden reference segfault

login
register
mail settings
Submitter Chung-Lin Tang
Date May 31, 2013, 8:13 a.m.
Message ID <51A85B99.1040102@codesourcery.com>
Download mbox | patch
Permalink /patch/247859/
State New
Headers show

Comments

Chung-Lin Tang - May 31, 2013, 8:13 a.m.
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
aldot - June 11, 2013, 9:20 a.m.
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
>
Chung-Lin Tang - June 20, 2013, 7:01 a.m.
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
>>
Chung-Lin Tang - July 14, 2013, 9:08 a.m.
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
>>>
>
Diego Novillo - July 14, 2013, 5:43 p.m.
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.
aldot - Aug. 1, 2013, 9:16 a.m.
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.
Chung-Lin Tang - Aug. 1, 2013, 9:19 a.m.
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.

Patch

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;
+}