This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] treat -Wxxx-larger-than=HWI_MAX special (PR 86631)
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.
> -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
>