On Tue, Aug 10, 2021 at 3:21 PM Andrew MacLeod <amacl...@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.
>
>
