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)
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Martin Sebor <msebor at gmail dot com>
- Cc: Jakub Jelinek <jakub at redhat dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 20 Aug 2018 14:14:01 +0200
- Subject: Re: [PATCH] treat -Wxxx-larger-than=HWI_MAX special (PR 86631)
- References: <fc296cec-7948-5f9f-c241-bd7a5e1b715a@gmail.com> <CAFiYyc1EHG0YGKVvOPc8QyUWy2dc0+f_5ubcqxZsJLX2GsSz-A@mail.gmail.com> <07254dc1-f173-321e-390b-1d79044a5727@gmail.com> <20180725145724.GR17988@tucnak> <3a5817e7-b766-3183-d119-f21bf6fbb838@gmail.com> <CAFiYyc1z6GF8LZ4BpmBJ_wCpeDSiS=kEYoNS0QoqSPBtLzaPxg@mail.gmail.com> <89ad68dc-d98b-8832-adc6-839e82953433@gmail.com> <d92657fa-e665-9065-ab86-ac9e8dcbdf3b@gmail.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
>