This is an archive of the discontinued LLVM Phabricator instance.

[Diagnose] Unify MCContext and LLVMContext diagnosing
ClosedPublic

Authored by ychen on Feb 24 2021, 11:45 PM.

Details

Summary

The situation with inline asm/MC error reporting is kind of messy at the
moment. The errors from MC layout are not reliably propagated(D96931) and users
have to specify an inlineasm handler separately to get inlineasm
diagnose. The latter issue is not a correctness issue but could be improved.

  • Kill LLVMContext inlineasm diagnose handler and migrate it to use DiagnoseInfo/DiagnoseHandler.
  • Introduce DiagnoseInfoSrcMgr to diagnose SourceMgr backed errors. This covers use cases like inlineasm, MC, and any clients using SourceMgr.
  • Move AsmPrinter::SrcMgrDiagInfo and its instance to MCContext. The next step is to combine MCContext::SrcMgr and MCContext::InlineSrcMgr because in all use cases, only one of them is used.
  • If LLVMContext is available, let MCContext uses LLVMContext's diagnose handler; if LLVMContext is not available, MCContext uses its own default diagnose handler which just prints SMDiagnostic.
  • Change a few clients(Clang, llc, lldb) to use the new way of reporting.

Diff Detail

Event Timeline

ychen created this revision.Feb 24 2021, 11:45 PM
ychen requested review of this revision.Feb 24 2021, 11:45 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptFeb 24 2021, 11:45 PM
ychen edited the summary of this revision. (Show Details)Feb 24 2021, 11:48 PM
rnk added a comment.Feb 25 2021, 2:43 PM

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.

llvm/include/llvm/MC/MCContext.h
33

MCContext.h is popular, let's try not to leak SourceMgr.h as an include. It probably brings in tons of FS headers that most files don't need.

76

It doesn't feel right to use MDNode, an IR type, from MC. Is there a way to make this opaque?

381

This would need to be out of line to get by with a forward decl of SourceMgr.

ychen added a comment.Feb 25 2021, 3:53 PM
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 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.)
llvm/include/llvm/MC/MCContext.h
33

will do

76

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.

ychen updated this revision to Diff 326597.Feb 25 2021, 9:45 PM
  • Refrain from including SourceMgr.h in MCContext.h
ychen updated this revision to Diff 326797.Feb 26 2021, 1:59 PM
  • Simplify MCContext changes.

I tested this on some wacko LTO kernel build failure (https://github.com/ClangBuiltLinux/linux/issues/1269). The error message went from:

<unknown>:0: error: __ia32_compat_sys_sysctl changed binding to STB_GLOBAL

To:

unimplemented
UNREACHABLE executed at ../include/llvm/IR/DiagnosticInfo.h:1036!
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: ld.lld -m elf_x86_64 -z max-page-size=0x200000 -plugin-opt=-code-model=kernel -plugin-opt=-stack-alignment=8 -mllvm -import-instr-limit=5 -r -o vmlinux.o -T .tmp_lto.lds --whole-archive built-in.a --no-whole-archive --start-group lib/lib.a arch/x86/lib/lib.a --end-group
1.      Running pass 'Function Pass Manager' on module 'ld-temp.o'.
2.      Running pass 'X86 Assembly Printer' on function '@__ia32_compat_sys_sysctl'
 #0 0x0000000001bb2d13 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/android0/llvm-project/llvm/build/bin/lld+0x1bb2d13)
 #1 0x0000000001bb0a5e llvm::sys::RunSignalHandlers() (/android0/llvm-project/llvm/build/bin/lld+0x1bb0a5e)
 #2 0x0000000001bb330f SignalHandler(int) Signals.cpp:0:0
 #3 0x00007f5d1768b140 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14140)
 #4 0x00007f5d16fb5ce1 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #5 0x00007f5d16f9f537 abort ./stdlib/abort.c:81:7
 #6 0x0000000001b2a261 (/android0/llvm-project/llvm/build/bin/lld+0x1b2a261)
 #7 0x0000000002cd3199 (/android0/llvm-project/llvm/build/bin/lld+0x2cd3199)
 #8 0x0000000001bb4479 lld::diagnosticHandler(llvm::DiagnosticInfo const&) (/android0/llvm-project/llvm/build/bin/lld+0x1bb4479)
 #9 0x0000000002bb03bf llvm::lto::LTOLLVMDiagnosticHandler::handleDiagnostics(llvm::DiagnosticInfo const&) LTO.cpp:0:0
#10 0x000000000407f96c llvm::LLVMContext::diagnose(llvm::DiagnosticInfo const&) (/android0/llvm-project/llvm/build/bin/lld+0x407f96c)
#11 0x0000000002cd30b4 std::_Function_handler<void (llvm::SMDiagnostic const&, bool, llvm::SourceMgr const&, std::vector<llvm::MDNode const*, std::allocator<llvm::MDNode const*> >&), llvm::MachineModuleInfoWrapperPass::doInitialization(llvm::Module&)::$_1>::_M_invoke(std::_Any_data const&, llvm::SMDiagnostic const&, bool&&, llvm::SourceMgr const&, std::vector<llvm::MDNode const*, std::allocator<llvm::MDNode const*> >&) MachineModuleInfo.cpp:0:0
#12 0x0000000003e5e87a llvm::MCContext::reportCommon(llvm::SMLoc, std::function<void (llvm::SMDiagnostic&, llvm::SourceMgr const*)>) (/android0/llvm-project/llvm/build/bin/lld+0x3e5e87a)
#13 0x0000000003e5aeee llvm::MCContext::reportError(llvm::SMLoc, llvm::Twine const&) (/android0/llvm-project/llvm/build/bin/lld+0x3e5aeee)
#14 0x0000000003e6c7de llvm::MCELFStreamer::emitSymbolAttribute(llvm::MCSymbol*, llvm::MCSymbolAttr) (/android0/llvm-project/llvm/build/bin/lld+0x3e6c7de)
#15 0x000000000271b6f2 llvm::AsmPrinter::emitFunctionHeader() (/android0/llvm-project/llvm/build/bin/lld+0x271b6f2)
#16 0x000000000271cb3d llvm::AsmPrinter::emitFunctionBody() (/android0/llvm-project/llvm/build/bin/lld+0x271cb3d)
#17 0x0000000002482556 llvm::X86AsmPrinter::runOnMachineFunction(llvm::MachineFunction&) X86AsmPrinter.cpp:0:0
#18 0x0000000002cb37fe llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/android0/llvm-project/llvm/build/bin/lld+0x2cb37fe)
#19 0x00000000040912a8 llvm::FPPassManager::runOnFunction(llvm::Function&) (/android0/llvm-project/llvm/build/bin/lld+0x40912a8)
#20 0x00000000040979a8 llvm::FPPassManager::runOnModule(llvm::Module&) (/android0/llvm-project/llvm/build/bin/lld+0x40979a8)
#21 0x0000000004091957 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/android0/llvm-project/llvm/build/bin/lld+0x4091957)
llvm/test/CodeGen/AMDGPU/lds-initializer.ll
1–3

Does the addition of not mean that this test would have failed due to these changes? How come?

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.

llvm/include/llvm/IR/DiagnosticInfo.h
1037

@nickdesaulniers's diagnostic means this is reachable.

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

llvm/lib/MC/MCContext.cpp
869

Use lightweight function_ref since you don't need to store the callback.

870

This looks a bit strange: we need to construct a fresh SourceMgr to print a diagnostic.

llvm/test/CodeGen/AMDGPU/lds-initializer.ll
1–3

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.

llvm/test/CodeGen/AMDGPU/lds-zero-initializer.ll
1–2

While here, -o /dev/null -> -filetype=null

4

Add error: to make it clear this is an error.

MaskRay added inline comments.Feb 26 2021, 7:29 PM
clang/lib/CodeGen/CodeGenAction.cpp
460

StringRef::consume_front

I know you are moving code, but do you know why it needs to chop off the error: prefix (why does the message get a prefix here?)

ychen updated this revision to Diff 327155.Mar 1 2021, 9:44 AM
  • Add clang Diagnostic Category/Group/Kinds for SourceMgr errors
  • Add comments in MCContext
  • Simplify Clang handler
  • Implement DiagnosticInfoSrcMgr::print
  • Simplify DiagnosticInfoSrcMgr ctor
clang/lib/CodeGen/CodeGenAction.cpp
460

It was introduced by https://github.com/llvm/llvm-project/commit/5ec32e7fd845e0b7db33689f33cc2ef7c83710fa.

I guess it is to canonicalize error messages in case the user just throws in an "error: " prefix like Ctx.diagnose("error: xxx") which would give two error prefixes otherwise.

llvm/lib/MC/MCContext.cpp
869

I was hesitant to do this because it requires including STLExtras.h in MCContext.h which could be bad for compile-time.

ychen added inline comments.Mar 1 2021, 9:46 AM
llvm/lib/MC/MCContext.cpp
870

I've included comments to explain that.

ychen updated this revision to Diff 327162.Mar 1 2021, 10:00 AM
  • Use StringRef::consume_front
MaskRay added a comment.EditedMar 1 2021, 12:50 PM

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
# ninja -C path/yo/your/build clang lld check-llvm-tools
PATH=path/to/your/build/bin:$PATH make -s -j 30 LLVM=1 O=/tmp/out/android gki_defconfig bzImage 
# very slow
llvm/lib/MC/MCContext.cpp
869

You can forward declare function_ref (see mlir/include/mlir/Support/LLVM.h).

ychen added a comment.Mar 1 2021, 12:58 PM

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
# ninja -C path/yo/your/build clang lld check-llvm-tools
PATH=path/to/your/build/bin:$PATH make -s -j 30 LLVM=1 O=/tmp/out/android gki_defconfig bzImage 
# very slow

I think so. DiagnosticInfoSrcMgr::print is now implemented.

MaskRay added a comment.EditedMar 1 2021, 1:06 PM

LG. Can you attach an example triggering clang InlineAsmDiagHandler2? I want to check what the diagnostics look like before and now.

clang/include/clang/Basic/DiagnosticFrontendKinds.td
22

These seem unused.

clang/include/clang/Basic/DiagnosticGroups.td
1148

Unused

ychen updated this revision to Diff 327256.Mar 1 2021, 1:07 PM
  • Use function_ref
ychen added inline comments.Mar 1 2021, 1:08 PM
llvm/lib/MC/MCContext.cpp
869

Thanks. Done.

ychen added inline comments.Mar 1 2021, 1:15 PM
clang/include/clang/Basic/DiagnosticFrontendKinds.td
22

These .td file changes are used by ComputeDiagID(DI.getKind(), source_mgr, DiagID); down below. Basically it makes non-inlineasm SourceMgr error its own diagnose category.

MaskRay accepted this revision.Mar 1 2021, 1:19 PM

LGTM.

This revision is now accepted and ready to land.Mar 1 2021, 1:19 PM
ychen updated this revision to Diff 327300.Mar 1 2021, 3:06 PM
  • Revert to use std::function since MCContext needs to own the handler callback
  • Fix a typo in BackendConsumer::SrcMgrDiagHandler: DI.getKind() -> DI.getSeverity()
ychen added a comment.Mar 1 2021, 3:06 PM

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.

MaskRay accepted this revision.Mar 1 2021, 3:12 PM
MaskRay added inline comments.
llvm/include/llvm/MC/MCContext.h
67

With std::function, this can be dropped.

ychen updated this revision to Diff 327304.Mar 1 2021, 3:19 PM
  • Remove dead code
This revision was landed with ongoing or failed builds.Mar 1 2021, 3:59 PM
This revision was automatically updated to reflect the committed changes.

@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");
}

./bin/clang --target=arm-arm-none-eabi -march=armv7-m -c /tmp/test.c -o /dev/null

Before:

/tmp/test.c:3:28: warning: inline asm clobber list contains reserved registers: SP [-Winline-asm]
    __asm__ __volatile__ ( "nop" : : : "sp");
                           ^
<inline asm>:1:1: note: instantiated into assembly here
        nop
^
/tmp/test.c:3:28: note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
    __asm__ __volatile__ ( "nop" : : : "sp");
                           ^
<inline asm>:1:1: note: instantiated into assembly here
        nop
^
1 warning generated.

After:

<inline asm>:1:1: warning: inline asm clobber list contains reserved registers: SP
        nop
^^^^^^^^
<inline asm>:1:1: note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
        nop
^^^^^^^^

(the reason it didn't break any tests is that we only check this message from IR and only look for the "contains reserved registers" line)

Can you guide me on how I might correct this?

My current impression is that since this code does SrcMgr.PrintMessage(Loc, SourceMgr::DK_Warning, Msg) it's one level too deep to get source information added to it automatically. And that if this code could use DK_InlineAsm that would help. However getting hold of the Diagnostic manager from MCContext isn't possible currently, perhaps that is by design?

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.