[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

486–489

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

486–489

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

190

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

489

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) &&
+        !isTargetAddressSpace(
+            UO->getSubExpr()->getType()->getPointeeType().getAddressSpace()) &&
+        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}} \
                   // expected-note {{consider using __builtin_trap}}
   *(volatile int*)0 = 0;  // Ok.
+   *(int __attribute__((address_space(256))) *)0 = 0; // Ok.
+   *(int_AS256*)0 = 0; // Ok.
 
   // rdar://9269271
   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 test20(int x) {