[clang][ExprConst] Consider integer pointers of value 0 nullptr

[tbaederr]

When casting a 0 to a pointer type, the IsNullPtr flag was always set to false, leading to weird results like a pointer with value 0 that isn't a null pointer.

This caused

struct B { const int *p;};
template<B> void f() {}
template void f<B{nullptr}>();
template void f<B{fold(reinterpret_cast<int*>(0))}>();

to be valid code, since nullptr and (int*)0 aren't equal. This seems weird and GCC doesn't behave like this.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

When casting a 0 to a pointer type, the IsNullPtr flag was always set to false, leading to weird results like a pointer with value 0 that isn't a null pointer.

This caused

struct B { const int *p;};
template<B> void f() {}
template void f<B{nullptr}>();
template void f<B{fold(reinterpret_cast<int*>(0))}>();

to be valid code, since nullptr and (int*)0 aren't equal. This seems weird and GCC doesn't behave like this.

---

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

2 Files Affected:

• (modified) clang/lib/AST/ExprConstant.cpp (+2-1)
• (modified) clang/test/CodeGenCXX/mangle-class-nttp.cpp (-6)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 0d12161756467..5fe2bd59fcc98 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -9834,7 +9834,8 @@ bool PointerExprEvaluator::VisitCastExpr(const CastExpr *E) {
       Result.InvalidBase = false;
       Result.Offset = CharUnits::fromQuantity(N);
       Result.Designator.setInvalid();
-      Result.IsNullPtr = false;
+      Result.IsNullPtr =
+          (N == Info.Ctx.getTargetNullPointerValue(E->getType()));
       return true;
     } else {
       // In rare instances, the value isn't an lvalue.
diff --git a/clang/test/CodeGenCXX/mangle-class-nttp.cpp b/clang/test/CodeGenCXX/mangle-class-nttp.cpp
index 12c81f2ba0514..c250be7c73c72 100644
--- a/clang/test/CodeGenCXX/mangle-class-nttp.cpp
+++ b/clang/test/CodeGenCXX/mangle-class-nttp.cpp
@@ -27,12 +27,6 @@ template void f<B{nullptr}>();
 // CHECK: define weak_odr void @_Z1fIXtl1BLPKi32EEEEvv(
 // MSABI: define {{.*}} @"??$f@$2UB@@PEBH0CA@H0A@@@@YAXXZ"
 template void f<B{fold((int*)32)}>();
-#ifndef _WIN32
-// FIXME: On MS ABI, we mangle this the same as nullptr, despite considering a
-// null pointer and zero bitcast to a pointer to be distinct pointer values.
-// CHECK: define weak_odr void @_Z1fIXtl1BrcPKiLi0EEEEvv(
-template void f<B{fold(reinterpret_cast<int*>(0))}>();
-#endif
 
 // Pointers to subobjects.
 struct Nested { union { int k; int arr[2]; }; } nested[2];

[AaronBallman]

Should we be retaining this test and getting a different mangling?

[zygoloid]

We should ensure we still have a test for mangling in the case where the null pointer representation is not zero. I don't know if there are any MS ABI targets where that happens, and I would guess there aren't, but we should retain an Itanium ABI test at least.

[tbaederr]

Not sure I understand, with this patch, template void f<B{fold(reinterpret_cast<int*>(0))}>(); is equivalent to the template void f<B{nullptr}>(); from line 25, so this test causes a second explicit template instantiation.

[zygoloid]

It should not, in general. Whether reinterpret_cast maps a zero integer to a null pointer is target dependent, although you will presumably need to find a target where they differ to test this.

[tbaederr]

Well it is the same with this patch, at least during constant evaluation. Are you saying we need to handle integral pointers differently during mangling so this test stays and still passes?

[efriedma-quic]

This specific test is fine, I think: if we know the value is in fact equivalent to a null pointer, there's no reason to treat it differently.

But I'd like to also see a test for a pointer where null is not zero.

[tbaederr]

The fact that the two casts behave differently seems weird, but the second one is a NullToPointer cast.

[zygoloid]

Yeah, it's weird, but that's just how C++ is: converting a null pointer constant to pointer type is a completely different operation from reinterpreting an integer value (that happens to be 0) as a pointer value.

Similar weirdness would happen in C for this target:

private int *p = (private int*)0; // null pointer.
intptr_t n = 0;
private int *q = (private int*)n; // not a null pointer.
intptr_t m = -1;
private int *r = (private int*)m; // null pointer.

[efriedma-quic]

LGTM

[zygoloid]

Can we also retain a test that we mangle a zero-but-not-null pointer properly?

[tbaederr]

@zygoloid Are the new tests what you expected?

[zygoloid]

@zygoloid Are the new tests what you expected?

Almost -- what I'd like is a test for -triple amdgcn showing we still use the rc mangling for the zero but not null case on that target (and a test that we use the L0 mangling for the case of -1 cast to pointer type would be nice too).