From: Richard Biener <richard.guenther@gmail.com>
To: "H.J. Lu" <hjl.tools@gmail.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] PR middle-end/67220: GCC fails to properly handle libcall symbol visibility of built functions
Date: Thu, 15 Oct 2015 08:44:00 -0000 [thread overview]
Message-ID: <CAFiYyc1xY4DSo69+Yn91V-FHsmN94BX4FtY+CHScHiDqGwTs9g@mail.gmail.com> (raw)
In-Reply-To: <20151014162146.GA24833@intel.com>
On Wed, Oct 14, 2015 at 6:21 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> By default, there is no visibility on builtin functions. When there is
> explicitly declared visibility on the C library function which a builtin
> function fall back on, we should honor the explicit visibility on the
> the C library function.
>
> There are 2 issues:
>
> 1. We never update visibility of the fall back C library function.
> 2. init_block_move_fn and init_block_clear_fn used to implement builtin
> memcpy and memset generate the library call to memcpy and memset
> directly without checking if there is explicitly declared visibility on
> them.
>
> This patch updates builtin function with explicit visibility and checks
> visibility on builtin memcpy/memset when generating library call.
>
> Tested on Linux/x86-64 without regressions. OK for trunk?
Doesn't the C++ FE have the same issue?
>
> H.J.
> ---
> gcc/c/
>
> PR middle-end/67220
> * c-decl.c (diagnose_mismatched_decls): Copy explicit visibility
> to builtin function.
>
> gcc/
>
> PR middle-end/67220
> * expr.c (init_block_move_fn): Copy visibility from the builtin
> memcpy.
> (init_block_clear_fn): Copy visibility from the builtin memset.
>
> gcc/testsuite/
>
> PR middle-end/67220
> * gcc.target/i386/pr67220-1.c: New test.
> * gcc.target/i386/pr67220-2.c: Likewise.
> * gcc.target/i386/pr67220-3.c: Likewise.
> * gcc.target/i386/pr67220-4.c: Likewise.
> * gcc.target/i386/pr67220-5.c: Likewise.
> * gcc.target/i386/pr67220-6.c: Likewise.
> ---
> gcc/c/c-decl.c | 21 +++++++++++++++++----
> gcc/expr.c | 12 ++++++++++--
> gcc/testsuite/gcc.target/i386/pr67220-1.c | 15 +++++++++++++++
> gcc/testsuite/gcc.target/i386/pr67220-2.c | 15 +++++++++++++++
> gcc/testsuite/gcc.target/i386/pr67220-3.c | 15 +++++++++++++++
> gcc/testsuite/gcc.target/i386/pr67220-4.c | 15 +++++++++++++++
> gcc/testsuite/gcc.target/i386/pr67220-5.c | 14 ++++++++++++++
> gcc/testsuite/gcc.target/i386/pr67220-6.c | 14 ++++++++++++++
> 8 files changed, 115 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/i386/pr67220-1.c
> create mode 100644 gcc/testsuite/gcc.target/i386/pr67220-2.c
> create mode 100644 gcc/testsuite/gcc.target/i386/pr67220-3.c
> create mode 100644 gcc/testsuite/gcc.target/i386/pr67220-4.c
> create mode 100644 gcc/testsuite/gcc.target/i386/pr67220-5.c
> create mode 100644 gcc/testsuite/gcc.target/i386/pr67220-6.c
>
> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
> index ce8406a..26460eb 100644
> --- a/gcc/c/c-decl.c
> +++ b/gcc/c/c-decl.c
> @@ -2232,11 +2232,24 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl,
> /* warnings */
> /* All decls must agree on a visibility. */
> if (CODE_CONTAINS_STRUCT (TREE_CODE (newdecl), TS_DECL_WITH_VIS)
> - && DECL_VISIBILITY_SPECIFIED (newdecl) && DECL_VISIBILITY_SPECIFIED (olddecl)
> - && DECL_VISIBILITY (newdecl) != DECL_VISIBILITY (olddecl))
> + && DECL_VISIBILITY_SPECIFIED (newdecl))
> {
> - warned |= warning (0, "redeclaration of %q+D with different visibility "
> - "(old visibility preserved)", newdecl);
> + if (DECL_VISIBILITY_SPECIFIED (olddecl))
> + {
> + if (DECL_VISIBILITY (newdecl) != DECL_VISIBILITY (olddecl))
> + warned |= warning (0, "redeclaration of %q+D with different "
> + "visibility (old visibility preserved)",
> + newdecl);
> + }
> + else if (TREE_CODE (olddecl) == FUNCTION_DECL
> + && DECL_BUILT_IN (olddecl))
> + {
> + enum built_in_function fncode = DECL_FUNCTION_CODE (olddecl);
> + tree fndecl = builtin_decl_explicit (fncode);
> + gcc_assert (fndecl && !DECL_VISIBILITY_SPECIFIED (fndecl));
> + DECL_VISIBILITY (fndecl) = DECL_VISIBILITY (newdecl);
> + DECL_VISIBILITY_SPECIFIED (fndecl) = 1;
> + }
> }
>
> if (TREE_CODE (newdecl) == FUNCTION_DECL)
> diff --git a/gcc/expr.c b/gcc/expr.c
> index 595324d..a12db96 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -1390,7 +1390,11 @@ init_block_move_fn (const char *asmspec)
> TREE_PUBLIC (fn) = 1;
> DECL_ARTIFICIAL (fn) = 1;
> TREE_NOTHROW (fn) = 1;
> - DECL_VISIBILITY (fn) = VISIBILITY_DEFAULT;
> + tree fndecl = builtin_decl_explicit (BUILT_IN_MEMCPY);
> + if (fndecl)
> + DECL_VISIBILITY (fn) = DECL_VISIBILITY (fndecl);
> + else
> + DECL_VISIBILITY (fn) = VISIBILITY_DEFAULT;
> DECL_VISIBILITY_SPECIFIED (fn) = 1;
>
> attr_args = build_tree_list (NULL_TREE, build_string (1, "1"));
> @@ -2846,7 +2850,11 @@ init_block_clear_fn (const char *asmspec)
> TREE_PUBLIC (fn) = 1;
> DECL_ARTIFICIAL (fn) = 1;
> TREE_NOTHROW (fn) = 1;
> - DECL_VISIBILITY (fn) = VISIBILITY_DEFAULT;
> + tree fndecl = builtin_decl_explicit (BUILT_IN_MEMSET);
> + if (fndecl)
> + DECL_VISIBILITY (fn) = DECL_VISIBILITY (fndecl);
> + else
> + DECL_VISIBILITY (fn) = VISIBILITY_DEFAULT;
> DECL_VISIBILITY_SPECIFIED (fn) = 1;
>
> block_clear_fn = fn;
> diff --git a/gcc/testsuite/gcc.target/i386/pr67220-1.c b/gcc/testsuite/gcc.target/i386/pr67220-1.c
> new file mode 100644
> index 0000000..06af0ed
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr67220-1.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile { target fpic } } */
> +/* { dg-options "-O2 -fPIC" } */
> +
> +typedef __SIZE_TYPE__ size_t;
> +extern void *memcpy (void *, const void *, size_t);
> +extern void *memcpy (void *, const void *, size_t)
> + __attribute__ ((visibility ("hidden")));
> +
> +void
> +bar (void *d, void *s, size_t n)
> +{
> + memcpy (d, s, n);
> +}
> +
> +/* { dg-final { scan-assembler-not "memcpy@PLT" } } */
> diff --git a/gcc/testsuite/gcc.target/i386/pr67220-2.c b/gcc/testsuite/gcc.target/i386/pr67220-2.c
> new file mode 100644
> index 0000000..71e1d1e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr67220-2.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile { target fpic } } */
> +/* { dg-options "-O2 -fPIC" } */
> +
> +typedef __SIZE_TYPE__ size_t;
> +extern void *memset (void *, int, size_t);
> +extern void *memset (void *, int, size_t)
> + __attribute__ ((visibility ("hidden")));
> +
> +void
> +bar (void *s, size_t n)
> +{
> + memset (s, '\0', n);
> +}
> +
> +/* { dg-final { scan-assembler-not "memset@PLT" } } */
> diff --git a/gcc/testsuite/gcc.target/i386/pr67220-3.c b/gcc/testsuite/gcc.target/i386/pr67220-3.c
> new file mode 100644
> index 0000000..fa54530
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr67220-3.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile { target fpic } } */
> +/* { dg-options "-O2 -fPIC" } */
> +
> +typedef __SIZE_TYPE__ size_t;
> +extern void *memcpy (void *, const void *, size_t)
> + __attribute__ ((visibility ("hidden")));
> +extern void *memcpy (void *, const void *, size_t);
> +
> +void
> +bar (void *d, void *s, size_t n)
> +{
> + __builtin_memcpy (d, s, n);
> +}
> +
> +/* { dg-final { scan-assembler-not "memcpy@PLT" } } */
> diff --git a/gcc/testsuite/gcc.target/i386/pr67220-4.c b/gcc/testsuite/gcc.target/i386/pr67220-4.c
> new file mode 100644
> index 0000000..9ce02f4
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr67220-4.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile { target fpic } } */
> +/* { dg-options "-O2 -fPIC" } */
> +
> +typedef __SIZE_TYPE__ size_t;
> +extern void *memset (void *, int, size_t)
> + __attribute__ ((visibility ("hidden")));
> +extern void *memset (void *, int, size_t);
> +
> +void
> +bar (void *s, size_t n)
> +{
> + __builtin_memset (s, '\0', n);
> +}
> +
> +/* { dg-final { scan-assembler-not "memset@PLT" } } */
> diff --git a/gcc/testsuite/gcc.target/i386/pr67220-5.c b/gcc/testsuite/gcc.target/i386/pr67220-5.c
> new file mode 100644
> index 0000000..e2c8ba3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr67220-5.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile { target fpic } } */
> +/* { dg-options "-O2 -fPIC" } */
> +
> +extern int strcmp (const char *, const char *);
> +extern int strcmp (const char *, const char *)
> + __attribute__ ((visibility ("hidden")));
> +
> +int
> +bar (const char *d, const char *s)
> +{
> + return __builtin_strcmp (d, s);
> +}
> +
> +/* { dg-final { scan-assembler-not "strcmp@PLT" } } */
> diff --git a/gcc/testsuite/gcc.target/i386/pr67220-6.c b/gcc/testsuite/gcc.target/i386/pr67220-6.c
> new file mode 100644
> index 0000000..c83d4c2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr67220-6.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile { target fpic } } */
> +/* { dg-options "-O2 -fPIC" } */
> +
> +extern int strcmp (const char *, const char *)
> + __attribute__ ((visibility ("hidden")));
> +extern int strcmp (const char *, const char *);
> +
> +int
> +bar (const char *d, const char *s)
> +{
> + return __builtin_strcmp (d, s);
> +}
> +
> +/* { dg-final { scan-assembler-not "strcmp@PLT" } } */
> --
> 2.4.3
>
next prev parent reply other threads:[~2015-10-15 8:44 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-14 16:21 H.J. Lu
2015-10-14 16:46 ` Ramana Radhakrishnan
2015-10-14 16:51 ` H.J. Lu
2015-10-14 17:17 ` Ramana Radhakrishnan
2015-10-15 8:44 ` Richard Biener [this message]
2015-10-15 10:38 ` H.J. Lu
2015-10-20 23:43 ` Bernd Schmidt
2015-10-21 3:42 ` H.J. Lu
2016-01-15 18:44 ` H.J. Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc1xY4DSo69+Yn91V-FHsmN94BX4FtY+CHScHiDqGwTs9g@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hjl.tools@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).