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