[Bug sanitizer/79265] -fsanitize=undefined inserts unnecessary null pointer tests

["msebor at gcc dot gnu.org" <gcc-bugzilla at gcc dot gnu dot org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79265

--- Comment #2 from Martin Sebor <msebor at gcc dot gnu.org> ---
As I mentioned, the unnecessary null tests inserted by the sanitizer are
usually removed by subsequent optimization passes so they serve no useful
purpose.  They do, however, make GCC work harder which can cause trouble when
they result in calls to the nonnull functions with constant null pointers as a
result of jump threading (as we have seen, and as demonstrated by the test case
below).

In the test case in comment #0 GCC eliminates the null pointer checks in f0
(local array) and f2 (alloca) but retains those in f1 (VLA) and f3 (attribute
nonnull).  In all of f0, f1, and f2, the object whose address is tested against
null is on the stack, so emitting a null check in all three is equally
important or equally pointless (depending on one's point of view).

The only case where a valid pointer cannot point into the stack is f3.  In this
case the pointer is "guaranteed" to be non-null by the function being declared
attribute returns_nonnull.  As I also mentioned, there may be some value in
emitting the null pointer check here in case the returns_nonnull function
returns null despite its promise.  Even then, however, there is no point in
emitting the check or the handler more than once when the pointer is being
passed to a nonnull argument of a built-in function like sprintf (or strcpy)
that we know will try to write to it and crash.  Doing so only leads to
unnecessarily inefficient code and can cause false positives.  (The repeated
calls to the UBSan null handler are also unnecessary in these cases and suggest
missing logic in the sanitizer.)

Here's another test case highlighting the returns_nonnull problem discussed
above:

$ cat u.c && gcc -O3 -S -Wall -fdump-tree-optimized=/dev/stdout
-fsanitize=undefined u.c

char* __attribute__ ((alloc_size (1), returns_nonnull))
myalloc (unsigned);

void sink (void*);

void __attribute__ ((noinline, nonnull))
f (unsigned n)
{
  char *p = myalloc(n);

  *p = 'a';

  __builtin_sprintf (p, "%i", 1);
  sink (p);

  __builtin_sprintf (p, "%i", 2);
  sink (p);
}
u.c: In function ‘f’:
u.c:13:3: warning: null destination pointer [-Wformat-overflow=]
   __builtin_sprintf (p, "%i", 1);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

;; Function f (f, funcdef_no=0, decl_uid=2121, cgraph_uid=0, symbol_order=0)

__attribute__((noinline))
f (unsigned int n)
{
  char * p;

  <bb 2> [100.00%]:
  p_4 = myalloc (n_2(D));
  if (p_4 == 0B)
    goto <bb 3>; [0.04%]
  else
    goto <bb 4>; [99.96%]

  <bb 3> [0.00%]:
  __builtin___ubsan_handle_type_mismatch (&*.Lubsan_data4, 0);

  <bb 4> [100.00%]:
  *p_4 = 97;
  if (p_4 == 0B)
    goto <bb 5>; [0.04%]
  else
    goto <bb 7>; [99.96%]

  <bb 5> [0.04%]:
  __builtin___ubsan_handle_nonnull_arg (&*.Lubsan_data0);
  __builtin_sprintf (0B, "%i", 1);
  sink (0B);
  __builtin___ubsan_handle_nonnull_arg (&*.Lubsan_data2);

  <bb 6> [100.00%]:
  __builtin_sprintf (p_4, "%i", 2);
  sink (p_4); [tail call]
  return;

  <bb 7> [99.96%]:
  __builtin_sprintf (p_4, "%i", 1);
  sink (p_4);
  goto <bb 6>; [100.00%]

}