Re: PATCH to fix -Wrestrict ICE (PR middle-end/83463)

[Martin Sebor <msebor at gmail dot com>]

On 12/18/2017 10:07 AM, Jakub Jelinek wrote:
> On Mon, Dec 18, 2017 at 10:00:36AM -0700, Martin Sebor wrote:
> > On 12/18/2017 08:10 AM, Marek Polacek wrote:
> > > I'm not entirely up to speed with this code, but this one seemed sufficiently
> > > obvious: check INTEGRAL_TYPE_P before looking at a tree's min/max value.
> > > Otherwise, go with maxobjsize.
> > >
> > > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >
> > Thanks for the fix.  I noticed the problem only comes up when
> > memcpy is declared without a prototype.  Otherwise, the memcpy
> > call is eliminated early on (during gimplification?)  So while
> > avoiding the ICE is obviously a necessary change, I wonder if
> > this bug also suggests a missing optimization that might still
> > be relevant (i.e., eliminating memcpy() with a zero size for
> > a memcpy without a prototype):
>
> No, we shouldn't try to optimize if people mess up stuff.

Declaring memcpy() without a prototype isn't messing up.  It may
be poor practice but there's nothing inherently wrong with it.
GCC accepts it, treats it as a built-in (i.e., doesn't complain
about the declaration being incompatible with the built-in), but
doesn't optimize it.  On the other hand, as I said, when someone
really does mess up and declares memcpy like so:

  void* memcpy (long, long, long);

GCC optimizes calls to it as if the function were declared with
the right prototype.

> What happens in this case is that
> gimple_builtin_call_types_compatible_p
> will fail, thus gimple_call_builtin_p and we won't be treating it specially.
> That just shows a problem in the new pass:
>   tree func = gimple_call_fndecl (call);
>   if (!func || DECL_BUILT_IN_CLASS (func) != BUILT_IN_NORMAL)
>     return;
> is just wrong, it should have been:
>   if (!gimple_call_builtin_p (call, BUILT_IN_NORMAL))
>     return;
>
> Then you don't need to worry about float or _Complex int third argument,
> or just 2 or 27 arguments instead of 3 for memcpy, etc.

Yes, but only at the cost of not checking calls to memcpy() and
other built ins declared without a prototype.  I didn't consider
those when I wrote the code but now that I have, since GCC allows
built-ins to be declared without a prototype and doesn't warn even
with -Wpedantic, it doesn't seem like a good approach to me.

Martin