On Fri, Aug 24, 2018 at 12:35 AM Martin Sebor <mse...@gmail.com> wrote: > > On 08/23/2018 07:18 AM, Richard Biener wrote: > > 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; > > 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 >