Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Nov 16, 2017 at 10:29 PM, Martin Sebor <msebor@gmail.com> wrote:
> Ping.
>
> I've fixed the outstanding false positive exposed by the Linux
> kernel.  The kernel builds with four instances of the warning,
> all of them valid (perfect overlap in memcpy).
>
> I also built Glibc.  It shows one instance of the warning, also
> a true positive (cause by calling a restrict-qualified function
> with two copies of the same argument).
>
> Finally, I built Binutils and GDB with no warnings.
>
> The attached patch includes just that one fix, with everything
> else being the same.

+             /* Detect invalid bounds and overlapping copies and issue
+                either -Warray-bounds or -Wrestrict.  */
+             if (check_bounds_or_overlap (stmt, dest, src, len, len))
+               gimple_set_no_warning (stmt, true);

if (! gimple_no_warning (stmt)
    && ...)

to avoid repeated work if this call doesn't get folded.

@@ -7295,3 +7342,4 @@ gimple_stmt_integer_valued_real_p (gimple *stmt,
int depth)
       return false;
     }
 }
+

please don't do unrelated whitespace changes.

+
+  if (const strinfo *chksi = olddsi ? olddsi : dsi)
+    if (si
+       && !check_bounds_or_overlap (stmt, chksi->ptr, si->ptr, NULL_TREE, len))
+      /* Avoid transforming strcpy when out-of-bounds offsets or
+        overlapping access is detected.  */
+      return;

as I said elsewhere diagnostics should not prevent optimization.  Your warning
code isn't optimization-grade (that is, false positives are possible).

+       if (!check_bounds_or_overlap (stmt, dst, sptr, NULL_TREE, slen))
+         /* Avoid transforming strcat when out-of-bounds offsets or
+            overlapping access is detected.  */
+         return;
+      }

Likewise.

+      if (!check_bounds_or_overlap (stmt, dst, sptr, dstlen, srcsize))
+         /* Avoid transforming strcat when out-of-bounds offsets or
+            overlapping access is detected.  */
+       return;

Likewise.

I have no strong opinion against the "code duplication" Jeff mentions with
regarding to builtin_access and friends.  The relation to ao_ref and friends
could be documented a bit and how builtin_memref/builtin_access are
not suitable for optimization.

Thanks,
Richard.

>
> On 11/09/2017 04:57 PM, Martin Sebor wrote:
>>
>> Ping:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01642.html
>>
>> On 10/23/2017 08:42 PM, Martin Sebor wrote:
>>>
>>> Attached is a reworked solution to enhance -Wrestrict while
>>> avoiding changing tree-vrp.c or any other VRP machinery.  Richard,
>>> in considering you suggestions I realized that the ao_ref struct
>>> isn't general enough to detect the kinds of problems I needed to
>>> etect (storing bit-offsets in HOST_WIDE_INT means out-of-bounds
>>> offsets cannot be represented or detected, leading to either false
>>> positives or false negatives).
>>>
>>> Instead, the solution adds a couple of small classes to builtins.c
>>> to overcome this limitation (I'm contemplating moving them along
>>> with -Wstringop-overflow to a separate file to keep builtins.c
>>> from getting too much bigger).
>>>
>>> The detection of out-of-bounds offsets and overlapping accesses
>>> is relatively simple but the rest of the changes are somewhat
>>> involved because of the computation of the offsets and sizes of
>>> the overlaps.
>>>
>>> Jeff, as per your suggestion/request in an earlier review (bug
>>> 81117) I've renamed some of the existing functions to better
>>> reflect their new function (including renaming strlen_optimize_stmt
>>> in tree-ssa-strlen.c to strlen_check_and_optimize_stmt).  There's
>>> quite a bit of churn due to some of this renaming.  I don't think
>>> this renaming makes the review too difficult but if you feel
>>> differently I can [be persuaded to] split it out into a separate
>>> patch.
>>>
>>> To validate the patch I compiled the Linux kernel and Binutils/GDB.
>>> There's one false positive I'm working on resolving that's caused
>>> by an incorrect interpretation of an offset in a range whose lower
>>> bound is greater than its upper bound.  This it so say that while
>>> I'm aware the patch isn't perfect it already works reasonably well
>>> in practice and I think it's close enough to review.
>>>
>>> Thanks
>>> Martin
>>
>>
>