PR32219, weak hidden reference segfault

Submitted by Chung-Lin Tang on May 31, 2013, 8:13 a.m.

Details

Message ID 51A85B99.1040102@codesourcery.com
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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