| Message ID | 51A85B99.1040102@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >>
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 >>> >
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.
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.
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.
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; +}