On 12/18/19 1:49 AM, Jakub Jelinek wrote:
On Tue, Dec 17, 2019 at 09:53:43AM -0700, Martin Sebor wrote:
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.
As written, there were two real changes in gimple_call_alloc_size,
one in maybe_warn_overflow and the rest formatting fixes (which I really
can't ignore, e.g. semicolon after if (...) { ... }; ?).
From the above, it seems you are talking about just one change in
gimple_call_alloc_size (the setting of rng1[0] to 0 on overflow).
So, let's talk first about the first real change in gimple_call_alloc_size.
Without it, you get garbage on testcase like:
/* { dg-do compile { target lp64 } } */
/* { dg-options "-O2 -Wall" } */
__attribute__((noipa, alloc_size (1)))
char *
foo (int a)
{
return (char *) __builtin_malloc (a);
}
void
bar (char *q)
{
char *p = foo (-13);
if (!p)
return;
__builtin_memcpy (p, q, (__SIZE_TYPE__) 0x100000002ULL);
}
alloc_size_test.c: In function ‘bar’:
alloc_size_test.c:17:3: warning: ‘__builtin_memcpy’ writing 4294967298 bytes
into a region of size 4294967283 [-Wstringop-overflow=]
17 | __builtin_memcpy (p, q, (__SIZE_TYPE__) 0x100000002ULL);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
alloc_size_test.c:14:13: note: at offset 0 to an object with size 4294967283
allocated by ‘foo’ here
14 | char *p = foo (-13);
| ^~~~~~~~~
alloc_size_test.c:14:13: warning: argument 1 value ‘-13’ is negative
[-Walloc-size-larger-than=]
alloc_size_test.c:6:1: note: in a call to allocation function ‘foo’ declared
here
6 | foo (int a)
| ^~~
The first warning is wrong, there is no object with size 4294967283 aka
-13U, the allocation will almost certainly fail
The warning is based on the alloc_size argument to foo. It cannot
infer whether or not the allocation will succeed based on foo's body.
But since no size can be negative the warning should conservatively
treat it as if it was zero. That's what I would like to see fixed.
and there will be nothing
wrong in the testcase, but if it wouldn't fail, it would need to allocate
-13UL aka 18446744073709551603 bytes. And the reason is that
gimple_call_alloc_size leaves bogus rng1[0] and rng1[1], of -13 with
precision 32, which the caller than extends to siz_prec, but with UNSIGNED
extension, which is reasonable assumption that it is given size (UNSIGNED)
wide_ints.
The second hunk, there can be still several cases, e.g. both low bound and
upper bound could overflow, such as on calloc (SIZE_MAX / 2 + 2, SIZE_MAX /
2 + 2). In this case, the code would produce a range of (say let's assume
ilp32 in this case) of [0x4000000100000001, 0xffffffff]. What the caller
will do with such thing is hard to predict, there is from UNSIGNED siz_prec
conversion. If you don't want the function to return real range of possible
values, but something else, it should be document what it is and not call it
range. For the reasons you stated, perhaps for warnings (but never for code
generation!) it could be useful to have both range and likely range, where
the latter would be on the assumption that no overflow happens and so e.g.
for [64, SIZE_MAX] * [64, SIZE_MAX] it could return range of [0, SIZE_MAX]
and likely range of [64 * 64, SIZE_MAX].
For standard functions like calloc, gimple_call_alloc_size should
return the valid size of the allocated object in the range [0,
SIZE_MAX] (it should really be [0, MAXOBJSIZE]). The size of
the object that would be allocated by calloc (SIZE_MAX / 2 - 2,
SIZE_MAX / 2 - 2) is outside that range so calloc must fail and
gimple_call_alloc_size should set the size to zero.
For user-defined functions that take signed arguments, negative
values should be treated as zero and the warning should be issued
for any access to the object.
Martin
And for the real change in maybe_warn_overflow, you can just try in the
debugger or using gprof see when that if (early out) will ever trigger, I'd
bet very rarely.
char *
foo (void)
{
char *p = __builtin_calloc (32, 32);
__builtin_memset (p, ' ', 32 * 32);
return p;
}
gdb --args ./cc1 -quiet -O2 -Wall test.c
b tree-ssa-strlen.c:2051
r
p destsize
$1 = <integer_cst 0x7fffea94a0d8>
p len
$2 = <integer_cst 0x7fffea929f90>
p debug_generic_stmt (destsize)
1024
p debug_generic_stmt (len)
1024
Jakub
