| Message ID | fc296cec-7948-5f9f-c241-bd7a5e1b715a@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | treat -Wxxx-larger-than=HWI_MAX special (PR 86631) | expand |
On Wed, Jul 25, 2018 at 4:07 AM Martin Sebor <msebor@gmail.com> wrote: > > The very large option argument enhancement committed last week > inadvertently introduced an assumption about the LP64 data model > that makes the -Wxxx-larger-than options have a different effect > at their default documented setting of PTRDIFF_MAX between ILP32 > and LP64. As a result, the options are treated as suppressed in > ILP32 while triggering the expected warnings for allocations or > sizes in excess of the limit in ILP64. > > To remove this I considered making it possible to use symbolic > constants like PTRDIFF_MAX as the option arguments so that > then defaults in the .opt files could be set to that. Sadly, > options in GCC are processed before the values of constants > like PTRDIFF_MAX for the target are known, and deferring > the handling of just the -Wxxx-larger-than= options until > the constants have been computed would be too involved to > make it worth the effort. > > To keep things simple I decided to have the code that handles > each of the affected options treat the HOST_WIDE_INT_MAX default > as a special request to substitute the value of PTRDIFF_MAX at > the time. > > The attached patch implements this. I wonder if documenting a special value of -1 would be easier for users to use. Your patch doesn't come with adjustments to invoke.texi so I wonder how people could know of this special handling? Richard. > Tested on x86_64-linux with -m32/-m64. > > Martin
On 07/25/2018 02:34 AM, Richard Biener wrote: > On Wed, Jul 25, 2018 at 4:07 AM Martin Sebor <msebor@gmail.com> wrote: >> >> The very large option argument enhancement committed last week >> inadvertently introduced an assumption about the LP64 data model >> that makes the -Wxxx-larger-than options have a different effect >> at their default documented setting of PTRDIFF_MAX between ILP32 >> and LP64. As a result, the options are treated as suppressed in >> ILP32 while triggering the expected warnings for allocations or >> sizes in excess of the limit in ILP64. >> >> To remove this I considered making it possible to use symbolic >> constants like PTRDIFF_MAX as the option arguments so that >> then defaults in the .opt files could be set to that. Sadly, >> options in GCC are processed before the values of constants >> like PTRDIFF_MAX for the target are known, and deferring >> the handling of just the -Wxxx-larger-than= options until >> the constants have been computed would be too involved to >> make it worth the effort. >> >> To keep things simple I decided to have the code that handles >> each of the affected options treat the HOST_WIDE_INT_MAX default >> as a special request to substitute the value of PTRDIFF_MAX at >> the time. >> >> The attached patch implements this. > > I wonder if documenting a special value of -1 would be easier for > users to use. Your patch doesn't come with adjustments to > invoke.texi so I wonder how people could know of this special > handling? 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.) I don't consider this the most elegant design but it's the best I could think of short of complicating things even more. Martin
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. Jakub
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) (1) implies warnings for allocations in excess of the size. For the alloca/VLA warnings it also means warnings for allocations that may be unbounded. (This feels like a bit of a wart.) (2) implies warnings for allocations in excess of PTRDIFF_MAX only. For the alloca/VLA warnings it also disables warnings for allocations that may be unbounded (also a bit of a wart) (3) isn't treated consistently by all options (yet) but for the alloca/VLA warnings it means no warnings. Since the argument value is stored in signed HOST_WIDE_INT this range is strictly negative. Any value from (3) could in theory be made special and used instead of -1 or HOST_WIDE_INT_MAX as a placeholder for PTRDIFF_MAX. But no matter what the choice is, it removes the value from the usable set in (3) (i.e., it doesn't have the expected effect of disabling the warning). I don't see the advantage of picking -2 over any other negative number. As inelegant as the current choice of HOST_WIDE_INT_MAX may be, it seems less arbitrary and less intrusive than picking a random value from the negative range. Martin PS The handling of these ranges isn't consistent across all the options because they were each developed independently and without necessarily aiming for it. I think making them more consistent would be nice as a followup patch. I would expect consistency to be achievable more easily if baking special cases into the design is kept to a minimum. It would also help to remove some existing special cases. E.g., by introducing a distinct option for the special case of diagnosing unbounded alloca/VLA allocations and removing it from -W{alloca,vla}-larger-than=.
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. 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. I think you are over-engineering this and the user-interface is awful. > (1) implies warnings for allocations in excess of the size. For > the alloca/VLA warnings it also means warnings for allocations > that may be unbounded. (This feels like a bit of a wart.) > > (2) implies warnings for allocations in excess of PTRDIFF_MAX > only. For the alloca/VLA warnings it also disables warnings > for allocations that may be unbounded (also a bit of a wart) > > (3) isn't treated consistently by all options (yet) but for > the alloca/VLA warnings it means no warnings. Since > the argument value is stored in signed HOST_WIDE_INT this > range is strictly negative. > > Any value from (3) could in theory be made special and used > instead of -1 or HOST_WIDE_INT_MAX as a placeholder for > PTRDIFF_MAX. But no matter what the choice is, it removes > the value from the usable set in (3) (i.e., it doesn't have > the expected effect of disabling the warning). > > I don't see the advantage of picking -2 over any other negative > number. As inelegant as the current choice of HOST_WIDE_INT_MAX > may be, it seems less arbitrary and less intrusive than picking > a random value from the negative range. > > Martin > > PS The handling of these ranges isn't consistent across all > the options because they were each developed independently > and without necessarily aiming for it. I think making them > more consistent would be nice as a followup patch. I would > expect consistency to be achievable more easily if baking > special cases into the design is kept to a minimum. It > would also help to remove some existing special cases. > E.g., by introducing a distinct option for the special case > of diagnosing unbounded alloca/VLA allocations and removing > it from -W{alloca,vla}-larger-than=.
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. > > 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
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). 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
Richard, do you have any further comments or suggestions or is the patch acceptable? I realize it's not ideal but I don't see how to achieve the ideal (understanding PTRDIFF_MAX) without deferring the processing of these options until the back end has been initialized. It would still mean setting aside some special value, traversing options again, looking for the Host_Wide_Int ones with the special value, and replacing it with the value of PTRDIFF_MAX. That doesn't seem any cleaner than the current solution, just a lot more work (not just to implement but for the compiler to go through at startup). Joseph, can you think of anything better? https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01455.html On 07/26/2018 02:52 PM, Martin Sebor 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). 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 >
On Wed, 1 Aug 2018, Martin Sebor wrote: > Richard, do you have any further comments or suggestions or is > the patch acceptable? > > I realize it's not ideal but I don't see how to achieve the ideal > (understanding PTRDIFF_MAX) without deferring the processing of > these options until the back end has been initialized. It would > still mean setting aside some special value, traversing options > again, looking for the Host_Wide_Int ones with the special value, > and replacing it with the value of PTRDIFF_MAX. That doesn't seem > any cleaner than the current solution, just a lot more work (not > just to implement but for the compiler to go through at startup). > > Joseph, can you think of anything better? > > https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01455.html I think the cleanest option for accepting symbolic values would require options accepting such a value in addition to numeric arguments to have additional data allocated (effectively storing both a numeric argument and an Enum argument), and then the late initialization as noted to set the numeric value based on the Enum argument if one was given. As noted, that's significant extra complexity.
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 >
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.) > > 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. 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 >>
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 > >> >
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: -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
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 >
On 08/24/2018 03:36 AM, Richard Biener wrote: > 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. 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 >>
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. 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 <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. > > 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 >>> >
On Tue, Aug 28, 2018 at 4:37 AM Martin Sebor <msebor@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 <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. > > > > 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 > >>> > > >
PR middle-end/86631 - missing -Walloc-size-larger-than on ILP32 hosts gcc/ChangeLog: PR middle-end/86631 * calls.c (alloc_max_size): Treat HOST_WIDE_INT special. * gimple-ssa-warn-alloca.c (adjusted_warn_limit): New function. (pass_walloca::gate): Use it. (alloca_call_type): Same. (pass_walloca::execute): Same. * stor-layout.c (layout_decl): Treat HOST_WIDE_INT special. gcc/testsuite/ChangeLog: PR middle-end/86631 * g++.dg/Walloca1.C: Adjust. 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; } Index: gcc/gimple-ssa-warn-alloca.c =================================================================== --- gcc/gimple-ssa-warn-alloca.c (revision 262951) +++ gcc/gimple-ssa-warn-alloca.c (working copy) @@ -38,6 +38,8 @@ along with GCC; see the file COPYING3. If not see #include "cfgloop.h" #include "intl.h" +static unsigned HOST_WIDE_INT adjusted_warn_limit (bool); + const pass_data pass_data_walloca = { GIMPLE_PASS, "walloca", @@ -82,7 +84,9 @@ pass_walloca::gate (function *fun ATTRIBUTE_UNUSED // Warning is disabled when its size limit is greater than PTRDIFF_MAX // for the target maximum, which makes the limit negative since when // represented in signed HOST_WIDE_INT. - return warn_alloca_limit >= 0 || warn_vla_limit >= 0; + unsigned HOST_WIDE_INT max = tree_to_uhwi (TYPE_MAX_VALUE (ptrdiff_type_node)); + return (adjusted_warn_limit (false) <= max + || adjusted_warn_limit (true) <= max); } // Possible problematic uses of alloca. @@ -127,6 +131,30 @@ struct alloca_type_and_limit { alloca_type_and_limit (enum alloca_type type) : type(type) { } }; +/* Return the value of the argument N to -Walloca-larger-than= or + -Wvla-larger-than= adjusted for the target data model so that + when N == HOST_WIDE_INT_MAX, the adjusted value is set to + PTRDIFF_MAX on the target. This is done to prevent warnings + for unknown/unbounded allocations in the "permissive mode" + while still diagnosing excessive and necessarily invalid + allocations. */ + +static unsigned HOST_WIDE_INT +adjusted_warn_limit (bool idx) +{ + static HOST_WIDE_INT limits[2]; + if (limits[idx]) + return limits[idx]; + + limits[idx] = idx ? warn_vla_limit : warn_alloca_limit; + if (limits[idx] != HOST_WIDE_INT_MAX) + return limits[idx]; + + limits[idx] = tree_to_shwi (TYPE_MAX_VALUE (ptrdiff_type_node)); + return limits[idx]; +} + + // NOTE: When we get better range info, this entire function becomes // irrelevant, as it should be possible to get range info for an SSA // name at any point in the program. @@ -309,11 +337,7 @@ alloca_call_type (gimple *stmt, bool is_vla, tree // Adjust warn_alloca_max_size for VLAs, by taking the underlying // type into account. - unsigned HOST_WIDE_INT max_size; - if (is_vla) - max_size = warn_vla_limit; - else - max_size = warn_alloca_limit; + unsigned HOST_WIDE_INT max_size = adjusted_warn_limit (is_vla); // Check for the obviously bounded case. if (TREE_CODE (len) == INTEGER_CST) @@ -510,6 +534,8 @@ pass_walloca::execute (function *fun) struct alloca_type_and_limit t = alloca_call_type (stmt, is_vla, &invalid_casted_type); + unsigned HOST_WIDE_INT adjusted_alloca_limit + = adjusted_warn_limit (false); // Even if we think the alloca call is OK, make sure it's not in a // loop, except for a VLA, since VLAs are guaranteed to be cleaned // up when they go out of scope, including in a loop. @@ -519,8 +545,7 @@ pass_walloca::execute (function *fun) is less than the maximum valid object size. */ const offset_int maxobjsize = wi::to_offset (max_object_size ()); - if ((unsigned HOST_WIDE_INT) warn_alloca_limit - < maxobjsize.to_uhwi ()) + if (adjusted_alloca_limit < maxobjsize.to_uhwi ()) t = alloca_type_and_limit (ALLOCA_IN_LOOP); } @@ -541,7 +566,7 @@ pass_walloca::execute (function *fun) print_decu (t.limit, buff); inform (loc, G_("limit is %wu bytes, but argument " "may be as large as %s"), - is_vla ? warn_vla_limit : warn_alloca_limit, buff); + is_vla ? warn_vla_limit : adjusted_alloca_limit, buff); } break; case ALLOCA_BOUND_DEFINITELY_LARGE: @@ -553,7 +578,7 @@ pass_walloca::execute (function *fun) { print_decu (t.limit, buff); inform (loc, G_("limit is %wu bytes, but argument is %s"), - is_vla ? warn_vla_limit : warn_alloca_limit, buff); + is_vla ? warn_vla_limit : adjusted_alloca_limit, buff); } break; case ALLOCA_BOUND_UNKNOWN: Index: gcc/stor-layout.c =================================================================== --- gcc/stor-layout.c (revision 262951) +++ gcc/stor-layout.c (working copy) @@ -761,14 +761,19 @@ layout_decl (tree decl, unsigned int known_align) { tree size = DECL_SIZE_UNIT (decl); - if (size != 0 && TREE_CODE (size) == INTEGER_CST - && compare_tree_int (size, warn_larger_than_size) > 0) + if (size != 0 && TREE_CODE (size) == INTEGER_CST) { - unsigned HOST_WIDE_INT uhwisize = tree_to_uhwi (size); + /* -Wlarger-than= argument of HOST_WIDE_INT_MAX is treated + as if PTRDIFF_MAX had been specified, with the value + being that on the target rather than the host. */ + unsigned HOST_WIDE_INT max_size = warn_larger_than_size; + if (max_size == HOST_WIDE_INT_MAX) + max_size = tree_to_shwi (TYPE_MAX_VALUE (ptrdiff_type_node)); - warning (OPT_Wlarger_than_, "size of %q+D %wu bytes exceeds " - "maximum object size %wu", - decl, uhwisize, warn_larger_than_size); + if (compare_tree_int (size, max_size) > 0) + warning (OPT_Wlarger_than_, "size of %q+D %E bytes exceeds " + "maximum object size %wu", + decl, size, max_size); } } Index: gcc/testsuite/g++.dg/Walloca1.C =================================================================== --- gcc/testsuite/g++.dg/Walloca1.C (revision 262951) +++ gcc/testsuite/g++.dg/Walloca1.C (working copy) @@ -1,7 +1,9 @@ -/* PR middle-end/79809 */ +/* PR middle-end/79809 - ICE in alloca_call_type, at gimple-ssa-warn-alloca.c */ /* { dg-do compile } */ /* { dg-options "-Walloca-larger-than=4207115063 -Wvla-larger-than=1233877270 -O2" } */ /* { dg-require-effective-target alloca } */ int a; -char *b = static_cast<char *>(__builtin_alloca (a)); // { dg-warning "argument to .alloca. may be too large|unbounded use of" } +char *b = static_cast<char *>(__builtin_alloca (a)); + +// { dg-prune-output "argument to .alloca." }