On 12/17/19 1:58 AM, Jakub Jelinek wrote:
Hi!
When looking at the PR, I wrote a cleanup patch with various things I've
noticed, with latest Martin's changes half of them aren't valid anymore, but
I found further ones.
So, besides formatting fixes, this patch tries to make sure the rng1 ranges
are meaningful even in some corner cases. The first hunk and half is about
what rng1 will be for single argument attribute where to the corresponding
argument INTEGER_CST is passed, vanilla trunk can stuck into rng1[0] ==
rng1[1] e.g. negative value with whatever precision the argument has
(say for int argument and -23 passed to it will return (size_t) -23,
but range will be [-23, -23]) or e.g. for arguments wider than size_t like
__int128 could return numbers above SIZE_MAX. Whatever the argument type
is, the attribute expects that value to be passed over to malloc/calloc or
similar functions and so it will be promoted or demoted to size_t. The
rng1[0] = wi::zero (rng1[1].get_precision ());
line is to avoid weird ranges and (conservatively) cover the whole range
of sizes the allocator function can return. E.g. if the ranges for the two
arguments are [2, SIZE_MAX] * [2, SIZE_MAX], the upper bound overflows and
gimple_call_alloc_size would return SIZE_MAX with [4, SIZE_MAX] in the rng1,
but that is not accurate, because due to the overflow also [0, 3] would be
possible.
We want allocations to appear to have "saturating behavior" so that
code like the following, for example, is diagnosed:
void* g (int n)
{
if (3 < n)
n = 3;
void *p = malloc (n);
strcpy (p, "12345"); // almost certain buffer overflow
return p;
}
PR 92942 tracks the missing warning in this case.
What you're doing in the patch, while necessary for an optimization,
would keep us from detecting these kinds of bugs.
Or for [SIZE_MAX - 2, SIZE_MAX] * [SIZE_MAX - 2, SIZE_MAX] where
the overflow is both on the low and upper bounds, the function would return
SIZE_MAX and set rng1 to [((__int128) SIZE_MAX - 2) * (SIZE_MAX - 2), SIZE_MAX]
i.e. range where low bound is much higher than upper bound (== invalid
range). The patch will just use [0, SIZE_MAX] range in those cases.
GCC diagnoses excessive allocations by -Walloc-size-larger-than
when the product exceeds PTRDIFF_MAX, so I'm less concerned about
diagnosing stores into them, but as above, the conservative approach
that's necessary for optimization is the opposite of what's needed
in order to detect bugs.
And the second hunk in tree-ssa-strlen.c is a fix for pointer comparison of
INTEGER_CSTs, while we cache INTEGER_CSTs and INTEGER_CSTs of the same type
with the same value and no overflow will compare equal, in that function, it
is actually quite unlikely they will have the same type, because destsize is
most likely a sizetype integer, while len most likely size_t (aka
size_type_node), but could be anything else that passes
useless_type_conversion_p).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
I appreciate a cleanup but I don't have the impression this patch
does clean anything up. Because of all the formatting changes and
no tests the effect of the changes isn't as clear as it should be.
(I wish you would resist the urge to reformat existing code on this
scale while also making changes with an observable effect in
the same diff.) But thanks to the detailed explanation above
I think I can safely say that the builtins.c changes are not in
line with what I would like to see.
FWIW, a change I would welcome is replacing the *POFF argument to
compute_objsize with a range like gimple_call_alloc_size() takes.
That would let the function handle offsets in negative ranges and
detect more buffer overflows, such as the one in PR 92939. (I plan
to fix that along with PR 92942 for GCC 11).
Martin
2019-12-17 Jakub Jelinek <ja...@redhat.com>
PR tree-optimization/92868
* builtins.c (gimple_call_alloc_size): If there is only one size
argument
and INTEGER_CST is passed to it, call get_range only on the converted
value.
For overflow, set rng1[0] to zero. Formatting fix.
(compute_objsize): Formatting fix.
* tree-ssa-strlen.c (maybe_warn_overflow): Remove spurious ; after }.
Use operand_equal_p instead of pointer comparison to compare
INTEGER_CSTs.
Formatting fixes.
--- gcc/builtins.c.jj 2019-12-14 23:19:40.861879033 +0100
+++ gcc/builtins.c 2019-12-16 10:00:27.858664110 +0100
@@ -3749,6 +3749,13 @@ gimple_call_alloc_size (gimple *stmt, wi
}
tree size = gimple_call_arg (stmt, argidx1);
+ if (argidx2 > nargs && TREE_CODE (size) == INTEGER_CST)
+ {
+ size = fold_convert (sizetype, size);
+ if (rng1)
+ get_range (size, rng1, rvals);
+ return size;
+ }
wide_int rng1_buf[2];
/* If RNG1 is not set, use the buffer. */
@@ -3758,12 +3765,10 @@ gimple_call_alloc_size (gimple *stmt, wi
if (!get_range (size, rng1, rvals))
return NULL_TREE;
- if (argidx2 > nargs && TREE_CODE (size) == INTEGER_CST)
- return fold_convert (sizetype, size);
-
/* To handle ranges do the math in wide_int and return the product
of the upper bounds as a constant. Ignore anti-ranges. */
- tree n = argidx2 < nargs ? gimple_call_arg (stmt, argidx2) :
integer_one_node;
+ tree n
+ = argidx2 < nargs ? gimple_call_arg (stmt, argidx2) : integer_one_node;
wide_int rng2[2];
if (!get_range (n, rng2, rvals))
return NULL_TREE;
@@ -3783,6 +3788,7 @@ gimple_call_alloc_size (gimple *stmt, wi
if (wi::gtu_p (rng1[1], wi::to_wide (size_max, prec)))
{
rng1[1] = wi::to_wide (size_max);
+ rng1[0] = wi::zero (rng1[1].get_precision ());
return size_max;
}
@@ -3962,8 +3968,7 @@ compute_objsize (tree dest, int ostype,
if (!ostype)
return NULL_TREE;
- if (TREE_CODE (dest) == ARRAY_REF
- || TREE_CODE (dest) == MEM_REF)
+ if (TREE_CODE (dest) == ARRAY_REF || TREE_CODE (dest) == MEM_REF)
{
tree ref = TREE_OPERAND (dest, 0);
tree off = TREE_OPERAND (dest, 1);
--- gcc/tree-ssa-strlen.c.jj 2019-12-14 23:19:40.860879048 +0100
+++ gcc/tree-ssa-strlen.c 2019-12-16 10:14:51.061654913 +0100
@@ -2039,7 +2039,7 @@ maybe_warn_overflow (gimple *stmt, tree
{
sizrng[0] = wi::zero (siz_prec);
sizrng[1] = wi::to_wide (TYPE_MAX_VALUE (sizetype));
- };
+ }
sizrng[0] = sizrng[0].from (sizrng[0], siz_prec, UNSIGNED);
sizrng[1] = sizrng[1].from (sizrng[1], siz_prec, UNSIGNED);
@@ -2048,7 +2048,11 @@ maybe_warn_overflow (gimple *stmt, tree
and the offset into the destination is zero. This might happen
in the case of a pair of malloc and memset calls to allocate
an object and clear it as if by calloc. */
- if (destsize == len && !plus_one && offrng[0] == 0 && offrng[0] == offrng[1])
+ if (destsize
+ && operand_equal_p (destsize, len, 0)
+ && !plus_one
+ && offrng[0] == 0
+ && offrng[0] == offrng[1])
return;
wide_int lenrng[2];
@@ -2096,13 +2100,11 @@ maybe_warn_overflow (gimple *stmt, tree
spcrng[1] -= wi::ltu_p (offrng[0], spcrng[1]) ? offrng[0] : spcrng[1];
}
- if (wi::leu_p (lenrng[0], spcrng[0])
- && wi::leu_p (lenrng[1], spcrng[1]))
+ if (wi::leu_p (lenrng[0], spcrng[0]) && wi::leu_p (lenrng[1], spcrng[1]))
return;
if (lenrng[0] == spcrng[1]
- && (len != destsize
- || !si || !is_strlen_related_p (si->ptr, len)))
+ && (len != destsize || !si || !is_strlen_related_p (si->ptr, len)))
return;
location_t loc = gimple_nonartificial_location (stmt);
@@ -2113,8 +2115,7 @@ maybe_warn_overflow (gimple *stmt, tree
bool warned = false;
if (wi::leu_p (lenrng[0], spcrng[1]))
{
- if (len != destsize
- && (!si || !is_strlen_related_p (si->ptr, len)))
+ if (len != destsize && (!si || !is_strlen_related_p (si->ptr, len)))
return;
warned = (writefn
Jakub
