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

[Sanne Wouda via Phabricator via llvm-commits]

sanwou01 added a comment.

In https://reviews.llvm.org/D29441#664789, @anemet wrote:

> Probably a completely silly question but why don't we use LLVMContext::InlineAsmDiagHandler for this?


The diagnostics callback will pass through the DiagHandler saved in LLVMContext already. It's the one saved in DiagInfo, and called through the local DiagHandler.  This patch adds plumbing for diagnostics to work //after// parsing, from the backend.



================
Comment at: include/llvm/CodeGen/AsmPrinter.h:114
+  /// A SourceMgr for inline assembly.
+  mutable std::unique_ptr<SourceMgr> InlineSrcMgr;
+
----------------
rengolin wrote:
> rnk wrote:
> > 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.
> Ok for me, then maybe bring `DiagInfos` up here and add a comment?
I'll move DiagInfos (or singular, see below) up here, thanks.


================
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
----------------
rnk wrote:
> 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.
My concern is with LocMDNode, which is used to generate a LocCookie in the diagHandler if it's non-null.  This could presumably be different for different calls to EmitInlineAsm, and we would overwrite that.

That said, there are no guarantees that the LocMDNode will still be alive during finalization.  Having one DiagInfo, like you suggested, and resetting LocMDNode to a nullptr after parsing seems to me like the way to go.  Agreed?


================
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>");
----------------
rnk wrote:
> 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.
Will do, thanks.


https://reviews.llvm.org/D29441