[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.
bkramer on Mar 2 2021, 4:41 AM.Authored by
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
+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?
Sorry for the confusion. 5de2d189e6ad was supposed to reduce but not cause more confusion.
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.
No worries, it looks like it will reduce it overall there's just some migration difficulty on the way
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.
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 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.