On Thu, Nov 16, 2017 at 10:29 PM, Martin Sebor <mse...@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
>>
>>
>
