Patchwork PR32219, weak hidden reference segfault

login
register
mail settings
Submitter Chung-Lin Tang
Date May 15, 2013, 10:11 a.m.
Message ID <51935F65.4000803@codesourcery.com>
Download mbox | patch
Permalink /patch/243963/
State New
Headers show

Comments

Chung-Lin Tang - May 15, 2013, 10:11 a.m.
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
Richard Sandiford - May 15, 2013, 12:12 p.m.
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

Patch

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: