Message ID | 1484573359-7879-1-git-send-email-tuliom@linux.vnet.ibm.com |
---|---|
State | Not applicable |
Headers |
Received: (qmail 19258 invoked by alias); 16 Jan 2017 13:30:17 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 18042 invoked by uid 89); 16 Jan 2017 13:30:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=!defined, Machado, machado, H*MI:vnet X-HELO: mx0a-001b2d01.pphosted.com From: "Tulio Magno Quites Machado Filho" <tuliom@linux.vnet.ibm.com> To: libc-alpha@sourceware.org, joseph@codesourcery.com Cc: acsawdey@linux.vnet.ibm.com, segher@kernel.crashing.org, siddhesh@sourceware.org Subject: [PATCH 2.25] powerpc: Avoid calling strncmp via PLT on GCC 7 Date: Mon, 16 Jan 2017 11:29:19 -0200 X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17011613-0020-0000-0000-0000027B98F2 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17011613-0021-0000-0000-00003095E75D Message-Id: <1484573359-7879-1-git-send-email-tuliom@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-01-16_11:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1701160204 |
Commit Message
Tulio Magno Quites Machado Filho
Jan. 16, 2017, 1:29 p.m. UTC
This patch only fixes the issue on elf/check-localplt. In case you notice any other test failures when comparing strings on GCC 7, please refer to this patch: https://gcc.gnu.org/ml/gcc-patches/2017-01/msg00744.html -- 8< -- GCC 7 added support for a strncmp built-in for POWER7, generating PLT calls to strncmp from libc. 2017-01-16 Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> * sysdeps/powerpc/symbol-hacks.h: New file. Enforce strncmp calls don't go through the PLT. --- sysdeps/powerpc/symbol-hacks.h | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 sysdeps/powerpc/symbol-hacks.h
Comments
On Mon, 2017-01-16 at 11:29 -0200, Tulio Magno Quites Machado Filho wrote: > This patch only fixes the issue on elf/check-localplt. > In case you notice any other test failures when comparing strings on > GCC > 7, please refer to this patch: > https://gcc.gnu.org/ml/gcc-patches/2017-01/msg00744.html Tulio, I'll be posting an updated version of that patch shortly that addresses the issues you were seeing. However it adds builtin expansion of both strncmp and strcmp so I think your patch needs to add both to powerpc/symbol-hacks.h. Here is what I get from check-localplt with gcc7 plus this patch: cat /home/sawdey/src/glibc/build/elf/check-localplt.out Extra PLT reference: libc.so: strcmp Extra PLT reference: libc.so: strncmp Thanks, Aaron > > -- 8< -- > > GCC 7 added support for a strncmp built-in for POWER7, generating PLT > calls > to strncmp from libc. > > 2017-01-16 Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm. > com> > > * sysdeps/powerpc/symbol-hacks.h: New file. Enforce strncmp > calls don't go through the PLT. > --- > sysdeps/powerpc/symbol-hacks.h | 7 +++++++ > 1 file changed, 7 insertions(+) > create mode 100644 sysdeps/powerpc/symbol-hacks.h > > diff --git a/sysdeps/powerpc/symbol-hacks.h b/sysdeps/powerpc/symbol- > hacks.h > new file mode 100644 > index 0000000..7558b18 > --- /dev/null > +++ b/sysdeps/powerpc/symbol-hacks.h > @@ -0,0 +1,7 @@ > +#include <sysdeps/generic/symbol-hacks.h> > + > +/* GCC 7.0 added support for a builtin strncmp that is used on POWER > >= 7. */ > +#if !defined __ASSEMBLER__ && IS_IN (libc) && defined SHARED \ > + && defined _ARCH_PWR7 > +asm ("strncmp = __GI_strncmp"); > +#endif
* Tulio Magno Quites Machado Filho: > +/* GCC 7.0 added support for a builtin strncmp that is used on POWER >= 7. */ > +#if !defined __ASSEMBLER__ && IS_IN (libc) && defined SHARED \ > + && defined _ARCH_PWR7 > +asm ("strncmp = __GI_strncmp"); > +#endif Would it be safe to do this unconditionally, in the generic header?
On Mon, 16 Jan 2017, Aaron Sawdey wrote: > Tulio, > I'll be posting an updated version of that patch shortly that > addresses the issues you were seeing. However it adds builtin expansion > of both strncmp and strcmp so I think your patch needs to add both to > powerpc/symbol-hacks.h. Here is what I get from check-localplt with > gcc7 plus this patch: > > cat /home/sawdey/src/glibc/build/elf/check-localplt.out > Extra PLT reference: libc.so: strcmp > Extra PLT reference: libc.so: strncmp On further consideration: I don't think addressing this in GCC should be hard, can't you just look up something like DECL_ASSEMBLER_NAME (builtin_decl_explicit (BUILT_IN_STRNCMP)) or similar and use that instead of hardcoded strncmp when generating code?
* Joseph Myers: > On Mon, 16 Jan 2017, Aaron Sawdey wrote: > >> Tulio, >> I'll be posting an updated version of that patch shortly that >> addresses the issues you were seeing. However it adds builtin expansion >> of both strncmp and strcmp so I think your patch needs to add both to >> powerpc/symbol-hacks.h. Here is what I get from check-localplt with >> gcc7 plus this patch: >> >> cat /home/sawdey/src/glibc/build/elf/check-localplt.out >> Extra PLT reference: libc.so: strcmp >> Extra PLT reference: libc.so: strncmp > > On further consideration: I don't think addressing this in GCC should be > hard, can't you just look up something like DECL_ASSEMBLER_NAME > (builtin_decl_explicit (BUILT_IN_STRNCMP)) or similar and use that instead > of hardcoded strncmp when generating code? Right. I forgot that GCC does exactly this for other builtins. We even have a configure check: AC_CACHE_CHECK(for redirection of built-in functions, libc_cv_gcc_builtin_redirection, [dnl cat > conftest.c <<\EOF extern char *strstr (const char *, const char *) __asm ("my_strstr"); char *foo (const char *a, const char *b) { return __builtin_strstr (a, b); } EOF dnl if AC_TRY_COMMAND([${CC-cc} -O3 -S conftest.c -o - | fgrep "my_strstr" > /dev/null]); then libc_cv_gcc_builtin_redirection=yes else libc_cv_gcc_builtin_redirection=no fi rm -f conftest* ]) if test "$libc_cv_gcc_builtin_redirection" = no; then AC_MSG_ERROR([support for the symbol redirection needed]) fi So you are right that this should be fixed in GCC.
On Mon, 2017-01-16 at 23:58 +0100, Florian Weimer wrote: > * Joseph Myers: > > > On Mon, 16 Jan 2017, Aaron Sawdey wrote: > > > > > Tulio, > > > I'll be posting an updated version of that patch shortly that > > > addresses the issues you were seeing. However it adds builtin > > > expansion > > > of both strncmp and strcmp so I think your patch needs to add > > > both to > > > powerpc/symbol-hacks.h. Here is what I get from check-localplt > > > with > > > gcc7 plus this patch: > > > > > > cat /home/sawdey/src/glibc/build/elf/check-localplt.out > > > Extra PLT reference: libc.so: strcmp > > > Extra PLT reference: libc.so: strncmp > > > > On further consideration: I don't think addressing this in GCC > > should be > > hard, can't you just look up something like DECL_ASSEMBLER_NAME > > (builtin_decl_explicit (BUILT_IN_STRNCMP)) or similar and use that > > instead > > of hardcoded strncmp when generating code? I really don't see how I'm supposed to discover that I need to use __GI_strncmp in this instance. For example when I compile misc/getttyent.c with this code in the expansion function: printf("ASM: %s\n", IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (builtin_decl_explicit (BUILT_IN_STRNCMP)))); I get "ASM: strncmp". This is one of the places that is producing a plt call to strncmp. The preprocessed source has this declaration for strncmp: extern int strncmp (const char *__s1, const char *__s2, size_t __n) __attribute__ ((__nothrow__ )) __attribute__ ((__pure__)) ; I don't really see how gcc is suppose to be able to find this __GI_strncmp name here. > > Right. I forgot that GCC does exactly this for other builtins. We > even have a configure check: > > AC_CACHE_CHECK(for redirection of built-in functions, > libc_cv_gcc_builtin_redirection, [dnl > cat > conftest.c <<\EOF > extern char *strstr (const char *, const char *) __asm ("my_strstr"); > char *foo (const char *a, const char *b) > { > return __builtin_strstr (a, b); > } > EOF > dnl > if AC_TRY_COMMAND([${CC-cc} -O3 -S conftest.c -o - | fgrep > "my_strstr" > /dev/null]); > then > libc_cv_gcc_builtin_redirection=yes > else > libc_cv_gcc_builtin_redirection=no > fi > rm -f conftest* ]) > if test "$libc_cv_gcc_builtin_redirection" = no; then > AC_MSG_ERROR([support for the symbol redirection needed]) > fi This test case does work for strncmp: extern int strncmp (const char *, const char *, unsigned long) __asm ("my_strncmp"); int foo (const char *a, const char *b) { return __builtin_strncmp (a, b, 10); } In this case I get "ASM: *my_strncmp" from the debug code in the compiler when it expands strncmp. But the preprocessed code for getttyent.c does not have an __asm directive to tell me this. > > So you are right that this should be fixed in GCC. I'm starting to believe this is possible but I think the glibc code doesn't give all the information needed for this yet. Also I can't quite see why symbol-hacks.h redefinitions of memcmp/memmov/memset is needed unless the compiler is not looking up this asm name elsewhere. Aaron
On Mon, 16 Jan 2017, Aaron Sawdey wrote: > In this case I get "ASM: *my_strncmp" from the debug code in the > compiler when it expands strncmp. But the preprocessed code for > getttyent.c does not have an __asm directive to tell me this. It should have such a directive, resulting from the libc_hidden_builtin_proto (strncmp) in include/string.h (that is, there should be a first strncmp declaration without the asm, and a later one with the asm). > I'm starting to believe this is possible but I think the glibc code > doesn't give all the information needed for this yet. Also I can't > quite see why symbol-hacks.h redefinitions of memcmp/memmov/memset is > needed unless the compiler is not looking up this asm name elsewhere. Those are generated without reference to a built-in function at all, which is different from the case where a file actually calls strncmp.
Florian Weimer <fw@deneb.enyo.de> writes: > [ text/plain ] > * Tulio Magno Quites Machado Filho: > >> +/* GCC 7.0 added support for a builtin strncmp that is used on POWER >= 7. */ >> +#if !defined __ASSEMBLER__ && IS_IN (libc) && defined SHARED \ >> + && defined _ARCH_PWR7 >> +asm ("strncmp = __GI_strncmp"); >> +#endif > > Would it be safe to do this unconditionally, in the generic header? Do you mean without _ARCH_PWR7? If so, I think that's possible and in the generic header. If this issue won't be fixed in GCC, I'll make that change here.
diff --git a/sysdeps/powerpc/symbol-hacks.h b/sysdeps/powerpc/symbol-hacks.h new file mode 100644 index 0000000..7558b18 --- /dev/null +++ b/sysdeps/powerpc/symbol-hacks.h @@ -0,0 +1,7 @@ +#include <sysdeps/generic/symbol-hacks.h> + +/* GCC 7.0 added support for a builtin strncmp that is used on POWER >= 7. */ +#if !defined __ASSEMBLER__ && IS_IN (libc) && defined SHARED \ + && defined _ARCH_PWR7 +asm ("strncmp = __GI_strncmp"); +#endif