This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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;

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
> >>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]