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

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

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

On 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
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]