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

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

On Tue, Aug 28, 2018 at 4:37 AM Martin Sebor <msebor@gmail.com> wrote:
>
> Richard, please let me know if the patch is acceptable as is
> (with the RejectNegative property added).  As I said, I realize
> it's not ideal, but neither is any of the alternatives we have
> discussed.  They all involve trade- offs, and I think they would
> all make the behavior of the options less regular/useful, and
> the ideal solution would likely be too intrusive and laborious
> to implement.
>
> I leave for Cauldron this Friday so I'm hoping we can get this
> wrapped up before then.

OK, I'm going out of the way.

OK as-is.

Richard.

> Thanks
> Martin
>
> On 08/24/2018 09:13 AM, Martin Sebor wrote:
> > On 08/24/2018 03:36 AM, Richard Biener wrote:
> >> On Fri, Aug 24, 2018 at 12:35 AM Martin Sebor <msebor@gmail.com> wrote:
> >>>
> >>> On 08/23/2018 07:18 AM, Richard Biener wrote:
> >>>> On Thu, Aug 23, 2018 at 12:20 AM Martin Sebor <msebor@gmail.com> wrote:
> >>>>>
> >>>>> On 08/20/2018 06:14 AM, Richard Biener wrote:
> >>>>>> 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.
> >>>>>
> >>>>> Not sure I understand this last part.  Remove the enhancement?
> >>>>> (We do need to handle option arguments in excess of INT_MAX.)
> >>>>
> >>>> I see.
> >>>>
> >>>>>>
> >>>>>> It's that HOST_WIDE_INT_MAX use that is problematic IMHO.  Why not
> >>>>>> use -1?
> >>>>>
> >>>>> -1 is a valid/documented value of the argument of all these
> >>>>> options because it's treated as unsigned HWI:
> >>>>>
> >>>>>    Warnings controlled by the option can be disabled either
> >>>>>    by specifying byte-size of ‘SIZE_MAX’
> >>>>>
> >>>>> It has an intuitive meaning: warning for any size larger than
> >>>>> the maximum means not warning at all.  Treating -1 as special
> >>>>> instead of HOST_WIDE_INT_MAX would replace that meaning with
> >>>>> "warn on any size in excess of PTRDIFF_MAX."
> >>>>>
> >>>>> A reasonable way to disable the warning is like so:
> >>>>>
> >>>>>    gcc -Walloc-size-larger-than=$(getconf ULONG_MAX) ...
> >>>>>
> >>>>> That would not work anymore.
> >>>>>
> >>>>> Treating HOST_WIDE_INT_MAX as PTRDIFF_MAX is the natural choice
> >>>>> on LP64: they have the same value.  It's only less than perfectly
> >>>>> natural in ILP32 and even there it's not a problem in practice
> >>>>> because it's either far out of the range of valid values [0, 4GB]
> >>>>> (i.e., where HWI is a 64-bit long long), or it's also equal to
> >>>>> PTRDIFF_MAX (on hosts with no 64-bit type, if GCC even supports
> >>>>> any).
> >>>>>
> >>>>> I'm not trying to be stubborn here but I just don't see why
> >>>>> you think that setting aside HOST_WIDE_INT_MAX is problematic.
> >>>>> Anything else is worse from a user-interface POV.  It makes
> >>>>> little difference inside GCC as long as we want to let users
> >>>>> choose to warn for allocation sizes over some value in
> >>>>> [PTRDIFF_MAX, SIZE_MAX] -- e.g., for malloc() over 3GB but
> >>>>> not for less.  There's also the -Walloca-size-larger-than=
> >>>>> case where PTRDIFF_MAX means only warn for known sizes over
> >>>>> than but not for unknown sizes.
> >>>>
> >>>> Well I understand all of the above.  Alternatively you can omit
> >>>> the initializer for the option and use
> >>>>
> >>>>   if (!global_options_set.x_warn_alloc_size_limit)
> >>>>     warn_alloc_size_limit = PTRDIFF_MAX;
> >>>
> >>> Using zero to mean unset would prevent the -larger-than options
> >>> from having the expected meaning with a zero argument:
> >>
> >> global_options_set doesn't include the actual values but
> >> 1 if the option was specified on the command-line and 0 if not.
> >
> > I had hoped it worked that way but it's not what actually
> > happens.
> >
> > global_options_set.x_warn_alloc_size_limit is zero when I set
> > -Walloc-size-larger-than=0 on the command line.
> >
> > IIRC, it's because the option processing machinery conflates
> > option arguments with option flags (an argument of zero is
> > treated as of the option were explicitly disabled).  It's
> > the missing bit I referred to.
> >
> > Martin
> >
> >>
> >>> -Wxxx-size-larger-than=0 requests a warning for all objects
> >>> or allocations with non-zero size, including 1.
> >>>
> >>> In other words, we would lose the ability to diagnose
> >>> the following:
> >>>
> >>>    void f (void*, ...);
> >>>    void g (void)
> >>>    {
> >>>      char a[1];                     // -Wlarger-than=
> >>>      f (__builtin_malloc (1), a);   // -Walloc-size-larger-than=
> >>>    }
> >>>
> >>> It may not be the most important case to diagnose but it's
> >>> one that I think should work.
> >>>
> >>> What I think is missing in the option processing infrastructure
> >>> to make this work the way you describe without conflating zero
> >>> with PTRDIFF_MAX is an extra bit: is an option explicitly
> >>> specified, or is it at its default setting?  If it's the latter
> >>> then set it later to PTRDIFF_MAX.
> >>>
> >>>> that would also remove the special value.  Of course
> >>>> -Wno-alloc-size-larger-than cannot be a simple alias anymore then.
> >>>> -Walloc-size-larger-than= misses a RejectNegative btw
> >>>
> >>> Thanks.  I'll add that to all the -larger-than= options once
> >>> we agree on the final patch.
> >>>
> >>> Martin
> >>>
> >
>