On 11/27/2017 05:44 AM, Richard Biener wrote:
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.
Sure.
@@ -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).
I remember you pointing it out earlier and agreeing that warning
options should not disable/enable optimizations. In this case,
if GCC can avoid the undefined behavior by folding a call that
would otherwise result from calling the library function, it
makes sense to go ahead and fold it. I forgot to allow for
that in the last revision of my patch but I've fixed it in
the updated one I just posted.
+ 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.
I added a comment to the classes to make it clearer. Although
now that the classes are in a dedicated warning pass there is
little risk of someone misusing them for optimization.
Thanks
Martin
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
