On Tue, Apr 05, 2022 at 10:50:01AM +0200, Richard Biener wrote:
> > In the following testcase sqrt is declared as unprototyped function
> > and so it depends on what type has the argument passed to it.
> > If an argument of incorrect type is passed, the sqrt comparison
> > simplification can create invalid GIMPLE.
> >
> > The patch fixes it by punting if there is a mismatch of types.
> > As I didn't want to reindent 133 lines, the first hunk contains an
> > ugly hack with if (false). If you prefer reindentation, I can do that
> > too.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> So why does this get pas gimple_call_combined_fn ()? Are those
> internal functions at the point we match them? Otherwise we
> invoke gimple_builtin_call_types_compatible_p on them. If
> they are internal functions why did we rewrite the calls to them
> when the types do not match? If gimple_builtin_call_types_compatible_p
> is fine then why did the frontend assign BUILT_IN_SQRT to the
> function decls?
For the unprototyped functions like double sqrt (); they can be called in a
valid way or they might not, so I think having DECL_BUILT_IN_CLASS other
than BUILT_IN_NORMAL is right.
The problem is that we don't do proper checking.
In generic-match.cc, we just call get_call_combined_fn which performs no
checking of the arguments whatsoever.
In gimple-match.cc, we call gimple_call_combined_fn, which performs bad
checking of the arguments.
When not an internal function (which doesn't need argument checking, the
compiler guarantees it is right), the former does just:
tree fndecl = get_callee_fndecl (call);
if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
return as_combined_fn (DECL_FUNCTION_CODE (fndecl));
In GIMPLE, we call:
&& gimple_builtin_call_types_compatible_p (stmt, fndecl)
but that is insufficient, that verifies whether the arguments passed to
GIMPLE_CALL match the fndecl argument types. But that fndecl may very well
be the user declaration, so doesn't have to match exactly the builtin
as predeclared by builtins.def etc. (though, there is the cotcha that say
for FILE * we just use void * etc.). So e.g. in tree-ssa-strlen.cc
we use additional:
tree callee = gimple_call_fndecl (stmt);
tree decl = builtin_decl_explicit (DECL_FUNCTION_CODE (callee));
if (decl
&& decl != callee
&& !gimple_builtin_call_types_compatible_p (stmt, decl))
return false;
For this particular testcase, seems the problem is just the generic folding,
if I add && GIMPLE to the sqrt comparison foldings, it doesn't ICE anymore,
so just adding generic_builtin_call_types_compatible_p (basically copy
gimple_builtin_call_types_compatible_p) might be enough.
Though a big question is what it should do, using useless_type_conversion_p
might be needed so that it can deal e.g. with the FILE * vs. void *
differences (and various others), on the other side, what e.g. the exact
sqrt comparison folding requires to be much more strict, as it turns
(cmp (sq @0) REAL_CST@1)
to
(cmp @0 @1)
and similarly for
(cmp (sq @0) (sq @1))
and that requires exact type match, no?
So one possibility would be to use useless_type_conversion_p but actually
not write (cmp @0 @1) in such cases, but (cmp (convert:op1type @0) @1)
or so?
Jakub
