HomePhabricator

[MCParser] Bring back srcmanager diagnostics in AsmParser

Authored by bkramer on Mar 2 2021, 4:41 AM.

Description

[MCParser] Bring back srcmanager diagnostics in AsmParser

AsmParser may have no LLVMContext attached to it, which means after
5de2d189e6ad466a1f0616195e8c524a4eb3cbc0 everything goes to stderr.
Restore the old behavior.

Details

Committed
bkramerMar 2 2021, 4:43 AM
Parents
rGcaa5144d569c: [mlir] Use mlir::OpState::operator->() to get to Operation::getAttrs().
Branches
Unknown
Tags
Unknown

Event Timeline

ychen added a subscriber: ychen.Mar 2 2021, 3:56 PM

Just to confirm, there are no existing clients of the restored Parser->SavedDiagHandler calls after 5de2d189e6ad466a1, right? The idea was that MCContext::setDiagnosticHandler should be used for diagnosing MC layer with or without LLVMContext.

/llvm/lib/MC/MCParser/AsmParser.cpp
2396

NewDiag?

Just to confirm, there are no existing clients of the restored Parser->SavedDiagHandler calls after 5de2d189e6ad466a1, right? The idea was that MCContext::setDiagnosticHandler should be used for diagnosing MC layer with or without LLVMContext.

I have an internal user that sets up a diag handler in SourceMgr directly. After your change it died with the really annoying failure mode of spamming stderr.

I guess they could migrate to setting the diag handler on MCContext instead of SrcMgr though, if that's the preferred way. It's just confusing to have multiple ways of setting up a diag handler.

Thanks for this workaround. The change also caused one of our downstream unittests to fail because errors stopped being delivered to our ASSERT_EQ code. I haven't quite got to the bottom of where our DiagHandler gets set up yet but cherry-picking this caused our unittest to pass again and now I know what I'm looking for

It's just confusing to have multiple ways of setting up a diag handler.

+1, I was particularly confused by the way it still compiled without change but just stopped reporting errors. Can we remove the old mechanism or plumb it into the new one?

ychen added a comment.Mar 3 2021, 9:25 PM

Sorry for the confusion. 5de2d189e6ad was supposed to reduce but not cause more confusion.

Can we remove the old mechanism or plumb it into the new one?

old mechanism means SourceMgr's diagnose handler? It is still needed for contexts where no MCContext/LLVMContext are available such as parsing YAML etc. MLIR also uses it.

With this workaround in place, both mechanisms work, could you please migrate to use the new one and LMK if works for your tests? Right now, AsmParser&MasmParser are two places where either MCContext or LLVMContext is available but we still diagnose through SourceMgr, I would like to remove these and beef up docs on MCContext/LLVMContext/SourceMgr's handlers so it becomes clear which one to use.

Sorry for the confusion. 5de2d189e6ad was supposed to reduce but not cause more confusion.

No worries, it looks like it will reduce it overall there's just some migration difficulty on the way

Can we remove the old mechanism or plumb it into the new one?

old mechanism means SourceMgr's diagnose handler? It is still needed for contexts where no MCContext/LLVMContext are available such as parsing YAML etc. MLIR also uses it.

I believe so, the one that was installed in the SourceMgr when AsmParser::AsmParser() was called and replaced by that constructor. At the moment (without this workaround), it seems that it replaces the handler promising to call it from inside the new one but then doesn't.
We seem to use both LLVMContext's handler and SourceMgr's depending on whether the input is a text file or not. The unittest was using SourceMgr's handler.

With this workaround in place, both mechanisms work, could you please migrate to use the new one and LMK if works for your tests? Right now, AsmParser&MasmParser are two places where either MCContext or LLVMContext is available but we still diagnose through SourceMgr, I would like to remove these and beef up docs on MCContext/LLVMContext/SourceMgr's handlers so it becomes clear which one to use.

I'll give it a go. It looks like I have a second problem hiding behind the first which I'm not sure how to solve yet. It only appears in our downstream tests because the test has a warning it's not supposed to have but has been hidden by llvm-lit -s. That warning is currently fatal (it calls llvm_unreachable) because MCContext::diagnose() expects SrcMgr or InlineSrcMgr to be non-null but we have neither because our MCContext was set up for disassembling an executable so we didn't have a SourceMgr. I can work around it by removing the llvm_unreachable() in MCContext::diagnose() but that results in passing a nullptr being passed to a const SourceMgr &. I think it might need to be made a const SourceMgr * to support this case

It looks like I have a second problem hiding behind the first which I'm not sure how to solve yet. It only appears in our downstream tests because the test has a warning it's not supposed to have but has been hidden by llvm-lit -s. That warning is currently fatal (it calls llvm_unreachable) because MCContext::diagnose() expects SrcMgr or InlineSrcMgr to be non-null but we have neither because our MCContext was set up for disassembling an executable so we didn't have a SourceMgr. I can work around it by removing the llvm_unreachable() in MCContext::diagnose() but that results in passing a nullptr being passed to a const SourceMgr &. I think it might need to be made a const SourceMgr * to support this case

It turns out there's a simpler fix. I can just create a SourceMgr and decline to add any files to it.

I was able to switch our downstream tools to use the MCContext diagnostic handler. For one case I added an empty SourceMgr when the MCContext was created, if you don't do that then SrcMgr can be nullptr even though it's a reference. For the other, I had a little trouble with MCContext's handler being able to store some opaque data and give it back when it calls the handler but I was able to resolve this by wrapping my handler in a capturing lambda and capturing the variable I needed.

ychen added a comment.Mar 5 2021, 12:41 PM

I was able to switch our downstream tools to use the MCContext diagnostic handler. For one case I added an empty SourceMgr when the MCContext was created, if you don't do that then SrcMgr can be nullptr even though it's a reference. For the other, I had a little trouble with MCContext's handler being able to store some opaque data and give it back when it calls the handler but I was able to resolve this by wrapping my handler in a capturing lambda and capturing the variable I needed.

Thanks for the heads up, @dsanders.