[PATCH] D61083: Recommitting r358783 and r358786 "[MS] Emit S_HEAPALLOCSITE debug info" with fixes for buildbot error (undefined assembler label).

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1078
+      MCSymbol *EndLabel = std::get<1>(HeapAllocSite);
+      if (BeginLabel->isDefined() && EndLabel->isDefined()) {
+        DIType *DITy = std::get<2>(HeapAllocSite);
----------------
I think it's worth a comment explaining why some of these labels might not be defined. Basically, if the instruction gets replaced anywhere in the codegen pipeline, they won't be defined.
Also, it would be LLVM-y to invert the condition and use `continue` to reduce indentation:
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code


================
Comment at: llvm/lib/Target/X86/X86FastISel.cpp:3988
   Result->addMemOperand(*FuncInfo.MF, createMachineMemOperandFor(LI));
+  Result->cloneInstrSymbols(*FuncInfo.MF, *MI);
   MachineBasicBlock::iterator I(MI);
----------------
Please add a new test case that fails if this line of code is removed. I think you can start from this C++ code:
```
struct Foo {
  __declspec(allocator) virtual void *alloc();
};
void use_alloc(void*);
void do_alloc(Foo *p) {
  use_alloc(p->alloc());
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61083