* [C++ PATCH] Don't redefine __* builtins (PR c++/84724)
@ 2018-03-08 18:35 Jakub Jelinek
2018-03-08 21:06 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2018-03-08 18:35 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
Hi!
The C FE just warns and doesn't override builtins, but C++ FE
on say int __builtin_trap (); will override the builtin, so later
builtin_decl_explicit will return the bogus user function which e.g.
doesn't have any merged attributes, can have different arguments or
return type etc.
This patch prevents that; generally the builtins we want to override
are the DECL_ANTICIPATED which we handle properly earlier.
The testcase in the PR ICEs not because of the argument mismatch (there is
none in this case) or return value mismatch (because nothing cares about the
return value), but because the new decl lacks noreturn attribute and GCC
relies on builtin_decl_explicit (BUILT_IN_TRAP) to be really noreturn.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Or shall we error on these, or ignore the name checks and just
if (DECL_BUILT_IN (olddecl)) return NULL_TREE;, something else?
2018-03-08 Jakub Jelinek <jakub@redhat.com>
PR c++/84724
* decl.c (duplicate_decls): Don't override __* prefixed builtins
except for __[^b]*_chk.
* g++.dg/ext/pr84724.C: New test.
--- gcc/cp/decl.c.jj 2018-03-05 17:23:45.901130856 +0100
+++ gcc/cp/decl.c 2018-03-08 10:28:55.574179119 +0100
@@ -1578,6 +1578,28 @@ next_arg:;
DECL_BUILT_IN (olddecl)
? G_("shadowing built-in function %q#D")
: G_("shadowing library function %q#D"), olddecl);
+ /* Don't really override olddecl for __* prefixed builtins
+ except for __[^b]*_chk, the compiler might be using those
+ explicitly. */
+ if (DECL_BUILT_IN (olddecl))
+ {
+ tree id = DECL_NAME (olddecl);
+ const char *name = IDENTIFIER_POINTER (id);
+
+ if (name[0] == '_' && name[1] == '_')
+ {
+ if (strncmp (name + 2, "builtin_",
+ strlen ("builtin_")) == 0)
+ return NULL_TREE;
+
+ size_t len = strlen (name);
+
+ if (len <= strlen ("___chk")
+ || memcmp (name + len - strlen ("_chk"),
+ "_chk", strlen ("_chk") + 1) != 0)
+ return NULL_TREE;
+ }
+ }
}
else
/* Discard the old built-in function. */
--- gcc/testsuite/g++.dg/ext/pr84724.C.jj 2018-03-08 10:31:03.683183914 +0100
+++ gcc/testsuite/g++.dg/ext/pr84724.C 2018-03-08 10:30:45.489183233 +0100
@@ -0,0 +1,13 @@
+// PR c++/84724
+// { dg-do compile }
+// { dg-options "-O3 -fpermissive" }
+
+int __builtin_trap (); // { dg-warning "ambiguates built-in declaration" }
+
+int
+foo ()
+{
+ int b;
+ int c (&b); // { dg-warning "invalid conversion from" }
+ return b %= b ? c : 0;
+}
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [C++ PATCH] Don't redefine __* builtins (PR c++/84724)
2018-03-08 18:35 [C++ PATCH] Don't redefine __* builtins (PR c++/84724) Jakub Jelinek
@ 2018-03-08 21:06 ` Jason Merrill
2018-03-09 12:35 ` [C++ PATCH] Don't redefine __* builtins (PR c++/84724, take 2) Jakub Jelinek
0 siblings, 1 reply; 4+ messages in thread
From: Jason Merrill @ 2018-03-08 21:06 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches List
On Thu, Mar 8, 2018 at 1:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> The C FE just warns and doesn't override builtins, but C++ FE
> on say int __builtin_trap (); will override the builtin, so later
> builtin_decl_explicit will return the bogus user function which e.g.
> doesn't have any merged attributes, can have different arguments or
> return type etc.
>
> This patch prevents that; generally the builtins we want to override
> are the DECL_ANTICIPATED which we handle properly earlier.
>
> The testcase in the PR ICEs not because of the argument mismatch (there is
> none in this case) or return value mismatch (because nothing cares about the
> return value), but because the new decl lacks noreturn attribute and GCC
> relies on builtin_decl_explicit (BUILT_IN_TRAP) to be really noreturn.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> Or shall we error on these, or ignore the name checks and just
> if (DECL_BUILT_IN (olddecl)) return NULL_TREE;, something else?
Seems like returning NULL_TREE means that we overload the built-in,
which also seems undesirable; what if we return olddecl unmodified?
It would also be good to improve the diagnostic to say that we're
ignoring the declaration. Perhaps as a permerror.
Jason
^ permalink raw reply [flat|nested] 4+ messages in thread
* [C++ PATCH] Don't redefine __* builtins (PR c++/84724, take 2)
2018-03-08 21:06 ` Jason Merrill
@ 2018-03-09 12:35 ` Jakub Jelinek
2018-03-09 16:13 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2018-03-09 12:35 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches List
On Thu, Mar 08, 2018 at 04:06:24PM -0500, Jason Merrill wrote:
> On Thu, Mar 8, 2018 at 1:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > The C FE just warns and doesn't override builtins, but C++ FE
> > on say int __builtin_trap (); will override the builtin, so later
> > builtin_decl_explicit will return the bogus user function which e.g.
> > doesn't have any merged attributes, can have different arguments or
> > return type etc.
> >
> > This patch prevents that; generally the builtins we want to override
> > are the DECL_ANTICIPATED which we handle properly earlier.
> >
> > The testcase in the PR ICEs not because of the argument mismatch (there is
> > none in this case) or return value mismatch (because nothing cares about the
> > return value), but because the new decl lacks noreturn attribute and GCC
> > relies on builtin_decl_explicit (BUILT_IN_TRAP) to be really noreturn.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > Or shall we error on these, or ignore the name checks and just
> > if (DECL_BUILT_IN (olddecl)) return NULL_TREE;, something else?
>
> Seems like returning NULL_TREE means that we overload the built-in,
> which also seems undesirable; what if we return olddecl unmodified?
> It would also be good to improve the diagnostic to say that we're
> ignoring the declaration. Perhaps as a permerror.
So like this (if it passes bootstrap/regtest, though I think we don't have
any tests other than these new ones that cover it anyway, so I don't expect
issues)?
2018-03-09 Jakub Jelinek <jakub@redhat.com>
PR c++/84724
* decl.c (duplicate_decls): Don't override __* prefixed builtins
except for __[^b]*_chk, instead issue permerror and for -fpermissive
also a note and return olddecl.
* g++.dg/ext/pr84724.C: New test.
--- gcc/cp/decl.c.jj 2018-03-08 21:53:44.989559552 +0100
+++ gcc/cp/decl.c 2018-03-09 11:53:58.557156637 +0100
@@ -1566,6 +1566,33 @@ next_arg:;
|| compparms (TYPE_ARG_TYPES (TREE_TYPE (newdecl)),
TYPE_ARG_TYPES (TREE_TYPE (olddecl))))
{
+ /* Don't really override olddecl for __* prefixed builtins
+ except for __[^b]*_chk, the compiler might be using those
+ explicitly. */
+ if (DECL_BUILT_IN (olddecl))
+ {
+ tree id = DECL_NAME (olddecl);
+ const char *name = IDENTIFIER_POINTER (id);
+ size_t len;
+
+ if (name[0] == '_'
+ && name[1] == '_'
+ && (strncmp (name + 2, "builtin_",
+ strlen ("builtin_")) == 0
+ || (len = strlen (name)) <= strlen ("___chk")
+ || memcmp (name + len - strlen ("_chk"),
+ "_chk", strlen ("_chk") + 1) != 0))
+ {
+ if (permerror (DECL_SOURCE_LOCATION (newdecl),
+ "new declaration %q#D ambiguates built-in"
+ " declaration %q#D", newdecl, olddecl)
+ && flag_permissive)
+ inform (DECL_SOURCE_LOCATION (newdecl),
+ "ignoring the %q#D declaration", newdecl);
+ return olddecl;
+ }
+ }
+
/* A near match; override the builtin. */
if (TREE_PUBLIC (newdecl))
--- gcc/testsuite/g++.dg/ext/pr84724-1.C.jj 2018-03-09 11:44:49.037993207 +0100
+++ gcc/testsuite/g++.dg/ext/pr84724-1.C 2018-03-09 11:57:11.599204801 +0100
@@ -0,0 +1,14 @@
+// PR c++/84724
+// { dg-do compile }
+// { dg-options "-O3 -fpermissive" }
+
+int __builtin_trap (); // { dg-warning "ambiguates built-in declaration" }
+ // { dg-message "ignoring the 'int __builtin_trap\\(\\)' declaration" "" { target *-*-* } .-1 }
+
+int
+foo ()
+{
+ int b;
+ int c (&b); // { dg-warning "invalid conversion from" }
+ return b %= b ? c : 0;
+}
--- gcc/testsuite/g++.dg/ext/pr84724-2.C.jj 2018-03-09 11:57:26.017208398 +0100
+++ gcc/testsuite/g++.dg/ext/pr84724-2.C 2018-03-09 11:57:40.775212078 +0100
@@ -0,0 +1,14 @@
+// PR c++/84724
+// { dg-do compile }
+// { dg-options "-O3 -fpermissive -w" }
+
+int __builtin_trap (); // { dg-bogus "ambiguates built-in declaration" }
+ // { dg-bogus "ignoring the 'int __builtin_trap\\(\\)' declaration" "" { target *-*-* } .-1 }
+
+int
+foo ()
+{
+ int b;
+ int c (&b); // { dg-bogus "invalid conversion from" }
+ return b %= b ? c : 0;
+}
--- gcc/testsuite/g++.dg/ext/pr84724-3.C.jj 2018-03-09 11:57:50.000214382 +0100
+++ gcc/testsuite/g++.dg/ext/pr84724-3.C 2018-03-09 11:58:10.797219571 +0100
@@ -0,0 +1,5 @@
+// PR c++/84724
+// { dg-do compile }
+// { dg-options "" }
+
+int __builtin_trap (); // { dg-error "ambiguates built-in declaration" }
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [C++ PATCH] Don't redefine __* builtins (PR c++/84724, take 2)
2018-03-09 12:35 ` [C++ PATCH] Don't redefine __* builtins (PR c++/84724, take 2) Jakub Jelinek
@ 2018-03-09 16:13 ` Jason Merrill
0 siblings, 0 replies; 4+ messages in thread
From: Jason Merrill @ 2018-03-09 16:13 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches List
OK.
On Fri, Mar 9, 2018 at 6:05 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Mar 08, 2018 at 04:06:24PM -0500, Jason Merrill wrote:
>> On Thu, Mar 8, 2018 at 1:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > The C FE just warns and doesn't override builtins, but C++ FE
>> > on say int __builtin_trap (); will override the builtin, so later
>> > builtin_decl_explicit will return the bogus user function which e.g.
>> > doesn't have any merged attributes, can have different arguments or
>> > return type etc.
>> >
>> > This patch prevents that; generally the builtins we want to override
>> > are the DECL_ANTICIPATED which we handle properly earlier.
>> >
>> > The testcase in the PR ICEs not because of the argument mismatch (there is
>> > none in this case) or return value mismatch (because nothing cares about the
>> > return value), but because the new decl lacks noreturn attribute and GCC
>> > relies on builtin_decl_explicit (BUILT_IN_TRAP) to be really noreturn.
>> >
>> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>> > Or shall we error on these, or ignore the name checks and just
>> > if (DECL_BUILT_IN (olddecl)) return NULL_TREE;, something else?
>>
>> Seems like returning NULL_TREE means that we overload the built-in,
>> which also seems undesirable; what if we return olddecl unmodified?
>> It would also be good to improve the diagnostic to say that we're
>> ignoring the declaration. Perhaps as a permerror.
>
> So like this (if it passes bootstrap/regtest, though I think we don't have
> any tests other than these new ones that cover it anyway, so I don't expect
> issues)?
>
> 2018-03-09 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/84724
> * decl.c (duplicate_decls): Don't override __* prefixed builtins
> except for __[^b]*_chk, instead issue permerror and for -fpermissive
> also a note and return olddecl.
>
> * g++.dg/ext/pr84724.C: New test.
>
> --- gcc/cp/decl.c.jj 2018-03-08 21:53:44.989559552 +0100
> +++ gcc/cp/decl.c 2018-03-09 11:53:58.557156637 +0100
> @@ -1566,6 +1566,33 @@ next_arg:;
> || compparms (TYPE_ARG_TYPES (TREE_TYPE (newdecl)),
> TYPE_ARG_TYPES (TREE_TYPE (olddecl))))
> {
> + /* Don't really override olddecl for __* prefixed builtins
> + except for __[^b]*_chk, the compiler might be using those
> + explicitly. */
> + if (DECL_BUILT_IN (olddecl))
> + {
> + tree id = DECL_NAME (olddecl);
> + const char *name = IDENTIFIER_POINTER (id);
> + size_t len;
> +
> + if (name[0] == '_'
> + && name[1] == '_'
> + && (strncmp (name + 2, "builtin_",
> + strlen ("builtin_")) == 0
> + || (len = strlen (name)) <= strlen ("___chk")
> + || memcmp (name + len - strlen ("_chk"),
> + "_chk", strlen ("_chk") + 1) != 0))
> + {
> + if (permerror (DECL_SOURCE_LOCATION (newdecl),
> + "new declaration %q#D ambiguates built-in"
> + " declaration %q#D", newdecl, olddecl)
> + && flag_permissive)
> + inform (DECL_SOURCE_LOCATION (newdecl),
> + "ignoring the %q#D declaration", newdecl);
> + return olddecl;
> + }
> + }
> +
> /* A near match; override the builtin. */
>
> if (TREE_PUBLIC (newdecl))
> --- gcc/testsuite/g++.dg/ext/pr84724-1.C.jj 2018-03-09 11:44:49.037993207 +0100
> +++ gcc/testsuite/g++.dg/ext/pr84724-1.C 2018-03-09 11:57:11.599204801 +0100
> @@ -0,0 +1,14 @@
> +// PR c++/84724
> +// { dg-do compile }
> +// { dg-options "-O3 -fpermissive" }
> +
> +int __builtin_trap (); // { dg-warning "ambiguates built-in declaration" }
> + // { dg-message "ignoring the 'int __builtin_trap\\(\\)' declaration" "" { target *-*-* } .-1 }
> +
> +int
> +foo ()
> +{
> + int b;
> + int c (&b); // { dg-warning "invalid conversion from" }
> + return b %= b ? c : 0;
> +}
> --- gcc/testsuite/g++.dg/ext/pr84724-2.C.jj 2018-03-09 11:57:26.017208398 +0100
> +++ gcc/testsuite/g++.dg/ext/pr84724-2.C 2018-03-09 11:57:40.775212078 +0100
> @@ -0,0 +1,14 @@
> +// PR c++/84724
> +// { dg-do compile }
> +// { dg-options "-O3 -fpermissive -w" }
> +
> +int __builtin_trap (); // { dg-bogus "ambiguates built-in declaration" }
> + // { dg-bogus "ignoring the 'int __builtin_trap\\(\\)' declaration" "" { target *-*-* } .-1 }
> +
> +int
> +foo ()
> +{
> + int b;
> + int c (&b); // { dg-bogus "invalid conversion from" }
> + return b %= b ? c : 0;
> +}
> --- gcc/testsuite/g++.dg/ext/pr84724-3.C.jj 2018-03-09 11:57:50.000214382 +0100
> +++ gcc/testsuite/g++.dg/ext/pr84724-3.C 2018-03-09 11:58:10.797219571 +0100
> @@ -0,0 +1,5 @@
> +// PR c++/84724
> +// { dg-do compile }
> +// { dg-options "" }
> +
> +int __builtin_trap (); // { dg-error "ambiguates built-in declaration" }
>
>
> Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-09 16:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-08 18:35 [C++ PATCH] Don't redefine __* builtins (PR c++/84724) Jakub Jelinek
2018-03-08 21:06 ` Jason Merrill
2018-03-09 12:35 ` [C++ PATCH] Don't redefine __* builtins (PR c++/84724, take 2) Jakub Jelinek
2018-03-09 16:13 ` Jason Merrill
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).