This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] fix powerpc64le bootstrap failure caused by r243661 (PR 78817)
On 12/19/2016 11:00 AM, Martin Sebor wrote:
On 12/19/2016 10:31 AM, Jeff Law wrote:
On 12/17/2016 02:55 PM, Martin Sebor wrote:
On 12/17/2016 01:01 PM, Markus Trippelsdorf wrote:
I agree that these warnings should probably not be issued, though
it's interesting to see where they come from. The calls are in
the code emitted by GCC, are reachable, and end up taking place
with the right Ubsan runtime recovery options. It turns out that
Ubsan transforms calls to nonnull functions into conditional
branches testing the argument for null, like so:
if (s == 0)
__builtin___ubsan_handle_nonnull_arg();
n = strlen (s);
and GCC then transforms those into
if (s == 0)
{
__builtin___ubsan_handle_nonnull_arg();
n = strlen (NULL);
}
When the ubsan_handle_nonnull_arg function returns to the caller
the call to strlen(NULL) is made.
So I'd like to see more complete dumps here.
The -Wnonnull warning can be reproduced with this C test case and
-fsantize=undefined:
char* f (const char *s)
{
unsigned n = __builtin_strlen (s) + 1;
char *d = __builtin_malloc (n);
if (!d)
return 0;
__builtin_memcpy (d, s, n);
return d;
}
The sanitizer emits the following code (I snipped the rest after
the call to malloc):
[ snip.]
THanks.
So this is a clear case of jump threading doing exactly what it is
supposed to do. We've duplicated code on the cold path to enable
removal of a test on the hot path. That is *exactly* what we want.
The fact that doing so creates a false positive is inherent in the
transformation. The question is does this happen enough to warrant
moving the warning early in the pipeline -- knowing that doing so will
likely reduce this kind of false positive, but may also introduce missed
warnings.
--- details follow --
So the key thing to note is that sanitization wants to add the test s !=
NULL before the strlen call and the memcpy call.
Looking at the .thread2 dump:
;; basic block 2, loop depth 0, count 0, freq 10000, maybe hot
;; prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
;; pred: ENTRY [100.0%] (FALLTHRU,EXECUTABLE)
if (s_7(D) == 0B)
goto <bb 3>; [0.0%]
else
goto <bb 4>; [100.0%]
;; succ: 4 [100.0%] (FALSE_VALUE,EXECUTABLE)
;; 3 [0.0%] (TRUE_VALUE,EXECUTABLE)
;; basic block 3, loop depth 0, count 0, freq 4
;; prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED)
;; pred: 2 [0.0%] (TRUE_VALUE,EXECUTABLE)
__builtin___ubsan_handle_nonnull_arg (&*.Lubsan_data0);
;; succ: 4 [100.0%] (FALLTHRU,EXECUTABLE)
;; basic block 4, loop depth 0, count 0, freq 10000, maybe hot
;; prev block 3, next block 5, flags: (NEW, REACHABLE, VISITED)
;; pred: 2 [100.0%] (FALSE_VALUE,EXECUTABLE)
;; 3 [100.0%] (FALLTHRU,EXECUTABLE)
_1 = __builtin_strlen (s_7(D));
_2 = (unsigned int) _1;
n_8 = _2 + 1;
_3 = (long unsigned int) n_8;
d_10 = __builtin_malloc (_3);
if (d_10 == 0B)
goto <bb 8>; [4.1%]
else
goto <bb 5>; [95.9%]
;; succ: 8 [4.1%] (TRUE_VALUE,EXECUTABLE)
;; 5 [95.9%] (FALSE_VALUE,EXECUTABLE)
;; basic block 5, loop depth 0, count 0, freq 9593, maybe hot
;; prev block 4, next block 6, flags: (NEW, REACHABLE, VISITED)
;; pred: 4 [95.9%] (FALSE_VALUE,EXECUTABLE)
if (s_7(D) == 0B)
goto <bb 6>; [0.0%]
else
goto <bb 7>; [100.0%]
;; succ: 7 [100.0%] (FALSE_VALUE,EXECUTABLE)
;; 6 [0.0%] (TRUE_VALUE,EXECUTABLE)
;; basic block 6, loop depth 0, count 0, freq 4
;; prev block 5, next block 7, flags: (NEW, REACHABLE, VISITED)
;; pred: 5 [0.0%] (TRUE_VALUE,EXECUTABLE)
__builtin___ubsan_handle_nonnull_arg (&*.Lubsan_data2);
;; succ: 7 [100.0%] (FALLTHRU,EXECUTABLE)
;; basic block 7, loop depth 0, count 0, freq 9593, maybe hot
;; prev block 6, next block 8, flags: (NEW, REACHABLE, VISITED)
;; pred: 5 [100.0%] (FALSE_VALUE,EXECUTABLE)
;; 6 [100.0%] (FALLTHRU,EXECUTABLE)
__builtin_memcpy (d_10, s_7(D), _3);
;; succ: 8 [100.0%] (FALLTHRU,EXECUTABLE)
;; basic block 8, loop depth 0, count 0, freq 10000, maybe hot
;; prev block 7, next block 1, flags: (NEW, REACHABLE, VISITED)
;; pred: 4 [4.1%] (TRUE_VALUE,EXECUTABLE)
;; 7 [100.0%] (FALLTHRU,EXECUTABLE)
# _4 = PHI <0B(4), d_10(7)>
return _4;
;; succ: EXIT [100.0%]
}
We can see the redundant test showing up in bb2 and bb5. Note the tests
are on the hot path.
Also note that 2->4->5 will result in a control transfer to 7 and
3->4->5 will result in a control transfer to 6. As seen in the .dom2 dump:
Registering jump thread: (3, 4) incoming edge; (4, 5) joiner; (5,
6) nocopy;
Registering jump thread: (2, 4) incoming edge; (4, 5) joiner; (5,
7) nocopy;
The "joiner" note means that we don't know where BB4 will go. But if it
goes to 5, then we will unconditionally transfer to 6 or 7 depending on
how we got to BB4.
We can't modify BB4 with the CFG in that state (again, remember, we
don't know where it will go).
To optimize this case we have to copy BB4 (call it BB4'). We redirect
the edge 3->4 to 3->4'. That has the side effect of making BB4 only
reachable from BB2. We've now isolated the two paths. BB4 happens to
be the cold path in this instance, BB4' is the hot path.
Showing just the first few blocks after some cleanups:
;; basic block 2, loop depth 0, count 0, freq 10000, maybe hot
;; prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
;; pred: ENTRY [100.0%] (FALLTHRU,EXECUTABLE)
if (s_7(D) == 0B)
goto <bb 3>; [0.0%]
else
goto <bb 4>; [100.0%]
;; succ: 4 [100.0%] (FALSE_VALUE,EXECUTABLE)
;; 3 [0.0%] (TRUE_VALUE,EXECUTABLE)
;; basic block 3, loop depth 0, count 0, freq 4
;; prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED)
;; pred: 2 [0.0%] (TRUE_VALUE,EXECUTABLE)
__builtin___ubsan_handle_nonnull_arg (&*.Lubsan_data0);
;; succ: 4 [100.0%] (FALLTHRU,EXECUTABLE)
;; basic block 4, loop depth 0, count 0, freq 10000, maybe hot
;; prev block 3, next block 5, flags: (NEW, REACHABLE, VISITED)
;; pred: 2 [100.0%] (FALSE_VALUE,EXECUTABLE)
;; 3 [100.0%] (FALLTHRU,EXECUTABLE)
_1 = __builtin_strlen (s_7(D));
_2 = (unsigned int) _1;
n_8 = _2 + 1;
_3 = (long unsigned int) n_8;
d_10 = __builtin_malloc (_3);
if (d_10 == 0B)
goto <bb 8>; [4.1%]
else
goto <bb 5>; [95.9%]
;; succ: 8 [4.1%] (TRUE_VALUE,EXECUTABLE)
;; 5 [95.9%] (FALSE_VALUE,EXECUTABLE)
We'll end up merging BB3 and BB4 (cold path). And you'll easily see
that the only value of s_7 going into that path is 0 which gets
propagated to the __builtin_strlen call and we eventually trigger the
warning. If you look at the full dump you'll see that we removed the
second test of s_7 (which was on the hot path).
Jeff