[PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D97449#2588804 <https://reviews.llvm.org/D97449#2588804>, @rnk wrote:

> This still feels a bit messy and I don't totally understand how it all fits together, but I'm super supportive of getting rid of the inline asm diagnostic handler and standardizing on DiagnosticHandler/DiagnosticInfo.

Fully agree. It took me a while to collect (hopefully) all the pieces. :-)

I think part of the complexity stems from the factor that frontend clients like Clang owns the SourceMgr equivalent and handles the source code mapping part (LLVMContext doesn't control source managers), but LLVM internally (MC/MCContext) uses SourceMgr for scenarios like reporting assembly related errors (inlineasem, or input asm file). So in AsmParser/AsmPrinter/AsmPrinterInlineAsm, there is code about SourceMgr and its handlers here and there.

There are two improvements that could be done for the next step:

- Move SourceMgr related processing in AsmParser/AsmPrinter/AsmPrinterInlineAsm to MCContext.
- Merge MCContext::SrcMgr and MCContext::InlineSrcMgr because in all use cases, only one of them is used.

Both make MCContext the main class for MC reporting and simplify things.

It may help to go through this patch in this order:

- DiagnosticInfo.h (introduces new diag info kind)
- AsmPrinterInlineAsm.cpp, MachineModuleInfo.cpp ([1] make MCContext own the information for inlineasm reporting [2]hook LLVMContext's handler to MCContext's reporting methods)
- LLVMContext.cpp/LLVMContextImpl.cpp (kill LLVMContext inlineasm handler)
- Clang/lldb/llc (switch clients to use DiagnosticInfoSrcMgr)
- AsmParser.cpp (this is a little bit tricky but it just switches to use the handler passed from LLVMContext to MCContext.)



================
Comment at: llvm/include/llvm/MC/MCContext.h:76
+        std::function<void(const SMDiagnostic &, bool, const SourceMgr &,
+                           std::vector<const MDNode *> &)>;
 
----------------
rnk wrote:
> It doesn't feel right to use MDNode, an IR type, from MC. Is there a way to make this opaque?
Yeah, I've paid attention to that. Here MCContext owns the std::vector but doesn't need to do any processing with it. Forward declaring `MDNode` is all it needs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97449