On 12/17/2016 01:01 PM, Markus Trippelsdorf wrote:
On 2016.12.16 at 18:27 +0100, Jakub Jelinek wrote:
On Fri, Dec 16, 2016 at 10:10:00AM -0700, Martin Sebor wrote:
No. The first call to sm_read_sector just doesn't exit. So it is warning
about dead code.
If the code is dead then GCC should eliminate it. With it eliminated
There is (especially with jump threading, but not limited to that, other
optimizations may result in that too), code that even very smart optimizing
compiler isn't able to prove that is dead.
the warning would be gone. The warning isn't smart and it doesn't
try to be. It only works with what GCC gives it. In this case the
dump shows that GCC thinks the code is reachable. If it isn't that
would seem to highlight a missed optimization opportunity, not a need
to make the warning smarter than the optimizer.
No, it highlights that the warning is done in a wrong place where it suffers
from too many false positives.
Another issue with Martin's patch is that it adds many false positives
when one uses -fsanitize=undefined, e.g.:
% cat test.ii
struct A {
char *msg;
A(const char *);
};
A::A(const char *p1) {
msg = new char[__builtin_strlen(p1) + 1];
__builtin_strcpy(msg, p1);
}
% g++ -Wall -O2 -c test.ii
% g++ -Wall -fsanitize=undefined -O2 -c test.ii
test.ii: In constructor ‘A::A(const char*)’:
test.ii:6:34: warning: argument 1 null where non-null expected [-Wnonnull]
msg = new char[__builtin_strlen(p1) + 1];
~~~~~~~~~~~~~~~~^~~~
test.ii:6:34: note: in a call to built-in function ‘long unsigned int
__builtin_strlen(const char*)’
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.
This doesn't happen when -fno-sanitize-recover=undefined is used
when compiling the file because then Ubsan inserts calls to
__builtin___ubsan_handle_nonnull_arg_abort instead which is declared
noreturn.
It would be easy to suppress these warnings when -fsantize=undefined
is used. Distinguishing the Ubsan-inserted calls from those in the
source code would be more challenging. Implementing the warning as
a pass that runs before the Ubsan pass gets around the problem.
Martin
