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

DavidSpickett added a comment. Can you guide me on how I might correct this? Actually, now I see that MCContext has reportWarning/reportError and just needs a reportNote. I'll have something for you to review shortly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

DavidSpickett added a comment. @ychen Since this change the warning emitted in `AsmPrinterInlineAsm.cpp` about reserved registers no longer has location information for the file that the inline asm is in. Here's an example: void bar(void) { __asm__ __volatile__ ( "nop" : : : "sp");

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5de2d189e6ad: [Diagnose] Unify MCContext and LLVMContext diagnosing (authored by ychen). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

ychen updated this revision to Diff 327304. ychen added a comment. - Remove dead code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97449/new/ https://reviews.llvm.org/D97449 Files: clang/include/clang/Basic/DiagnosticCategories.td

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

MaskRay accepted this revision. MaskRay added inline comments. Comment at: llvm/include/llvm/MC/MCContext.h:67 class SourceMgr; + template class function_ref; With `std::function`, this can be dropped. Repository: rG LLVM Github Monorepo CHANGES

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

ychen added a comment. In D97449#2595415 , @MaskRay wrote: > LG. Can you attach an example triggering clang `InlineAsmDiagHandler2`? I > want to check what the diagnostics look like before and now. `clang/test/CodeGen/asm-errors.c` should trigger it.

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

ychen updated this revision to Diff 327300. ychen added a comment. - Revert to use std::function since MCContext needs to own the handler callback - Fix a typo in BackendConsumer::SrcMgrDiagHandler: DI.getKind() -> DI.getSeverity() Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97449/new/ https://reviews.llvm.org/D97449

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

ychen added inline comments. Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:22 def note_fe_inline_asm_here : Note<"instantiated into assembly here">; +def err_fe_source_mgr : Error<"%0">, CatSourceMgr; +def warn_fe_source_mgr : Warning<"%0">, CatSourceMgr,

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

ychen added inline comments. Comment at: llvm/lib/MC/MCContext.cpp:869 +SMLoc Loc, +std::function GetMessage) { + SourceMgr SM; MaskRay wrote: > ychen wrote: > > MaskRay wrote: > > > Use lightweight function_ref since you don't need to store the

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

ychen updated this revision to Diff 327256. ychen added a comment. - Use function_ref Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97449/new/ https://reviews.llvm.org/D97449 Files: clang/include/clang/Basic/DiagnosticCategories.td

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

MaskRay added a comment. LG. Can you attach an example triggering clang `InlineAsmDiagHandler2`? I want to check what the diagnostic looks like now. Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:22 def note_fe_inline_asm_here : Note<"instantiated into

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

ychen added a comment. In D97449#2595390 , @MaskRay wrote: > Is the https://github.com/ClangBuiltLinux/linux/issues/1269 unreachable error > fixed? > > Reproduce: > > git clone https://android.googlesource.com/kernel/common.git --branch >

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

MaskRay added a comment. Is the https://github.com/ClangBuiltLinux/linux/issues/1269 unreachable error fixed? Reproduce: git clone https://android.googlesource.com/kernel/common.git --branch android-4.19-stable cd common PATH=path/to/your/clang/bin:$PATH make -s -j 30 LLVM=1

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

ychen updated this revision to Diff 327162. ychen added a comment. - Use StringRef::consume_front Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97449/new/ https://reviews.llvm.org/D97449 Files: clang/include/clang/Basic/DiagnosticCategories.td

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

ychen added inline comments. Comment at: llvm/lib/MC/MCContext.cpp:870 +std::function GetMessage) { + SourceMgr SM; + const SourceMgr *SMP = MaskRay wrote: > This looks a bit strange: we need to construct a fresh SourceMgr to print a > diagnostic. I've

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

ychen added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:477 + StringRef Message = D.getMessage(); + if (Message.startswith("error: ")) +Message = Message.substr(7); MaskRay wrote: > `StringRef::consume_front` > > I know you are moving

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

ychen updated this revision to Diff 327155. ychen added a comment. - Add clang Diagnostic Category/Group/Kinds for SourceMgr errors - Add comments in MCContext - Simplify Clang handler - Implement DiagnosticInfoSrcMgr::print - Simplify DiagnosticInfoSrcMgr ctor Repository: rG LLVM Github

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

MaskRay added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:477 + StringRef Message = D.getMessage(); + if (Message.startswith("error: ")) +Message = Message.substr(7); `StringRef::consume_front` I know you are moving code, but do you

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

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

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

nickdesaulniers added a comment. I tested this on some wacko LTO kernel build failure (https://github.com/ClangBuiltLinux/linux/issues/1269). The error message went from: > :0: error: __ia32_compat_sys_sysctl changed binding to STB_GLOBAL To: unimplemented UNREACHABLE executed at

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

ychen updated this revision to Diff 326797. ychen added a comment. - Simplify MCContext changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97449/new/ https://reviews.llvm.org/D97449 Files: clang/lib/CodeGen/CodeGenAction.cpp

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

ychen updated this revision to Diff 326597. ychen added a comment. - Refrain from including SourceMgr.h in MCContext.h Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97449/new/ https://reviews.llvm.org/D97449 Files:

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

ychen added inline comments. Comment at: llvm/include/llvm/MC/MCContext.h:33 #include "llvm/Support/MD5.h" +#include "llvm/Support/SourceMgr.h" #include "llvm/Support/raw_ostream.h" rnk wrote: > MCContext.h is popular, let's try not to leak SourceMgr.h as an

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

ychen added a comment. In 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

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

rnk added a comment. 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. Comment at:

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

ychen created this revision. ychen added reviewers: MaskRay, rnk, tejohnson, qcolombet, anemet. Herald added subscribers: dexonsmith, kerbowa, hiraditya, nhaehnle, jvesely. ychen requested review of this revision. Herald added projects: clang, LLDB, LLVM. Herald added subscribers: llvm-commits,