Re: [PATCH] treat -Wxxx-larger-than=HWI_MAX special (PR 86631)

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Jul 26, 2018 at 10:52 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 07/26/2018 08:58 AM, Martin Sebor wrote:
> > On 07/26/2018 02:38 AM, Richard Biener wrote:
> >> On Wed, Jul 25, 2018 at 5:54 PM Martin Sebor <msebor@gmail.com> wrote:
> >>>
> >>> On 07/25/2018 08:57 AM, Jakub Jelinek wrote:
> >>>> On Wed, Jul 25, 2018 at 08:54:13AM -0600, Martin Sebor wrote:
> >>>>> I don't mean for the special value to be used except internally
> >>>>> for the defaults.  Otherwise, users wanting to override the default
> >>>>> will choose a value other than it.  I'm happy to document it in
> >>>>> the .opt file for internal users though.
> >>>>>
> >>>>> -1 has the documented effect of disabling the warnings altogether
> >>>>> (-1 is SIZE_MAX) so while I agree that -1 looks better it doesn't
> >>>>> work.  (It would need more significant changes.)
> >>>>
> >>>> The variable is signed, so -1 is not SIZE_MAX.  Even if -1 disables
> >>>> it, you
> >>>> could use e.g. -2 or other negative value for the other special case.
> >>>
> >>> The -Wxxx-larger-than=N distinguish three ranges of argument
> >>> values (treated as unsigned):
> >>>
> >>>    1.  [0, HOST_WIDE_INT_MAX)
> >>>    2.  HOST_WIDE_INT_MAX
> >>>    3.  [HOST_WIDE_INT_MAX + 1, Infinity)
> >>
> >> But it doesn't make sense for those to be host dependent.
> >
> > It isn't when the values are handled by each warning.  That's
> > also the point of this patch: to remove this (unintended)
> > dependency.
> >
> >> I think numerical user input should be limited to [0, ptrdiff_max]
> >> and cases (1) and (2) should be simply merged, I see no value
> >> in distinguishing them.  -Wxxx-larger-than should be aliased
> >> to [0, ptrdiff_max], case (3) is achieved by -Wno-xxx-larger-than.
>
> To be clear: this is also close to what this patch does.
>
> The only wrinkle is that we don't know the value of PTRDIFF_MAX
> either at the time the option initial value is set in the .opt
> file or when the option is processed when it's specified either
> on the command line or as an alias in the .opt file (as all
> -Wno-xxx-larger-than options are).

But then why not make that special value accessible and handle
it as PTRDIFF_MAX when that is available (at users of the params)?

That is,

Index: gcc/calls.c
===================================================================
--- gcc/calls.c (revision 262951)
+++ gcc/calls.c (working copy)
@@ -1222,9 +1222,12 @@ alloc_max_size (void)
   if (alloc_object_size_limit)
     return alloc_object_size_limit;

-  alloc_object_size_limit
-    = build_int_cst (size_type_node, warn_alloc_size_limit);
+  HOST_WIDE_INT limit = warn_alloc_size_limit;
+  if (limit == HOST_WIDE_INT_MAX)
+    limit = tree_to_shwi (TYPE_MAX_VALUE (ptrdiff_type_node));

+  alloc_object_size_limit = build_int_cst (size_type_node, limit);
+
   return alloc_object_size_limit;
 }

use sth like

 if (warn_alloc_size_limit == -1)
   alloc_object_size_limit = fold_convert (size_type_node,
TYPE_MAX_VALUE (ptrdiff_type_node));
 else
   alloc_object_size_limit = size_int (warn_alloc_size_limit);

?  Also removing the need to have > int params values.

It's that HOST_WIDE_INT_MAX use that is problematic IMHO.  Why not use -1?

Richard.

>  Case (2) above is only
> used by the implementation as a placeholder for PTRDIFF_MAX.
> It's not part of the interface -- it's an internal workaround
> for lack of a better word.
>
> (There is an additional wrinkle in the -Walloca-larger-than=
> has two modes both of which are controlled by a single option:
> (a) diagnose only allocations >= PTRDIFF_MAX (default), and
> (b) diagnose allocations > limit plus also unbounded/unknown
> allocations.  I think these modes should be split up and (b)
> controlled by a separate option (say something like
> -Walloca-may-be-unbounded).
>
> >> I think you are over-engineering this and the user-interface is
> >> awful.
> >
> > Thank you.
> >
> > I agree that what you describe would be the ideal solution.
> > As I explained in the description of the patch, I did consider
> > handling PTRDIFF_MAX but the target-dependent value is not
> > available at the time the option argument is processed.  We
> > don't even know yet what the target data model is.
> >
> > This is the best I came up with.  What do you suggest instead?
> >
> > Martin
>