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: [PING #3] [PATCH] make -Wrestrict for strcat more meaningful (PR 83698)


Ping: https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01488.html

On 02/05/2018 08:20 PM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01488.html

As I mentioned, this doesn't solve a regression per se but
rather implements what I consider an important usability
improvement to the -Wrestrict warnings.  Printing offsets
that are the most meaningful makes debugging the problems
the warnings point out easier.

On 01/30/2018 10:24 AM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01488.html

This is a minor improvement but there have been a couple of
comments lately pointing out that the numbers in the -Wrestrict
messages can make them confusing.

Jakub, since you made one of those comments (the other came up
in the context of bug 84095 - [8 Regression] false-positive
-Wrestrict warnings for memcpy within array).  Can you please
either approve this patch or suggest changes?

On 01/16/2018 05:35 PM, Martin Sebor wrote:
On 01/16/2018 02:32 PM, Jakub Jelinek wrote:
On Tue, Jan 16, 2018 at 01:36:26PM -0700, Martin Sebor wrote:
--- gcc/gimple-ssa-warn-restrict.c    (revision 256752)
+++ gcc/gimple-ssa-warn-restrict.c    (working copy)
@@ -384,6 +384,12 @@ builtin_memref::builtin_memref (tree expr,
tree si
       base = SSA_NAME_VAR (base);
       }

+  if (DECL_P (base) && TREE_CODE (TREE_TYPE (base)) == ARRAY_TYPE)
+    {
+      if (offrange[0] < 0 && offrange[1] > 0)
+    offrange[0] = 0;
+    }

Why the 2 nested ifs?

No particular reason.  There may have been more code in there
that I ended up removing.  Or a comment.  I can remove the
extra braces when the patch is approved.


@@ -1079,14 +1085,35 @@ builtin_access::strcat_overlap ()
     return false;

   /* When strcat overlap is certain it is always a single byte:
-     the terminatinn NUL, regardless of offsets and sizes.  When
+     the terminating NUL, regardless of offsets and sizes.  When
      overlap is only possible its range is [0, 1].  */
   acs.ovlsiz[0] = dstref->sizrange[0] == dstref->sizrange[1] ? 1 : 0;
   acs.ovlsiz[1] = 1;
-  acs.ovloff[0] = (dstref->sizrange[0] +
dstref->offrange[0]).to_shwi ();
-  acs.ovloff[1] = (dstref->sizrange[1] +
dstref->offrange[1]).to_shwi ();

You use to_shwi many times in the patch, do the callers or something
earlier
in this function guarantee that you aren't throwing away any bits
(unlike
tree_to_shwi, to_shwi method doesn't ICE, just throws away upper bits).
Especially when you perform additions like here, even if both
wide_ints fit
into a shwi, the result might not.

No, I'm not sure.  In fact, it wouldn't surprise me if it did
happen.  It doesn't cause false positives or negatives but it
can make the offsets less than meaningful in cases where they
are within valid bounds.  There are also cases where they are
meaningless to begin with and there is little the pass can do
about that.

IMO, the ideal solution to the first problem is to add a format
specifier for wide ints to the pretty printer and get rid of
the conversions.  It's probably too late for something like
that now but I'd like to do it for GCC 9.  Unless someone
files a bug/regression, it's also too late for me to go and
try to find and fix these conversions now.

Martin

PS While looking for a case you asked about I came up with
the following.  I don't think there's any slicing involved
but the offsets are just as meaningless as if there were.
I think the way to do significantly better is to detect
out-of-bounds offsets earlier (e.g., as in this patch:
https://gcc.gnu.org/ml/gcc-patches/2017-10/msg02143.html)

$ cat z.c && gcc -O2 -S -Warray-bounds -m32 z.c
extern int a[];

void f (__PTRDIFF_TYPE__ i)
{
  if (i < __PTRDIFF_MAX__ - 7 || __PTRDIFF_MAX__ - 5 < i)
    i = __PTRDIFF_MAX__ -  7;

  const int *s = a + i;

  __builtin_memcpy (a, &s[i], 3);
}
z.c: In function ‘f’:
z.c:10:3: warning: ‘__builtin_memcpy’ offset [-64, -48] is out of the
bounds of object ‘a’ with type ‘int[]’ [-Warray-bounds]
   __builtin_memcpy (a, &s[i], 3);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
z.c:1:12: note: ‘a’ declared here
 extern int a[];
            ^





Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]