| Message ID | 20151014162146.GA24833@intel.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 14, 2015 at 5:21 PM, H.J. Lu <hongjiu.lu@intel.com> wrote: > --- > 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. Why aren't these tests in gcc.dg ? The problem affects all targets not just x86. Thanks, Ramana > --- > 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 >
On Wed, Oct 14, 2015 at 9:46 AM, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > On Wed, Oct 14, 2015 at 5:21 PM, H.J. Lu <hongjiu.lu@intel.com> wrote: > >> --- >> 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. > > Why aren't these tests in gcc.dg ? The problem affects all targets > not just x86. > If I move tests to gcc.dg, would you mind updating them to verify that they pass on arm?
On Wed, Oct 14, 2015 at 5:51 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Wed, Oct 14, 2015 at 9:46 AM, Ramana Radhakrishnan > <ramana.gcc@googlemail.com> wrote: >> On Wed, Oct 14, 2015 at 5:21 PM, H.J. Lu <hongjiu.lu@intel.com> wrote: >> >>> --- >>> 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. >> >> Why aren't these tests in gcc.dg ? The problem affects all targets >> not just x86. >> > > If I move tests to gcc.dg, would you mind updating them to verify > that they pass on arm? It's not just a question of ARM. This affects all targets that support symbol visibility in shared libraries ... please do the math as to how many targets are affected. Now reading the test even more, it appears that you also need a dg-require-visibility so that this test is run on all targets that support symbol visibility. The test as written uses target_fpic - target-supports.exp does not have anything ARM specific about handling this - there is some m68k , so it should just work on any target that supports symbol visibility. If it doesn't, that target has an issue and then folks interested in that target will do something about it. Even if you don't fix the issue on every target, please have the courtesy of letting them find it in some sort of automatic manner rather than auditing every single commit in every gcc.target directory. regards Ramana > > > -- > H.J.
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 >
On Thu, Oct 15, 2015 at 1:44 AM, Richard Biener <richard.guenther@gmail.com> wrote: > 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? > Unlike gcc, visibility triggers a warning in g++: [hjl@gnu-tools-1 pr67220]$ cat memcpy.i typedef unsigned long size_t; extern void *memcpy(void *dest, const void *src, size_t n) __attribute__ ((visibility ("hidden"))); void bar (void *d, void *s, size_t n) { memcpy (d, s, n); } [hjl@gnu-tools-1 pr67220]$ make memcpy.s /export/build/gnu/gcc/build-x86_64-linux/gcc/xg++ -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -O2 -fPIC -S memcpy.i memcpy.i:2:14: warning: ‘void* memcpy(void*, const void*, size_t)’: visibility attribute ignored because it conflicts with previous declaration [-Wattributes] extern void *memcpy(void *dest, const void *src, size_t n) ^ <built-in>: note: previous declaration of ‘void* memcpy(void*, const void*, size_t)’ [hjl@gnu-tools-1 pr67220]$ Since those fallback functions are in the C library, it is very unlikely they can be hidden in C++. They are mostly likely to happen inside of the C library.
On 10/15/2015 12:37 PM, H.J. Lu wrote: > On Thu, Oct 15, 2015 at 1:44 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> 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. >> Doesn't the C++ FE have the same issue? >> > > Unlike gcc, visibility triggers a warning in g++: > > memcpy.i:2:14: warning: ‘void* memcpy(void*, const void*, size_t)’: > visibility attribute ignored because it conflicts with previous > declaration [-Wattributes] > extern void *memcpy(void *dest, const void *src, size_t n) > ^ > <built-in>: note: previous declaration of ‘void* memcpy(void*, const > void*, size_t)’ > [hjl@gnu-tools-1 pr67220]$ I see no good reason for C and C++ to have different behaviour here. It looks like the C++ frontend sets DECL_VISIBILITY_SPECIFIED to 1 for builtins, causing the above behaviour. Cc'ing Jason, but I think the C++ frontend should be changed not to set D_V_S and have the same changes as the C frontend for merging the visibilities. Other than that I don't see a problem with the concept. However, I also agree that the tests should not be i386 specific. One final question - it would seem that glibc is currently not affected by this problem (at least I'm not seeing memcpy@plt calls in the binary on my system), so how come this has become an issue now? Bernd
On Tue, Oct 20, 2015 at 4:37 PM, Bernd Schmidt <bschmidt@redhat.com> wrote: > On 10/15/2015 12:37 PM, H.J. Lu wrote: >> >> On Thu, Oct 15, 2015 at 1:44 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> >>> 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. > > >>> Doesn't the C++ FE have the same issue? >>> >> >> Unlike gcc, visibility triggers a warning in g++: >> >> memcpy.i:2:14: warning: ‘void* memcpy(void*, const void*, size_t)’: >> visibility attribute ignored because it conflicts with previous >> declaration [-Wattributes] >> extern void *memcpy(void *dest, const void *src, size_t n) >> ^ >> <built-in>: note: previous declaration of ‘void* memcpy(void*, const >> void*, size_t)’ >> [hjl@gnu-tools-1 pr67220]$ > > > I see no good reason for C and C++ to have different behaviour here. It > looks like the C++ frontend sets DECL_VISIBILITY_SPECIFIED to 1 for > builtins, causing the above behaviour. Cc'ing Jason, but I think the C++ > frontend should be changed not to set D_V_S and have the same changes as the > C frontend for merging the visibilities. > > Other than that I don't see a problem with the concept. However, I also > agree that the tests should not be i386 specific. Sure. Just add target-specific scan-assembler-not. > One final question - it would seem that glibc is currently not affected by > this problem (at least I'm not seeing memcpy@plt calls in the binary on my > system), so how come this has become an issue now? > > R_386_PLT32 only shows in .o files. There are many of them in libc_pic.os.
On Tue, Oct 20, 2015 at 4:37 PM, Bernd Schmidt <bschmidt@redhat.com> wrote: > On 10/15/2015 12:37 PM, H.J. Lu wrote: >> >> On Thu, Oct 15, 2015 at 1:44 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> >>> 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. > > >>> Doesn't the C++ FE have the same issue? >>> >> >> Unlike gcc, visibility triggers a warning in g++: >> >> memcpy.i:2:14: warning: ‘void* memcpy(void*, const void*, size_t)’: >> visibility attribute ignored because it conflicts with previous >> declaration [-Wattributes] >> extern void *memcpy(void *dest, const void *src, size_t n) >> ^ >> <built-in>: note: previous declaration of ‘void* memcpy(void*, const >> void*, size_t)’ >> [hjl@gnu-tools-1 pr67220]$ > > > I see no good reason for C and C++ to have different behaviour here. It > looks like the C++ frontend sets DECL_VISIBILITY_SPECIFIED to 1 for > builtins, causing the above behaviour. Cc'ing Jason, but I think the C++ > frontend should be changed not to set D_V_S and have the same changes as the > C frontend for merging the visibilities. > What should we do with C++ front-end?
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" } } */