Page MenuHomePhabricator

Use llvm_unreachable instead of errs+abort in MCStreamer.cpp (in EmitRawTextImpl). NFC.
ClosedPublic

Authored by kristina on Wed, Jan 2, 10:34 PM.

Details

Summary

NFC change to handle it more consistently in line with other errors, though I'd prefer a quick review since I'm not sure if there's a reason it's was written like that.

(Tidying up parts of MCStreamer code before a bigger differential)

Diff Detail

Repository
rL LLVM

Event Timeline

kristina created this revision.Wed, Jan 2, 10:34 PM
kristina added a reviewer: rnk.Thu, Jan 3, 6:58 AM
rnk added a comment.Thu, Jan 3, 9:52 AM

I don't think this should be llvm_unreachable, I think it should be report_fatal_error. LLVM has some some support for out of tree backends, and they could plausibly call this when using a release build of LLVM as a library. If all the callers were in-tree, then calling this would represent an LLVM-internal logic error (forgetting to check hasRawTextSupport()), and unreachable or assert would be appropriate.

kristina updated this revision to Diff 180101.Thu, Jan 3, 10:39 AM

Revised to be a fatal error after discussion and added comment for any future maintainers. Also made empty braced blocks consistent.

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Jan 3, 10:46 AM
This revision was automatically updated to reflect the committed changes.
In D56245#1345316, @rnk wrote:

I don't think this should be llvm_unreachable, I think it should be report_fatal_error. LLVM has some some support for out of tree backends, and they could plausibly call this when using a release build of LLVM as a library. If all the callers were in-tree, then calling this would represent an LLVM-internal logic error (forgetting to check hasRawTextSupport()), and unreachable or assert would be appropriate.

Not a strong disagreement, but I'm not sure I follow this entirely - even for external users, there are preconditions for certain API calls that must be met & can still be considered assertion-level failures (ie: the calling code is buggy if it makes this call, not "the calling code can expect a specific failure behavior & rely on this if it wants to") & sounds like this could be one of those.