[PATCH] correct handling of offsets in bounds warnings (PR 89350)
The false positive in PR89350 is due to -Wstringop-overflow
trusting that the sizetype offset in POINTER_PLUS_EXPR means
the offset is, in fact, unsigned. Avoiding the false positive
in the cases when this isn't so is trivial but comes at a cost
of false negatives. Avoiding those will, I expect, require
enhancing the compute_builtin_object_size() function and that
seems risky at this stage so I would like to defer that until
stage 1. Except in the instance of memset, the false positives
also aren't too serious because the same problem is also
diagnosed by the -Warray-bounds warning in the wrestrict pass.
Unfortunately, the wrestrict pass only handles copy functions
and not memset.
With that as background, the attached patch avoids
the -Wstringop-overflow false positive by disabling the warning
for offsets whose lower bound is positive and upper bound negative.
To avoid the false negatives for memset the patch lets the wrestrict
pass handle the function (for the bounds checking only). While
testing this I noticed that the wrestrict pass makes the same
assumption about offsets, so it too is susceptible to similar
false positives. The rest of the patch corrects this problem
n the wrestrict pass. Because the pass doesn't depend on
the compute_builtin_object_size() function as much as
-Wstringop-overflow, the fix does not cause false positives (at
least none that I came across).
Tested on x86_64-linux.
Martin
PR tree-optimization/89350 - Wrong -Wstringop-overflow= warning since r261518
gcc/ChangeLog:
PR tree-optimization/89350
* builtins.c (compute_objsize): Also ignore offsets whose upper
bound is negative.
* gimple-ssa-warn-restrict.c (builtin_memref): Add new member.
(builtin_memref::builtin_memref): Initialize new member.
Allow EXPR to be null.
(builtin_memref::extend_offset_range): Replace local with a member.
Avoid assuming pointer offsets are unsigned.
(builtin_memref::set_base_and_offset): Determine base object
before computing offset range.
(builtin_access::builtin_access): Handle memset.
(builtin_access::generic_overlap): Replace local with a member.
(builtin_access::strcat_overlap): Same.
(builtin_access::overlap): Same.
(maybe_diag_overlap): Same.
(maybe_diag_access_bounds): Same.
(wrestrict_dom_walker::check_call): Handle memset.
(check_bounds_or_overlap): Same.
gcc/testsuite/ChangeLog:
PR tree-optimization/89350
* gcc.dg/Wstringop-overflow.c: Xfail overly ambitious tests.
* gcc.dg/Wstringop-overflow-10.c: New test.
* gcc.dg/Wstringop-overflow-11.c: New test.
* gcc.dg/pr40340-1.c: Adjust expected warning.
* gcc.dg/pr40340-2.c: Same.
* gcc.dg/pr40340-4.c: Same.
* gcc.dg/pr40340-5.c: Same.
Index: gcc/builtins.c
===
--- gcc/builtins.c (revision 269190)
+++ gcc/builtins.c (working copy)
@@ -3652,7 +3652,8 @@ compute_objsize (tree dest, int ostype)
/* Ignore negative offsets for now. For others,
use the lower bound as the most optimistic
estimate of the (remaining)size. */
- if (wi::sign_mask (min))
+ if (wi::sign_mask (min)
+ || wi::sign_mask (max))
;
else if (wi::ltu_p (min, wisiz))
return wide_int_to_tree (TREE_TYPE (size),
Index: gcc/gimple-ssa-warn-restrict.c
===
--- gcc/gimple-ssa-warn-restrict.c (revision 269190)
+++ gcc/gimple-ssa-warn-restrict.c (working copy)
@@ -147,6 +147,9 @@ struct builtin_memref
/* The size range of the access to this reference. */
offset_int sizrange[2];
+ /* Cached result of get_max_objsize(). */
+ const offset_int maxobjsize;
+
/* True for "bounded" string functions like strncat, and strncpy
and their variants that specify either an exact or upper bound
on the size of the accesses they perform. For strncat both
@@ -233,6 +236,7 @@ builtin_memref::builtin_memref (tree expr, tree si
refoff (HOST_WIDE_INT_MIN),
offrange (),
sizrange (),
+ maxobjsize (tree_to_shwi (max_object_size ())),
strbounded_p ()
{
/* Unfortunately, wide_int default ctor is a no-op so array members
@@ -240,7 +244,8 @@ builtin_memref::builtin_memref (tree expr, tree si
offrange[0] = offrange[1] = 0;
sizrange[0] = sizrange[1] = 0;
- const offset_int maxobjsize = tree_to_shwi (max_object_size ());
+ if (!expr)
+return;
/* Find the BASE object or pointer referenced by EXPR and set
the offset range OFFRANGE in the process. */
@@ -292,13 +297,13 @@ builtin_memref::builtin_memref (tree expr, tree si
}
}
-/* Ctor helper to set or extend OFFRANGE based on the OFFSET argument. */
+/* Ctor helper to set or extend OFFRANGE based on the OFFSET argument.
+ Pointer offsets are represented as unsigned sizetype but must be
+ treated as signed. */
void
builtin_memref::extend_offset_range (tree offset)
{
- const offset_int maxobjsize = tree_to_shwi (max_object_size ());
-
if (TREE_CODE (offset) == INTEGER_
Re: [PATCH] correct handling of offsets in bounds warnings (PR 89350)
Please disregard the original patch and consider the attached
version instead.
On 2/26/19 5:03 PM, Martin Sebor wrote:
The false positive in PR89350 is due to -Wstringop-overflow
trusting that the sizetype offset in POINTER_PLUS_EXPR means
the offset is, in fact, unsigned. Avoiding the false positive
in the cases when this isn't so is trivial but comes at a cost
of false negatives. Avoiding those will, I expect, require
enhancing the compute_builtin_object_size() function and that
seems risky at this stage so I would like to defer that until
stage 1. Except in the instance of memset, the false positives
also aren't too serious because the same problem is also
diagnosed by the -Warray-bounds warning in the wrestrict pass.
Unfortunately, the wrestrict pass only handles copy functions
and not memset.
With that as background, the attached patch avoids
the -Wstringop-overflow false positive by disabling the warning
for offsets whose lower bound is positive and upper bound negative.
To avoid the false negatives for memset the patch lets the wrestrict
pass handle the function (for the bounds checking only). While
testing this I noticed that the wrestrict pass makes the same
assumption about offsets, so it too is susceptible to similar
false positives. The rest of the patch corrects this problem
n the wrestrict pass. Because the pass doesn't depend on
the compute_builtin_object_size() function as much as
-Wstringop-overflow, the fix does not cause false positives (at
least none that I came across).
Tested on x86_64-linux.
Martin
PR tree-optimization/89350 - Wrong -Wstringop-overflow= warning since r261518
gcc/ChangeLog:
PR tree-optimization/89350
* builtins.c (compute_objsize): Also ignore offsets whose upper
bound is negative.
* gimple-ssa-warn-restrict.c (builtin_memref): Add new member.
(builtin_memref::builtin_memref): Initialize new member.
Allow EXPR to be null.
(builtin_memref::extend_offset_range): Replace local with a member.
Avoid assuming pointer offsets are unsigned.
(builtin_memref::set_base_and_offset): Determine base object
before computing offset range.
(builtin_access::builtin_access): Handle memset.
(builtin_access::generic_overlap): Replace local with a member.
(builtin_access::strcat_overlap): Same.
(builtin_access::overlap): Same.
(maybe_diag_overlap): Same.
(maybe_diag_access_bounds): Same.
(wrestrict_dom_walker::check_call): Handle memset.
(check_bounds_or_overlap): Same.
gcc/testsuite/ChangeLog:
PR tree-optimization/89350
* gcc.dg/Wstringop-overflow.c: Xfail overly ambitious tests.
* gcc.dg/Wstringop-overflow-10.c: New test.
* gcc.dg/Wstringop-overflow-11.c: New test.
* gcc.dg/pr89350.c: New test.
* gcc.dg/pr40340-1.c: Adjust expected warning.
* gcc.dg/pr40340-2.c: Same.
* gcc.dg/pr40340-4.c: Same.
* gcc.dg/pr40340-5.c: Same.
Index: gcc/builtins.c
===
--- gcc/builtins.c (revision 269190)
+++ gcc/builtins.c (working copy)
@@ -3652,7 +3652,8 @@ compute_objsize (tree dest, int ostype)
/* Ignore negative offsets for now. For others,
use the lower bound as the most optimistic
estimate of the (remaining)size. */
- if (wi::sign_mask (min))
+ if (wi::sign_mask (min)
+ || wi::sign_mask (max))
;
else if (wi::ltu_p (min, wisiz))
return wide_int_to_tree (TREE_TYPE (size),
Index: gcc/gimple-ssa-warn-restrict.c
===
--- gcc/gimple-ssa-warn-restrict.c (revision 269190)
+++ gcc/gimple-ssa-warn-restrict.c (working copy)
@@ -147,6 +147,9 @@ struct builtin_memref
/* The size range of the access to this reference. */
offset_int sizrange[2];
+ /* Cached result of get_max_objsize(). */
+ const offset_int maxobjsize;
+
/* True for "bounded" string functions like strncat, and strncpy
and their variants that specify either an exact or upper bound
on the size of the accesses they perform. For strncat both
@@ -233,6 +236,7 @@ builtin_memref::builtin_memref (tree expr, tree si
refoff (HOST_WIDE_INT_MIN),
offrange (),
sizrange (),
+ maxobjsize (tree_to_shwi (max_object_size ())),
strbounded_p ()
{
/* Unfortunately, wide_int default ctor is a no-op so array members
@@ -240,7 +244,8 @@ builtin_memref::builtin_memref (tree expr, tree si
offrange[0] = offrange[1] = 0;
sizrange[0] = sizrange[1] = 0;
- const offset_int maxobjsize = tree_to_shwi (max_object_size ());
+ if (!expr)
+return;
/* Find the BASE object or pointer referenced by EXPR and set
the offset range OFFRANGE in the process. */
@@ -292,13 +297,13 @@ builtin_memref::builtin_memref (tree expr, tree si
}
}
-/* Ctor helper to set or extend OFFRANGE based on the OFFSET argument. */
+/* Ctor helper to set or extend OFFRANGE based on the OFFSET argument.
+ Pointer offsets are represented as unsigned sizetype but must be
+ treated as signed. */
void
builti
PING [PATCH] correct handling of offsets in bounds warnings (PR 89350)
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-02/msg02029.html (This is marked as a P1 regression.) On 2/26/19 6:32 PM, Martin Sebor wrote: Please disregard the original patch and consider the attached version instead. On 2/26/19 5:03 PM, Martin Sebor wrote: The false positive in PR89350 is due to -Wstringop-overflow trusting that the sizetype offset in POINTER_PLUS_EXPR means the offset is, in fact, unsigned. Avoiding the false positive in the cases when this isn't so is trivial but comes at a cost of false negatives. Avoiding those will, I expect, require enhancing the compute_builtin_object_size() function and that seems risky at this stage so I would like to defer that until stage 1. Except in the instance of memset, the false positives also aren't too serious because the same problem is also diagnosed by the -Warray-bounds warning in the wrestrict pass. Unfortunately, the wrestrict pass only handles copy functions and not memset. With that as background, the attached patch avoids the -Wstringop-overflow false positive by disabling the warning for offsets whose lower bound is positive and upper bound negative. To avoid the false negatives for memset the patch lets the wrestrict pass handle the function (for the bounds checking only). While testing this I noticed that the wrestrict pass makes the same assumption about offsets, so it too is susceptible to similar false positives. The rest of the patch corrects this problem n the wrestrict pass. Because the pass doesn't depend on the compute_builtin_object_size() function as much as -Wstringop-overflow, the fix does not cause false positives (at least none that I came across). Tested on x86_64-linux. Martin
Re: [PATCH] correct handling of offsets in bounds warnings (PR 89350)
On 2/26/19 6:32 PM, Martin Sebor wrote: > Please disregard the original patch and consider the attached > version instead. > > On 2/26/19 5:03 PM, Martin Sebor wrote: >> The false positive in PR89350 is due to -Wstringop-overflow >> trusting that the sizetype offset in POINTER_PLUS_EXPR means >> the offset is, in fact, unsigned. Avoiding the false positive >> in the cases when this isn't so is trivial but comes at a cost >> of false negatives. Avoiding those will, I expect, require >> enhancing the compute_builtin_object_size() function and that >> seems risky at this stage so I would like to defer that until >> stage 1. Except in the instance of memset, the false positives >> also aren't too serious because the same problem is also >> diagnosed by the -Warray-bounds warning in the wrestrict pass. >> Unfortunately, the wrestrict pass only handles copy functions >> and not memset. >> >> With that as background, the attached patch avoids >> the -Wstringop-overflow false positive by disabling the warning >> for offsets whose lower bound is positive and upper bound negative. >> To avoid the false negatives for memset the patch lets the wrestrict >> pass handle the function (for the bounds checking only). While >> testing this I noticed that the wrestrict pass makes the same >> assumption about offsets, so it too is susceptible to similar >> false positives. The rest of the patch corrects this problem >> n the wrestrict pass. Because the pass doesn't depend on >> the compute_builtin_object_size() function as much as >> -Wstringop-overflow, the fix does not cause false positives (at >> least none that I came across). >> >> Tested on x86_64-linux. >> >> Martin > > > gcc-89350.diff > > PR tree-optimization/89350 - Wrong -Wstringop-overflow= warning since r261518 > > gcc/ChangeLog: > > PR tree-optimization/89350 > * builtins.c (compute_objsize): Also ignore offsets whose upper > bound is negative. > * gimple-ssa-warn-restrict.c (builtin_memref): Add new member. > (builtin_memref::builtin_memref): Initialize new member. > Allow EXPR to be null. > (builtin_memref::extend_offset_range): Replace local with a member. > Avoid assuming pointer offsets are unsigned. > (builtin_memref::set_base_and_offset): Determine base object > before computing offset range. > (builtin_access::builtin_access): Handle memset. > (builtin_access::generic_overlap): Replace local with a member. > (builtin_access::strcat_overlap): Same. > (builtin_access::overlap): Same. > (maybe_diag_overlap): Same. > (maybe_diag_access_bounds): Same. > (wrestrict_dom_walker::check_call): Handle memset. > (check_bounds_or_overlap): Same. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/89350 > * gcc.dg/Wstringop-overflow.c: Xfail overly ambitious tests. > * gcc.dg/Wstringop-overflow-10.c: New test. > * gcc.dg/Wstringop-overflow-11.c: New test. > * gcc.dg/pr89350.c: New test. > * gcc.dg/pr40340-1.c: Adjust expected warning. > * gcc.dg/pr40340-2.c: Same. > * gcc.dg/pr40340-4.c: Same. > * gcc.dg/pr40340-5.c: Same. OK. And just to be clear, totally agree with not trying to change c_b_o_s to return a range at this point in the release cycle. jeff
PING #2 [PATCH] correct handling of offsets in bounds warnings (PR 89350)
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-02/msg02029.html On 3/6/19 2:40 PM, Martin Sebor wrote: Ping: https://gcc.gnu.org/ml/gcc-patches/2019-02/msg02029.html (This is marked as a P1 regression.) On 2/26/19 6:32 PM, Martin Sebor wrote: Please disregard the original patch and consider the attached version instead. On 2/26/19 5:03 PM, Martin Sebor wrote: The false positive in PR89350 is due to -Wstringop-overflow trusting that the sizetype offset in POINTER_PLUS_EXPR means the offset is, in fact, unsigned. Avoiding the false positive in the cases when this isn't so is trivial but comes at a cost of false negatives. Avoiding those will, I expect, require enhancing the compute_builtin_object_size() function and that seems risky at this stage so I would like to defer that until stage 1. Except in the instance of memset, the false positives also aren't too serious because the same problem is also diagnosed by the -Warray-bounds warning in the wrestrict pass. Unfortunately, the wrestrict pass only handles copy functions and not memset. With that as background, the attached patch avoids the -Wstringop-overflow false positive by disabling the warning for offsets whose lower bound is positive and upper bound negative. To avoid the false negatives for memset the patch lets the wrestrict pass handle the function (for the bounds checking only). While testing this I noticed that the wrestrict pass makes the same assumption about offsets, so it too is susceptible to similar false positives. The rest of the patch corrects this problem n the wrestrict pass. Because the pass doesn't depend on the compute_builtin_object_size() function as much as -Wstringop-overflow, the fix does not cause false positives (at least none that I came across). Tested on x86_64-linux. Martin
PING #3 [PATCH] correct handling of offsets in bounds warnings (PR 89350)
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-02/msg02029.html As an aside, I introduced the same mistake/false positive in three places: -Wstringop-overflow in builtins.c, -Warray-bounds in gimple-ssa-warn-restrict.c, and -Warray-bounds in tree-vrp.c. This patch fixes the first two. I'm working on PR 89720 to fix the last one. On 3/11/19 8:52 PM, Martin Sebor wrote: Ping: https://gcc.gnu.org/ml/gcc-patches/2019-02/msg02029.html On 3/6/19 2:40 PM, Martin Sebor wrote: Ping: https://gcc.gnu.org/ml/gcc-patches/2019-02/msg02029.html (This is marked as a P1 regression.) On 2/26/19 6:32 PM, Martin Sebor wrote: Please disregard the original patch and consider the attached version instead. On 2/26/19 5:03 PM, Martin Sebor wrote: The false positive in PR89350 is due to -Wstringop-overflow trusting that the sizetype offset in POINTER_PLUS_EXPR means the offset is, in fact, unsigned. Avoiding the false positive in the cases when this isn't so is trivial but comes at a cost of false negatives. Avoiding those will, I expect, require enhancing the compute_builtin_object_size() function and that seems risky at this stage so I would like to defer that until stage 1. Except in the instance of memset, the false positives also aren't too serious because the same problem is also diagnosed by the -Warray-bounds warning in the wrestrict pass. Unfortunately, the wrestrict pass only handles copy functions and not memset. With that as background, the attached patch avoids the -Wstringop-overflow false positive by disabling the warning for offsets whose lower bound is positive and upper bound negative. To avoid the false negatives for memset the patch lets the wrestrict pass handle the function (for the bounds checking only). While testing this I noticed that the wrestrict pass makes the same assumption about offsets, so it too is susceptible to similar false positives. The rest of the patch corrects this problem n the wrestrict pass. Because the pass doesn't depend on the compute_builtin_object_size() function as much as -Wstringop-overflow, the fix does not cause false positives (at least none that I came across). Tested on x86_64-linux. Martin
