[Lldb-commits] [PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I am supportive of getting rid of InlineAsmDiagnosticHandler, too.

The updated AMDGPU tests suggeste that previously `MCContext::reportError` may be called with no `SrcMgr` or `InlineSrcMgr`, so the error is propagated to the temporary `SourceMgr()`. The `LLCDiagnosticHandler` does not receive anything so the exit code is incorrect 0.

The new behavior moves the `SrcMgr` or `InlineSrcMgr` check under `if (Loc.isValid()) {` (in `MCContext::reportCommon`). There is still a temporary `SrcMgr`. I wonder whether this can be improved.



================
Comment at: llvm/include/llvm/IR/DiagnosticInfo.h:1036
+  void print(DiagnosticPrinter &DP) const override {
+    llvm_unreachable("unimplemented");
+  }
----------------
@nickdesaulniers's diagnostic means this is reachable.

Perhaps `Diagnostic.print` needs to be called with appropriate arguments.


================
Comment at: llvm/lib/MC/MCContext.cpp:869
+    SMLoc Loc,
+    std::function<void(SMDiagnostic &, const SourceMgr *)> GetMessage) {
+  SourceMgr SM;
----------------
Use lightweight function_ref since you don't need to store the callback.


================
Comment at: llvm/lib/MC/MCContext.cpp:870
+    std::function<void(SMDiagnostic &, const SourceMgr *)> GetMessage) {
+  SourceMgr SM;
+  const SourceMgr *SMP = &SM;
----------------
This looks a bit strange: we need to construct a fresh SourceMgr to print a diagnostic.


================
Comment at: llvm/test/CodeGen/AMDGPU/lds-initializer.ll:1-2
-; RUN: llc -march=amdgcn -mcpu=tahiti < %s -o /dev/null 2>&1 | FileCheck %s
-; RUN: llc -march=amdgcn -mcpu=tonga < %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -march=amdgcn -mcpu=tahiti < %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -march=amdgcn -mcpu=tonga < %s -o /dev/null 2>&1 | FileCheck %s
 
----------------
nickdesaulniers wrote:
> Does the addition of `not` mean that this test would have failed due to these changes? How come?
This is an improvement. Errors should return non-zero. It might be clear to change the CHECK below to have `error:`.

In the new code, `LLCDiagnosticHandler` sets `HasError` to true.


================
Comment at: llvm/test/CodeGen/AMDGPU/lds-zero-initializer.ll:1
-; RUN: llc -march=amdgcn -mcpu=tahiti < %s -o /dev/null 2>&1 | FileCheck %s
-; RUN: llc -march=amdgcn -mcpu=tonga < %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -march=amdgcn -mcpu=tahiti < %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -march=amdgcn -mcpu=tonga < %s -o /dev/null 2>&1 | FileCheck %s
----------------
While here, `-o /dev/null` -> `-filetype=null`


================
Comment at: llvm/test/CodeGen/AMDGPU/lds-zero-initializer.ll:4
 
 ; CHECK: lds: unsupported initializer for address space
 
----------------
Add `error:` to make it clear this is an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97449