On Thu, Aug 23, 2018 at 12:20 AM Martin Sebor <mse...@gmail.com> wrote:
>
> On 08/20/2018 06:14 AM, Richard Biener wrote:
> > On Thu, Jul 26, 2018 at 10:52 PM Martin Sebor <mse...@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 <mse...@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;
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
Richard.
> Martin
>
> >
> > 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
> >>
>
