| Message ID | 49b8d18d-98fe-84de-6a00-34d08fc2cfc7@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [COMMITTED] PR tree-optimization/101741 - Ensure toupper and tolower follow the expected pattern. | expand |
On Mon, Aug 9, 2021 at 10:31 PM Andrew MacLeod via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > The user has overridden the function name "toupper" . Its marked as a > builtin function, presumably because it matches the name. In range > folding, we were assuming the LHS and the parameter were compatible > types... but they are not in this case.. > > I don't know if we should be marking such a thing as a builtin function, > but regardless.. we shouldn't be trapping. If the type of the argument > is not compatible with the LHS, then we'll simply assume nothing about > the function. > > Bootstraps with no regression on x86_64-pc-linux-gnu. pushed. I wonder why the gimple_call_combined_fn verification using gimple_builtin_call_types_compatible_p isn't enough here? Yes, it's matching is a bit lazy, but only with respect to promotion of arguments to 'int'. Richard. > Andrew > > >
On 8/10/21 3:45 AM, Richard Biener wrote: > On Mon, Aug 9, 2021 at 10:31 PM Andrew MacLeod via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> The user has overridden the function name "toupper" . Its marked as a >> builtin function, presumably because it matches the name. In range >> folding, we were assuming the LHS and the parameter were compatible >> types... but they are not in this case.. >> >> I don't know if we should be marking such a thing as a builtin function, >> but regardless.. we shouldn't be trapping. If the type of the argument >> is not compatible with the LHS, then we'll simply assume nothing about >> the function. >> >> Bootstraps with no regression on x86_64-pc-linux-gnu. pushed. > I wonder why the gimple_call_combined_fn verification > using gimple_builtin_call_types_compatible_p isn't enough here? > Yes, it's matching is a bit lazy, but only with respect to promotion > of arguments to 'int'. > > Richard. > I set a breakpoint on the return type field for the builtin... A quick check shows the return type of the builtin is being changed to "unsigned int" here: #0 merge_decls (newdecl=0x7fffe9f67100, olddecl=0x7fffe9ed2100, newtype=0x7fffe9e3ae70, oldtype=0x7fffe9e3ae70) at /gcc/master/gcc/gcc/c/c-decl.c:2598 #1 0x0000000000a0628d in duplicate_decls (newdecl=0x7fffe9f67100, olddecl=0x7fffe9ed2100) at /gcc/master/gcc/gcc/c/c-decl.c:2963 #2 0x0000000000a07464 in pushdecl (x=0x7fffe9f67100) at /gcc/master/gcc/gcc/c/c-decl.c:3256 #3 0x0000000000a1d113 in start_function (declspecs=0x37728b0, declarator=0x3772ae0, attributes=0x0) at /gcc/master/gcc/gcc/c/c-decl.c:9644 #4 0x0000000000a8667c in c_parser_declaration_or_fndef (parser=0x7ffff7ff7ab0, fndef_ok=true, static_assert_ok=true, empty_ok=true, nested=false, start_attr_ok=true, objc_foreach_object_declaration=0x0, omp_declare_simd_clauses=0x0, have_attrs=false, attrs=0x0, oacc_routine_data=0x0, fallthru_attr_p=0x0) at /gcc/master/gcc/gcc/c/c-parser.c:2444 It would appear that merge_decls is merging the olddecl (the bultin) with newdecl and results in changing the LHS to unsigned int, so now it thinks the builtin matches. eeeks. I don't know what the correct thing to do is, but a quick hack to check if old_decl is a builtin and return false from duplicate_decl also seems to resolve the problem: diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 221a67fe57b..27ab6ac9f1a 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -2960,6 +2960,9 @@ duplicate_decls (tree newdecl, tree olddecl) return false; } + if (DECL_BUILT_IN_CLASS (olddecl) != NOT_BUILT_IN) + return false; + merge_decls (newdecl, olddecl, newtype, oldtype); /* The NEWDECL will no longer be needed.
On Tue, Aug 10, 2021 at 3:21 PM Andrew MacLeod <amacleod@redhat.com> wrote: > > On 8/10/21 3:45 AM, Richard Biener wrote: > > On Mon, Aug 9, 2021 at 10:31 PM Andrew MacLeod via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > >> The user has overridden the function name "toupper" . Its marked as a > >> builtin function, presumably because it matches the name. In range > >> folding, we were assuming the LHS and the parameter were compatible > >> types... but they are not in this case.. > >> > >> I don't know if we should be marking such a thing as a builtin function, > >> but regardless.. we shouldn't be trapping. If the type of the argument > >> is not compatible with the LHS, then we'll simply assume nothing about > >> the function. > >> > >> Bootstraps with no regression on x86_64-pc-linux-gnu. pushed. > > I wonder why the gimple_call_combined_fn verification > > using gimple_builtin_call_types_compatible_p isn't enough here? > > Yes, it's matching is a bit lazy, but only with respect to promotion > > of arguments to 'int'. > > > > Richard. > > > I set a breakpoint on the return type field for the builtin... A quick > check shows the return type of the builtin is being changed to "unsigned > int" here: OK, so gimple_builtin_call_types_compatible_p only checks that the call is consistent with the fndecl type - iff the declaration is incompatible with the declaration as specified by builtins.def then that's of course not detected by that check (this check is to catch cases where a formerly indirect call through an incompatible type is now known to be to a builtin). IIRC that is a recurring issue and indeed my opinion is that frontends should not mark function decls as BUILT_IN if the definition/declaration signature is not compatible. > #0 merge_decls (newdecl=0x7fffe9f67100, olddecl=0x7fffe9ed2100, > newtype=0x7fffe9e3ae70, oldtype=0x7fffe9e3ae70) at > /gcc/master/gcc/gcc/c/c-decl.c:2598 > #1 0x0000000000a0628d in duplicate_decls (newdecl=0x7fffe9f67100, > olddecl=0x7fffe9ed2100) at /gcc/master/gcc/gcc/c/c-decl.c:2963 > #2 0x0000000000a07464 in pushdecl (x=0x7fffe9f67100) at > /gcc/master/gcc/gcc/c/c-decl.c:3256 > #3 0x0000000000a1d113 in start_function (declspecs=0x37728b0, > declarator=0x3772ae0, attributes=0x0) at /gcc/master/gcc/gcc/c/c-decl.c:9644 > #4 0x0000000000a8667c in c_parser_declaration_or_fndef > (parser=0x7ffff7ff7ab0, fndef_ok=true, static_assert_ok=true, > empty_ok=true, nested=false, start_attr_ok=true, > objc_foreach_object_declaration=0x0, > omp_declare_simd_clauses=0x0, have_attrs=false, attrs=0x0, > oacc_routine_data=0x0, fallthru_attr_p=0x0) at > /gcc/master/gcc/gcc/c/c-parser.c:2444 > > > It would appear that merge_decls is merging the olddecl (the bultin) > with newdecl and results in changing the LHS to unsigned int, so now it > thinks the builtin matches. eeeks. > > I don't know what the correct thing to do is, but a quick hack to check > if old_decl is a builtin and return false from duplicate_decl also seems > to resolve the problem: Yeah, but that's of course too harsh - we do want correct user declarations of say 'malloc' to be classified as built-in. The odd thing is that we merge int foo() and unsigned foo () using composite_type. diagnose_mismatched_decls should be made more strict here although it explicitly mentions /* Accept "harmless" mismatches in function types such as missing qualifiers or int vs long when they're the same size. However, diagnose return and argument types that are incompatible according to language rules. */ whatever 'language rules' means here. Anyway, this function is where we should avoid making newdecl built-in (and we should of course not adjust olddecl either). Richard. > diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c > index 221a67fe57b..27ab6ac9f1a 100644 > --- a/gcc/c/c-decl.c > +++ b/gcc/c/c-decl.c > @@ -2960,6 +2960,9 @@ duplicate_decls (tree newdecl, tree olddecl) > return false; > } > > + if (DECL_BUILT_IN_CLASS (olddecl) != NOT_BUILT_IN) > + return false; > + > merge_decls (newdecl, olddecl, newtype, oldtype); > > /* The NEWDECL will no longer be needed. > >
On Tue, Aug 10, 2021 at 04:14:15PM +0200, Richard Biener via Gcc-patches wrote: > OK, so gimple_builtin_call_types_compatible_p only checks that the > call is consistent with the fndecl type - iff the declaration is incompatible > with the declaration as specified by builtins.def then that's of course > not detected by that check (this check is to catch cases where a > formerly indirect call through an incompatible type is now known to > be to a builtin). > > IIRC that is a recurring issue and indeed my opinion is that frontends > should not mark function decls as BUILT_IN if the definition/declaration > signature is not compatible. Different FEs use different strictness for what is or is not compatible. And we can't be too strict, because e.g. of FILE * arguments where the FILE type isn't pre-declared for the builtins. On severe mismatches I think the FEs already don't mark it built in (say double vs. int etc.), and e.g. const char * vs. char * differences should be allowed (e.g. consider C++ vs. C strchr). But perhaps we should consider as incompatible somethng that doesn't pass useless_type_conversion_p too... Jakub
commit c86c95edd165d674614516cda0b1fcb6616c1096
Author: Andrew MacLeod <amacleod@redhat.com>
Date: Mon Aug 9 15:53:42 2021 -0400
Ensure toupper and tolower follow the expected pattern.
If the parameter is not compatible with the LHS, assume this is not really a
builtin function to avoid a trap.
gcc/
PR tree-optimization/101741
* gimple-range-fold.cc (fold_using_range::range_of_builtin_call): Check
type of parameter for toupper/tolower.
gcc/testsuite/
* gcc.dg/pr101741.c: New.
diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
index 410bc4ddca4..d3e3e14ff64 100644
--- a/gcc/gimple-range-fold.cc
+++ b/gcc/gimple-range-fold.cc
@@ -894,6 +894,9 @@ fold_using_range::range_of_builtin_call (irange &r, gcall *call,
case CFN_BUILT_IN_TOUPPER:
{
arg = gimple_call_arg (call, 0);
+ // If the argument isn't compatible with the LHS, do nothing.
+ if (!range_compatible_p (type, TREE_TYPE (arg)))
+ return false;
if (!src.get_operand (r, arg))
return false;
@@ -913,6 +916,9 @@ fold_using_range::range_of_builtin_call (irange &r, gcall *call,
case CFN_BUILT_IN_TOLOWER:
{
arg = gimple_call_arg (call, 0);
+ // If the argument isn't compatible with the LHS, do nothing.
+ if (!range_compatible_p (type, TREE_TYPE (arg)))
+ return false;
if (!src.get_operand (r, arg))
return false;
diff --git a/gcc/testsuite/gcc.dg/pr101741.c b/gcc/testsuite/gcc.dg/pr101741.c
new file mode 100644
index 00000000000..6587dca77d5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr101741.c
@@ -0,0 +1,16 @@
+/* PR tree-optimization/101741 */
+/* { dg-do compile } */
+/* { dg-options "-O2 " } */
+
+int
+foo (void);
+
+unsigned int
+toupper (int c)
+{
+ c = foo ();
+ while (c)
+ c = toupper (c);
+
+ return c;
+}