[clang][ExprConst] Reject field access with nullptr base

[tbaederr]

Reject them if the base is null, not only if the entire pointer is null.

Fixes #113821

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Reject them if the base is null, not only if the entire pointer is null.

---

Full diff: https://github.com/llvm/llvm-project/pull/113885.diff

2 Files Affected:

• (modified) clang/lib/AST/ExprConstant.cpp (+1-1)
• (modified) clang/test/CXX/expr/expr.const/p2-0x.cpp (+2-1)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 8e36cad2d2c6e7..a6694bc0641508 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -1703,7 +1703,7 @@ namespace {
     bool checkNullPointerDiagnosingWith(const GenDiagType &GenDiag) {
       if (Designator.Invalid)
         return false;
-      if (IsNullPtr) {
+      if (getLValueBase().isNull()) {
         GenDiag();
         Designator.setInvalid();
         return false;
diff --git a/clang/test/CXX/expr/expr.const/p2-0x.cpp b/clang/test/CXX/expr/expr.const/p2-0x.cpp
index 767eee1c74f054..67160ba571f33c 100644
--- a/clang/test/CXX/expr/expr.const/p2-0x.cpp
+++ b/clang/test/CXX/expr/expr.const/p2-0x.cpp
@@ -188,7 +188,7 @@ namespace UndefinedBehavior {
 
   namespace Ptr {
     struct A {};
-    struct B : A { int n; };
+    struct B : A { int n; int m; };
     B a[3][3];
     constexpr B *p = a[0] + 4; // expected-error {{constant expression}} expected-note {{element 4 of array of 3 elements}}
     B b = {};
@@ -204,6 +204,7 @@ namespace UndefinedBehavior {
     static_assert((A*)nb == 0, "");
     static_assert((B*)na == 0, "");
     constexpr const int &nf = nb->n; // expected-error {{constant expression}} expected-note {{cannot access field of null pointer}}
+    constexpr const int &mf = nb->m; // expected-error {{constant expression}} expected-note {{cannot access field of null pointer}}
     constexpr const int *np1 = (int*)nullptr + 0; // ok
     constexpr const int *np2 = &(*(int(*)[4])nullptr)[0]; // ok
     constexpr const int *np3 = &(*(int(*)[4])nullptr)[2]; // expected-error {{constant expression}} expected-note {{cannot perform pointer arithmetic on null pointer}}

[zygoloid]

Why was the old check not good enough here? In the added testcase, nb is a null pointer.

[zygoloid]

Oh, I see. We're adjusting the offset before we call addDecl. I think this would be better fixed by reversing the order in which we do those steps -- first addDecl and then adjust the offset. A null base isn't necessarily a null pointer, so checking for a null base here seems wrong (but only in the weird case of a zero integer cast to a non-null pointer value, which we should only accept in constant-folding mode, not when properly evaluating).

[tbaederr]

The offset is added in adjustOffset(), which calls clearIsNullPointer(). This is called before we call addDecl(), so at that point the pointer is already considered non-null.

[tbaederr]

Looks like the check here was !Base until 402804b

[tbaederr]

Ping

[AaronBallman]

LGTM but the changes should come with a release note.

[tbaederr]

@zygoloid Do you approve too?

[tbaederr]

Ping

[zygoloid]

As mentioned in the comment thread, I'd prefer to see this addressed by reversing the order in which we call addDecl versus add an offset. While it's a corner case, I think this change would cause us to start rejecting things like this example where we form a non-null pointer with no base in a constant-folded region. Currently, the subsequent member access (->hardware_register_1) is accepted, because it's not accessing a member of a null pointer -- it's accessing a member of an integral non-null pointer.

I don't know how important things like this example are, but they do at least seem useful in some cases.

[tbaederr]

I think I understand what you mean by changing the order now. I changed two other places as well, just to be consistent.

[zygoloid]

LG, thank you!