[PATCH] D60237: [MS] Add metadata for __declspec(allocator)

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/lib/CodeGen/CGAtomic.cpp:1691
   } else {
-    // Build new lvalue for temp address
+    // Build new lvalue for temp address.
     Address Ptr = Atomics.materializeRValue(OldRVal);
----------------
I don't have an issue with these changes if you want to make them, but they should be committed separately.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:4384-4385
+    if (TargetDecl && TargetDecl->hasAttr<MSAllocatorAttr>())
+      CI->setMetadata("heapallocsite", getDebugInfo()->
+                       getMSAllocatorMetadata(RetTy, Loc));
+  }
----------------
I think we should make CGDebugInfo responsible for calling setMetadata. In some sense, "heapallocsite" metadata is debug info, so it makes sense that it would be documented there. Also, if there are other places where we need to add this metadata, we won't have to duplicate this string literal.

So, CGDebugInfo should have some new method `addHeapAllocSiteMetadata(Instruction *CallSite, QualType Ty)`, and that can call the private getOrCreateType method. Sound good?


================
Comment at: clang/test/CodeGen/debug-info-codeview-heapallocsite.c:13
+
+// CHECK: !heapallocsite [[DBG_F1:!.*]]
+// CHECK: [[DBG_F1:!.*]] = !{}
----------------
We should expect this test case to grow, so it would be good to add more context around here. Something like:
  CHECK-LABEL: define{{.*}} @call_alloc
  CHECK: call i8* @myalloc(i64 1) {{.*}} !heapallocsite ...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60237/new/

https://reviews.llvm.org/D60237