[clang][ExprConst] Let diagnostics point to std::allocator calls

[tbaederr]

Instead of the underlying operator new calls. This fixes a longstanding FIXME comment in cxx2a-constexpr-dynalloc.cpp.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Instead of the underlying operator new calls. This fixes a longstanding FIXME comment in cxx2a-constexpr-dynalloc.cpp.

---

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

3 Files Affected:

• (modified) clang/lib/AST/ExprConstant.cpp (+3-2)
• (modified) clang/test/AST/ByteCode/new-delete.cpp (+2-2)
• (modified) clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp (+6-5)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 2e680d1569f60f..ef75e31ba1ebdb 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -1132,6 +1132,7 @@ namespace {
     struct StdAllocatorCaller {
       unsigned FrameIndex;
       QualType ElemType;
+      const Expr *Call;
       explicit operator bool() const { return FrameIndex != 0; };
     };
 
@@ -1155,7 +1156,7 @@ namespace {
         if (CTSD->isInStdNamespace() && ClassII &&
             ClassII->isStr("allocator") && TAL.size() >= 1 &&
             TAL[0].getKind() == TemplateArgument::Type)
-          return {Call->Index, TAL[0].getAsType()};
+          return {Call->Index, TAL[0].getAsType(), Call->CallExpr};
       }
 
       return {};
@@ -7033,7 +7034,7 @@ static bool HandleOperatorNewCall(EvalInfo &Info, const CallExpr *E,
 
   QualType AllocType = Info.Ctx.getConstantArrayType(
       ElemType, Size, nullptr, ArraySizeModifier::Normal, 0);
-  APValue *Val = Info.createHeapAlloc(E, AllocType, Result);
+  APValue *Val = Info.createHeapAlloc(Caller.Call, AllocType, Result);
   *Val = APValue(APValue::UninitArray(), 0, Size.getZExtValue());
   Result.addArray(Info, E, cast<ConstantArrayType>(AllocType));
   return true;
diff --git a/clang/test/AST/ByteCode/new-delete.cpp b/clang/test/AST/ByteCode/new-delete.cpp
index 8466e9b88782f2..87aa220d6f6cfe 100644
--- a/clang/test/AST/ByteCode/new-delete.cpp
+++ b/clang/test/AST/ByteCode/new-delete.cpp
@@ -602,7 +602,7 @@ namespace std {
   using size_t = decltype(sizeof(0));
   template<typename T> struct allocator {
     constexpr T *allocate(size_t N) {
-      return (T*)__builtin_operator_new(sizeof(T) * N); // both-note 2{{allocation performed here}} \
+      return (T*)__builtin_operator_new(sizeof(T) * N); // expected-note 2{{allocation performed here}} \
                                                         // #alloc
     }
     constexpr void deallocate(void *p) {
@@ -641,7 +641,7 @@ namespace OperatorNewDelete {
       p = new int[1]; // both-note {{heap allocation performed here}}
       break;
     case 2:
-      p = std::allocator<int>().allocate(1);
+      p = std::allocator<int>().allocate(1); // ref-note 2{{heap allocation performed here}}
       break;
     }
     switch (dealloc_kind) {
diff --git a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
index 6d9c0b607d8a67..ed8cbbbfe70678 100644
--- a/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
+++ b/clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp
@@ -12,10 +12,9 @@ static_assert(alloc_from_user_code()); // expected-error {{constant expression}}
 
 namespace std {
   using size_t = decltype(sizeof(0));
-  // FIXME: It would be preferable to point these notes at the location of the call to allocator<...>::[de]allocate instead
   template<typename T> struct allocator {
     constexpr T *allocate(size_t N) {
-      return (T*)NEW(sizeof(T) * N); // expected-note 3{{heap allocation}} expected-note {{not deallocated}}
+      return (T*)NEW(sizeof(T) * N);
     }
     constexpr void deallocate(void *p) {
       DELETE(p); // #dealloc expected-note 2{{'std::allocator<...>::deallocate' used to delete pointer to object allocated with 'new'}}
@@ -59,7 +58,7 @@ constexpr bool mismatched(int alloc_kind, int dealloc_kind) {
     p = new int[1]; // expected-note {{heap allocation}}
     break;
   case 2:
-    p = std::allocator<int>().allocate(1);
+    p = std::allocator<int>().allocate(1); // expected-note 2{{heap allocation}}
     break;
   }
   switch (dealloc_kind) {
@@ -81,8 +80,10 @@ static_assert(mismatched(2, 0)); // expected-error {{constant expression}} expec
 static_assert(mismatched(2, 1)); // expected-error {{constant expression}} expected-note {{in call}}
 static_assert(mismatched(2, 2));
 
-constexpr int *escape = std::allocator<int>().allocate(3); // expected-error {{constant expression}} expected-note {{pointer to subobject of heap-allocated}}
-constexpr int leak = (std::allocator<int>().allocate(3), 0); // expected-error {{constant expression}}
+constexpr int *escape = std::allocator<int>().allocate(3); // expected-error {{constant expression}} expected-note {{pointer to subobject of heap-allocated}} \
+                                                           // expected-note {{heap allocation performed here}}
+constexpr int leak = (std::allocator<int>().allocate(3), 0); // expected-error {{constant expression}} \
+                                                             // expected-note {{not deallocated}}
 constexpr int no_lifetime_start = (*std::allocator<int>().allocate(1) = 1); // expected-error {{constant expression}} expected-note {{assignment to object outside its lifetime}}
 constexpr int no_deallocate_nullptr = (std::allocator<int>().deallocate(nullptr), 1); // expected-error {{constant expression}} expected-note {{in call}}
 // expected-note@#dealloc {{'std::allocator<...>::deallocate' used to delete a null pointer}}

[shafik]

Thanks, this would be an improvement but I have one concern with this change.

[shafik]

So Call will be nullptr here and following some of the code from createHeapAlloc I am not sure that is safe. Maybe the combination can't happen but it is not obvious to me.

[tbaederr]

All callers of getStdAllocatorCaller() check that the returned value converts to true, and Call can't benullptr if that's the case.