Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
On 3/18/20 1:04 PM, Jakub Jelinek wrote: On Wed, Mar 18, 2020 at 12:57:18PM -0600, Martin Sebor via Gcc-patches wrote: I noticed this last night: https://sourceware.org/pipermail/glibc-cvs/2020q1/069150.html Presumably that's the fix. Or maybe for REAL_TYPE just care here about TYPE_MODE which should be all that matters? If double and long double are the same, it isn't a big deal. And similarly for INTEGER_TYPEs only care about TYPE_MODE/TYPE_PRECISION? If unsigned long and unsigned long long are the same, why should we care? There are a few reasons why diagnosing incompatible declarations (of built-ins or any other kind) is helpful even for same size types. First, -Wbuiltin-declaration-mismatch is documented to "warn if a built-in function is declared with an incompatible signature." Distinct types like unsigned long and long long are incompatible and redeclaring functions with incompatible argument types (or return types) makes their signatures incompatible, and the code undefined. Detecting incompatibilities is the purpose of the warning, irrespective of whether or not a subset of them might be considered "big deal" in some situations[*]. Second, the TYPE_MODE test isn't sufficient to discriminate between signed and unsigned types with the same precision, and those can cause subtle bugs to go undetected. Third, built-in type mismatches tend to cause us headaches (such as ICEs) in parts of the compiler (the middle-end or other parts of the front ends) that are updated with the assumption of type compatibility in the C/C++ sense. They are an unnecessary gotcha to keep in mind that's easy to forget about or not get exactly right and that leads to wasted resources: users or testers reporting them as bugs and developers fixing them at the end of each release. The overly loose matching based on TYPE_MODE was in place before GCC 8. Since then, we have been tightening up these checks. It would be a step backward to change direction and start encouraging sloppy code, glossing over latent bugs, and exposing ourselves to more reports of ICEs. Specialized projects like Glibc that have a legitimate need for declaring symbols with incompatible types have the option to disable either the warnings or the built-ins themselves. Nothing indicates that this practice is commonplace or that GCC is too strict (it is still more permissive than the same option in Clang that predates GCC's by a number of years.) Martin [*] As a precedent for warning on similarly "benign" mismatches consider issuing -Wformat when passing same size arguments to directives like %i, %li, and %lli, or (for issuing "portability" warnings for code that's fully valid and safe for the current target), -Wchar-subscripts when -fno-signed-char is in effect.
Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
On Wed, Mar 18, 2020 at 12:57:18PM -0600, Martin Sebor via Gcc-patches wrote: > I noticed this last night: > > https://sourceware.org/pipermail/glibc-cvs/2020q1/069150.html > > Presumably that's the fix. Or maybe for REAL_TYPE just care here about TYPE_MODE which should be all that matters? If double and long double are the same, it isn't a big deal. And similarly for INTEGER_TYPEs only care about TYPE_MODE/TYPE_PRECISION? If unsigned long and unsigned long long are the same, why should we care? Jakub
Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
On 3/18/20 8:30 AM, Jeff Law wrote:
On Wed, 2020-03-18 at 14:25 +, Szabolcs Nagy wrote:
The 03/13/2020 10:45, Martin Sebor via Gcc-patches wrote:
On 3/12/20 7:17 PM, Joseph Myers wrote:
On Thu, 5 Mar 2020, Martin Sebor wrote:
Tested on x86_64-linux. Is this acceptable for GCC 10? How about 9?
OK for GCC 10.
Thank you. I committed it to trunk in r10-7162.
arm glibc build fails for me since this commit.
../sysdeps/ieee754/dbl-64/s_modf.c:84:28: error: conflicting types for built-in
function 'modfl'; expected 'long double(long double, long double *)' [-
Werror=builtin-declaration-mismatch]
84 | libm_alias_double (__modf, modf)
|^~~~
it seems this used to compile but not any more:
double modf (double x, double *p) { return x; }
extern __typeof (modf) modfl __attribute__ ((weak, alias ("modf")))
__attribute__ ((__copy__ (modf)));
I think Joseph posted something this morning that might fix this.
I noticed this last night:
https://sourceware.org/pipermail/glibc-cvs/2020q1/069150.html
Presumably that's the fix.
Martin
Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
On Wed, 2020-03-18 at 14:25 +, Szabolcs Nagy wrote:
> The 03/13/2020 10:45, Martin Sebor via Gcc-patches wrote:
> > On 3/12/20 7:17 PM, Joseph Myers wrote:
> > > On Thu, 5 Mar 2020, Martin Sebor wrote:
> > >
> > > > Tested on x86_64-linux. Is this acceptable for GCC 10? How about 9?
> > >
> > > OK for GCC 10.
> >
> > Thank you. I committed it to trunk in r10-7162.
>
> arm glibc build fails for me since this commit.
>
> ../sysdeps/ieee754/dbl-64/s_modf.c:84:28: error: conflicting types for
> built-in
> function 'modfl'; expected 'long double(long double, long double *)' [-
> Werror=builtin-declaration-mismatch]
>84 | libm_alias_double (__modf, modf)
> |^~~~
>
> it seems this used to compile but not any more:
>
> double modf (double x, double *p) { return x; }
> extern __typeof (modf) modfl __attribute__ ((weak, alias ("modf")))
> __attribute__ ((__copy__ (modf)));
I think Joseph posted something this morning that might fix this.
jeff
Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
The 03/13/2020 10:45, Martin Sebor via Gcc-patches wrote:
> On 3/12/20 7:17 PM, Joseph Myers wrote:
> > On Thu, 5 Mar 2020, Martin Sebor wrote:
> >
> > > Tested on x86_64-linux. Is this acceptable for GCC 10? How about 9?
> >
> > OK for GCC 10.
>
> Thank you. I committed it to trunk in r10-7162.
i see glibc build failure on arm since this commit:
../sysdeps/ieee754/dbl-64/s_modf.c:84:28: error: conflicting types for built-in
function 'modfl'; expected 'long double(long double, long double *)'
[-Werror=builtin-declaration-mismatch]
84 | libm_alias_double (__modf, modf)
|^~~~
it seems this used to compile:
double modf (double x, double *p) { return x; }
extern __typeof (modf) modfl __attribute__ ((weak, alias ("modf")))
__attribute__ ((__copy__ (modf)));
Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
The 03/13/2020 10:45, Martin Sebor via Gcc-patches wrote:
> On 3/12/20 7:17 PM, Joseph Myers wrote:
> > On Thu, 5 Mar 2020, Martin Sebor wrote:
> >
> > > Tested on x86_64-linux. Is this acceptable for GCC 10? How about 9?
> >
> > OK for GCC 10.
>
> Thank you. I committed it to trunk in r10-7162.
arm glibc build fails for me since this commit.
../sysdeps/ieee754/dbl-64/s_modf.c:84:28: error: conflicting types for built-in
function 'modfl'; expected 'long double(long double, long double *)'
[-Werror=builtin-declaration-mismatch]
84 | libm_alias_double (__modf, modf)
|^~~~
it seems this used to compile but not any more:
double modf (double x, double *p) { return x; }
extern __typeof (modf) modfl __attribute__ ((weak, alias ("modf")))
__attribute__ ((__copy__ (modf)));
Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
On Fri, 13 Mar 2020, Martin Sebor via Gcc-patches wrote: > On 3/12/20 7:17 PM, Joseph Myers wrote: > > On Thu, 5 Mar 2020, Martin Sebor wrote: > > > > > Tested on x86_64-linux. Is this acceptable for GCC 10? How about 9? > > > > OK for GCC 10. > > Thank you. I committed it to trunk in r10-7162. > > Do you not want me to commit it to GCC 9 or are you leaving it up to me? I don't think this is that safe for GCC 9. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
On 3/6/20 2:11 AM, Richard Biener wrote:
On Fri, Mar 6, 2020 at 2:04 AM Martin Sebor wrote:
Treating incompatible redeclarations of built-in functions as built-ins
is a problem not just for the middle-end but even for the C front-end
itself, when different parts of it make different assumptions about
what is and isn't valid. The test case that is the subject of this
bug report (a GCC 9 and 10 regression) is one such example: it shows
that the attribute format validation assumes the function declaration
the attribute applies to has passed the prerequisite validation. But
that's not the case when the function is an incompatibly redeclared
built-in where a format attribute's positional argument refers to
parameter of an invalid/nonsensical type.
The attached patch further adjusts the front-end to consider even more
incompatible redeclarations as built-ins: in particular, redeclarations
whose pointer arguments point to incompatible variants of unqualified
types (e.g., char* vs int*, though not char* vs const char*).
Besides avoiding the front-end and some middle-end ICEs, the effect
of the patch is also to diagnose more incompatible redeclarations
of built-ins than before, but fewer invalid calls to such functions
(since they're no longer considered built-ins). That seems like
an unavoidable trade-off.
Tested on x86_64-linux. Is this acceptable for GCC 10? How about 9?
The actual patch needs reviewing from a FE maintainer but I'd support
putting this in for GCC 10.
It would be nice if we can put in code like the following to catch "bogus"
built-in declarations. I've put it in call verification because there it's
where it matters most, free-lang-data would be another canonical place
which would then process all "reachable" function declarations but we
don't yet call free-lang-data when not doing LTO.
I committed the patch to trunk as it was but I wasn't brave enough
to also add this. I have tested it though and it fails in pr89998*.c
which declares sprintf to return unsigned int while the built-in has
it return signed in. GCC does diagnose these mismatches but only
with -Wextra and it still accepts such redeclarations as benign.
The patch also fails in c-c++-common/dfp/signbit-2.c which declares
int signbit (double) while GCC declares it as a variadic function.
(There a few more similar failures like this.)
I'm sure these expected mismatches can be dealt with and I'm up for
tightening up the checking even more and requiring exact matches but
I'd rather not do it at this stage.
Martin
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index f7b817d94e6..ae695891bd4 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -3356,6 +3356,17 @@ verify_gimple_call (gcall *stmt)
return true;
}
+ if (fndecl
+ && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)
+ && !types_compatible_p (TREE_TYPE (fndecl),
+ TREE_TYPE (builtin_decl_explicit
+ (DECL_FUNCTION_CODE (fndecl)
+{
+ error ("function declaration declared built-in but does not "
+"match expected type");
+ return true;
+}
+
tree lhs = gimple_call_lhs (stmt);
if (lhs
&& (!is_gimple_lvalue (lhs)
Martin
Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
On 3/12/20 7:17 PM, Joseph Myers wrote: On Thu, 5 Mar 2020, Martin Sebor wrote: Tested on x86_64-linux. Is this acceptable for GCC 10? How about 9? OK for GCC 10. Thank you. I committed it to trunk in r10-7162. Do you not want me to commit it to GCC 9 or are you leaving it up to me? Martin
Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
On Thu, 5 Mar 2020, Martin Sebor wrote: > Tested on x86_64-linux. Is this acceptable for GCC 10? How about 9? OK for GCC 10. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
On Fri, Mar 6, 2020 at 2:04 AM Martin Sebor wrote:
>
> Treating incompatible redeclarations of built-in functions as built-ins
> is a problem not just for the middle-end but even for the C front-end
> itself, when different parts of it make different assumptions about
> what is and isn't valid. The test case that is the subject of this
> bug report (a GCC 9 and 10 regression) is one such example: it shows
> that the attribute format validation assumes the function declaration
> the attribute applies to has passed the prerequisite validation. But
> that's not the case when the function is an incompatibly redeclared
> built-in where a format attribute's positional argument refers to
> parameter of an invalid/nonsensical type.
>
> The attached patch further adjusts the front-end to consider even more
> incompatible redeclarations as built-ins: in particular, redeclarations
> whose pointer arguments point to incompatible variants of unqualified
> types (e.g., char* vs int*, though not char* vs const char*).
>
> Besides avoiding the front-end and some middle-end ICEs, the effect
> of the patch is also to diagnose more incompatible redeclarations
> of built-ins than before, but fewer invalid calls to such functions
> (since they're no longer considered built-ins). That seems like
> an unavoidable trade-off.
>
> Tested on x86_64-linux. Is this acceptable for GCC 10? How about 9?
The actual patch needs reviewing from a FE maintainer but I'd support
putting this in for GCC 10.
It would be nice if we can put in code like the following to catch "bogus"
built-in declarations. I've put it in call verification because there it's
where it matters most, free-lang-data would be another canonical place
which would then process all "reachable" function declarations but we
don't yet call free-lang-data when not doing LTO.
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index f7b817d94e6..ae695891bd4 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -3356,6 +3356,17 @@ verify_gimple_call (gcall *stmt)
return true;
}
+ if (fndecl
+ && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)
+ && !types_compatible_p (TREE_TYPE (fndecl),
+ TREE_TYPE (builtin_decl_explicit
+ (DECL_FUNCTION_CODE (fndecl)
+{
+ error ("function declaration declared built-in but does not "
+"match expected type");
+ return true;
+}
+
tree lhs = gimple_call_lhs (stmt);
if (lhs
&& (!is_gimple_lvalue (lhs)
> Martin
[PATCH] avoid treating more incompatible redeclarations as builtin-ins [PR94040]
Treating incompatible redeclarations of built-in functions as built-ins
is a problem not just for the middle-end but even for the C front-end
itself, when different parts of it make different assumptions about
what is and isn't valid. The test case that is the subject of this
bug report (a GCC 9 and 10 regression) is one such example: it shows
that the attribute format validation assumes the function declaration
the attribute applies to has passed the prerequisite validation. But
that's not the case when the function is an incompatibly redeclared
built-in where a format attribute's positional argument refers to
parameter of an invalid/nonsensical type.
The attached patch further adjusts the front-end to consider even more
incompatible redeclarations as built-ins: in particular, redeclarations
whose pointer arguments point to incompatible variants of unqualified
types (e.g., char* vs int*, though not char* vs const char*).
Besides avoiding the front-end and some middle-end ICEs, the effect
of the patch is also to diagnose more incompatible redeclarations
of built-ins than before, but fewer invalid calls to such functions
(since they're no longer considered built-ins). That seems like
an unavoidable trade-off.
Tested on x86_64-linux. Is this acceptable for GCC 10? How about 9?
Martin
PR c/94040 - ICE on a call to an invalid redeclaration of strftime
gcc/c/ChangeLog:
PR c/94040
* c-decl.c (builtin_structptr_type_count): New constant.
(match_builtin_function_types): Reject decls that are incompatible
in types pointed to by pointers.
(diagnose_mismatched_decls): Adjust comments.
gcc/testsuite/ChangeLog:
PR c/94040
* gcc.dg/Wbuiltin-declaration-mismatch-12.c: Relax test to look
for warning name rather than the exact text.
* gcc.dg/Wbuiltin-declaration-mismatch-14.c: New test.
* gcc.dg/Wbuiltin-declaration-mismatch-15.c: New test.
* gcc.dg/pr62090.c: Prune expected warning.
* gcc.dg/pr89314.c: Look for warning name rather than text.
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index c819fd0d0d5..87a0734b715 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -1641,13 +1641,17 @@ c_bind (location_t loc, tree decl, bool is_global)
}
-/* Stores the first FILE*, const struct tm* etc. argument type (whatever it
- is) seen in a declaration of a file I/O etc. built-in. Subsequent
- declarations of such built-ins are expected to refer to it rather than to
- fileptr_type_node etc. which is just void* (or to any other type).
+/* Stores the first FILE*, const struct tm* etc. argument type (whatever
+ it is) seen in a declaration of a file I/O etc. built-in, corresponding
+ to the builtin_structptr_types array. Subsequent declarations of such
+ built-ins are expected to refer to it rather than to fileptr_type_node,
+ etc. which is just void* (or to any other type).
Used only by match_builtin_function_types. */
-static GTY(()) tree last_structptr_types[6];
+static const unsigned builtin_structptr_type_count
+ = sizeof builtin_structptr_types / sizeof builtin_structptr_types[0];
+
+static GTY(()) tree last_structptr_types[builtin_structptr_type_count];
/* Returns true if types T1 and T2 representing return types or types
of function arguments are close enough to be considered interchangeable
@@ -1692,10 +1696,13 @@ match_builtin_function_types (tree newtype, tree oldtype,
tree newargs = TYPE_ARG_TYPES (newtype);
tree tryargs = newargs;
- gcc_checking_assert ((sizeof (last_structptr_types)
- / sizeof (last_structptr_types[0]))
- == (sizeof (builtin_structptr_types)
- / sizeof (builtin_structptr_types[0])));
+ const unsigned nlst
+= sizeof last_structptr_types / sizeof last_structptr_types[0];
+ const unsigned nbst
+= sizeof builtin_structptr_types / sizeof builtin_structptr_types[0];
+
+ gcc_checking_assert (nlst == nbst);
+
for (unsigned i = 1; oldargs || newargs; ++i)
{
if (!oldargs
@@ -1710,11 +1717,12 @@ match_builtin_function_types (tree newtype, tree oldtype,
if (!types_close_enough_to_match (oldtype, newtype))
return NULL_TREE;
- unsigned j = (sizeof (builtin_structptr_types)
- / sizeof (builtin_structptr_types[0]));
+ unsigned j = nbst;
if (POINTER_TYPE_P (oldtype))
- for (j = 0; j < (sizeof (builtin_structptr_types)
- / sizeof (builtin_structptr_types[0])); ++j)
+ /* Iterate over well-known struct types like FILE (whose types
+ aren't known to us) and compare the pointer to each to
+ the pointer argument. */
+ for (j = 0; j < nbst; ++j)
{
if (TREE_VALUE (oldargs) != builtin_structptr_types[j].node)
continue;
@@ -1734,13 +1742,26 @@ match_builtin_function_types (tree newtype, tree oldtype,
last_structptr_types[j] = newtype;
break;
}
- if (j == (sizeof (builtin_structptr_types)
- / sizeof (builtin_structptr_types[0]))
- && !*strict
- && !comptypes (oldtype, newtype))
+
+ if (j == nbst && !comptypes (oldtype,
