On Tue, Aug 28, 2018 at 4:37 AM Martin Sebor <mse...@gmail.com> wrote: > > Richard, please let me know if the patch is acceptable as is > (with the RejectNegative property added). As I said, I realize > it's not ideal, but neither is any of the alternatives we have > discussed. They all involve trade- offs, and I think they would > all make the behavior of the options less regular/useful, and > the ideal solution would likely be too intrusive and laborious > to implement. > > I leave for Cauldron this Friday so I'm hoping we can get this > wrapped up before then.
OK, I'm going out of the way. OK as-is. Richard. > Thanks > Martin > > On 08/24/2018 09:13 AM, Martin Sebor wrote: > > On 08/24/2018 03:36 AM, Richard Biener wrote: > >> 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. > > > > I had hoped it worked that way but it's not what actually > > happens. > > > > global_options_set.x_warn_alloc_size_limit is zero when I set > > -Walloc-size-larger-than=0 on the command line. > > > > IIRC, it's because the option processing machinery conflates > > option arguments with option flags (an argument of zero is > > treated as of the option were explicitly disabled). It's > > the missing bit I referred to. > > > > Martin > > > >> > >>> -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 > >>> > > >