[Diagnostics] Teach -Wnull-dereference about address_space attribute

[xbolva00]

Summary

Clang should not warn for:

test.c:2:12: warning: indirection of non-volatile null pointer will be deleted,

  not trap [-Wnull-dereference]
return *(int __attribute__((address_space(256))) *) 0;
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Solves PR42292.

[xbolva00]

clang/lib/Sema/SemaExpr.cpp

484–491

With this patch, we no longer warn for:

*(int attribute((address_space(0))) *) 0;

Worth to handle this case?

[xbolva00]

Added testcase with typedefed int.

[xbolva00]

Ping :)

[aaron.ballman]

LGTM with a testing request.

clang/lib/Sema/SemaExpr.cpp

484–491

I mildly think it would be useful to warn in that case, but I'm fine with that being done later.

clang/test/Sema/exprs.c

192

Can you add a test case for dereferencing for a read instead of a write, just to ensure we don't diagnose that case?

You may want to add the address_space(0) test case as well, with a FIXME that it should warn at some point.

[xbolva00]

More tests.
Handle attribute((address_space(0))).

[xbolva00]

Removed unused variable.

[xbolva00]

Fixed comments.

[leonardchan]

Hi. I have a suspicion this patch is causing a segfault for us when building zircon: https://ci.chromium.org/p/fuchsia/builders/ci/fuchsia-x64-debug-clang/b8897388724384628544

I'm almost done with a bisect and will provide a reproducer if this is the case. Just raising awareness in the meantime.

[leonardchan]

Yup, a bisect shows it was this patch that caused it. I filed a bug for tracking at https://bugs.llvm.org/show_bug.cgi?id=43950 and attached a reproducer. Could you look into this? Thanks.

clang/lib/Sema/SemaExpr.cpp

487

If I were to guess what would cause the segfault, I'd probably think it was the getPointeeType().getAddressSpace() in the event the type isn't actually a pointer.

clang/lib/Sema/SemaExpr.cpp

 }
 
 static void CheckForNullPointerDereference(Sema &S, Expr *E) {
   // Check to see if we are dereferencing a null pointer.  If so,
   // and if not volatile-qualified, this is undefined behavior that the
   // optimizer will delete, so warn about it.  People sometimes try to use this
   // to get a deterministic trap and are surprised by clang's behavior.  This
   // only handles the pattern "*null", which is a very syntactic check.
-  if (UnaryOperator *UO = dyn_cast<UnaryOperator>(E->IgnoreParenCasts()))
-    if (UO->getOpcode() == UO_Deref &&
-        UO->getSubExpr()->IgnoreParenCasts()->
-          isNullPointerConstant(S.Context, Expr::NPC_ValueDependentIsNotNull) &&
+  const auto *UO = dyn_cast<UnaryOperator>(E->IgnoreParenCasts());
+  if (UO && UO->getOpcode() == UO_Deref) {
+    const LangAS AS =
+        UO->getSubExpr()->getType()->getPointeeType().getAddressSpace();
+    if ((!isTargetAddressSpace(AS) ||
+         (isTargetAddressSpace(AS) && toTargetAddressSpace(AS) == 0)) &&
+        UO->getSubExpr()->IgnoreParenCasts()->isNullPointerConstant(
+            S.Context, Expr::NPC_ValueDependentIsNotNull) &&
         !UO->getType().isVolatileQualified()) {
       S.DiagRuntimeBehavior(UO->getOperatorLoc(), UO,
                             S.PDiag(diag::warn_indirection_through_null)
                                 << UO->getSubExpr()->getSourceRange());
       S.DiagRuntimeBehavior(UO->getOperatorLoc(), UO,
                             S.PDiag(diag::note_indirection_through_null));
     }
   }
+}
 
 static void DiagnoseDirectIsaAccess(Sema &S, const ObjCIvarRefExpr *OIRE,
                                     SourceLocation AssignLoc,
                                     const Expr* RHS) {
   const ObjCIvarDecl *IV = OIRE->getDecl();
   if (!IV)
     return;
 

clang/test/Sema/exprs.c

void test18(int b) {
   test18_a(); // expected-error {{too few arguments to function call, single argument 'a' was not specified}}
   test18_b(); // expected-error {{too few arguments to function call, expected 1, have 0}}
   test18_c(b); // expected-error {{too few arguments to function call, expected 2, have 1}}
   test18_c(b, b, b); // expected-error {{too many arguments to function call, expected 2, have 3}}
   test18_d(); // expected-error {{too few arguments to function call, at least argument 'a' must be specified}}
   test18_e(); // expected-error {{too few arguments to function call, expected at least 2, have 0}}
 }
 
+typedef int __attribute__((address_space(256))) int_AS256;
 // PR7569
 void test19() {
-  *(int*)0 = 0;   // expected-warning {{indirection of non-volatile null pointer}} \
+  *(int *)0 = 0;                                     // expected-warning {{indirection of non-volatile null pointer}} \
                   // expected-note {{consider using __builtin_trap}}
-  *(volatile int*)0 = 0;  // Ok.
+  *(volatile int *)0 = 0;                            // Ok.
+  *(int __attribute__((address_space(256))) *)0 = 0; // Ok.
+  *(int __attribute__((address_space(0))) *)0 = 0;   // expected-warning {{indirection of non-volatile null pointer}} \
+                     // expected-note {{consider using __builtin_trap}}
+  *(int_AS256 *)0 = 0;                               // Ok.
 
   // rdar://9269271
-  int x = *(int*)0;  // expected-warning {{indirection of non-volatile null pointer}} \
+  int x = *(int *)0;                                                                          // expected-warning {{indirection of non-volatile null pointer}} \
                      // expected-note {{consider using __builtin_trap}}
-  int x2 = *(volatile int*)0; // Ok.
-  int *p = &(*(int*)0); // Ok;
+  int x2 = *(volatile int *)0;                                                                // Ok.
+  int x3 = *(int __attribute__((address_space(0))) *)0;                                       // expected-warning {{indirection of non-volatile null pointer}} \
+                     // expected-note {{consider using __builtin_trap}}
+  int x4 = *(int_AS256 *)0;                                                                   // Ok.
+  int *p = &(*(int *)0);                                                                      // Ok.
+  int_AS256 *p1 = &(*(int __attribute__((address_space(256))) *)0);                           // Ok.
+  int __attribute__((address_space(0))) *p2 = &(*(int __attribute__((address_space(0))) *)0); // Ok.
 }
 
 int test20(int x) {
   return x && 4; // expected-warning {{use of logical '&&' with constant operand}} \
                  // expected-note {{use '&' for a bitwise operation}} \
                  // expected-note {{remove constant to silence this warning}}
 
   return x && sizeof(int) == 4;  // no warning, RHS is logical op.