[PATCH] D29441: [Assembler] Enable nicer diagnostics for inline assembly.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: include/llvm/CodeGen/AsmPrinter.h:114
+  /// A SourceMgr for inline assembly.
+  mutable std::unique_ptr<SourceMgr> InlineSrcMgr;
+
----------------
rengolin wrote:
> I'd move this line down, as it's being handled by the new methods there.
Most classes seem to group fields together separately from methods, so I think this is fine.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp:105
   if (LLVMCtx.getInlineAsmDiagnosticHandler() != nullptr) {
+    SrcMgrDiagInfo &DiagInfo = getNewDiagInfo();
     // If the source manager has an issue, we arrange for srcMgrDiagHandler
----------------
Why do we need a collection of new DiagInfos for every inline asm buffer? It seems like we can have just one in place of the vector on AsmPrinter, and then let the assignments below rewrite it for every inline asm blob we process. In fact, now we don't need to set the source manager diagnostic handler every time we process an inline asm blob. We could do it when we create the source manager instead.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp:116-117
   std::unique_ptr<MemoryBuffer> Buffer;
-  if (isNullTerminated)
-    Buffer = MemoryBuffer::getMemBuffer(Str, "<inline asm>");
-  else
-    Buffer = MemoryBuffer::getMemBufferCopy(Str, "<inline asm>");
+  // Always copy the inline assembly string into the memory buffer to preserve
+  // it until after finalization.
+  Buffer = MemoryBuffer::getMemBufferCopy(Str, "<inline asm>");
----------------
I'd reword this comment along these lines to help explain the lifetime issues:
  The inline asm source manager will outlive Str, so make a copy of the string for SourceMgr to own.


https://reviews.llvm.org/D29441