This is an archive of the discontinued LLVM Phabricator instance.

PR24071 Fix an assert in SelectionDAGBuilder
ClosedPublic

Authored by rovka on May 11 2016, 4:01 AM.

Details

Summary

When processing inline asm that contains errors, make sure we can recover gracefully by creating an UNDEF SDValue for the inline asm statement before returning from SelectionDAGBuilder::visitInlineAsm. This is necessary for consumers that don't exit on the first error that is emitted (e.g. clang) and that would assert later on.

Fixes PR24071.

Diff Detail

Event Timeline

rovka updated this revision to Diff 56871.May 11 2016, 4:01 AM
rovka retitled this revision from to PR24071 Fix an assert in SelectionDAGBuilder.
rovka updated this object.
rovka added a subscriber: llvm-commits.

It seems like it would be better to modify llc as you suggest. Adding an explicitly llc test to Clang is almost certainly the wrong thing to do.

Tim.

rengolin edited edge metadata.May 11 2016, 9:01 AM

It seems like it would be better to modify llc as you suggest. Adding an explicitly llc test to Clang is almost certainly the wrong thing to do.

Hi Tim,

That's a big change though. I agree llc should have diagnostics, too, but this bug shouldn't remain broken until it does, which can take a while and involve a lot of people.

I don't think having the Clang test is wrong, either. Just that it's better if the test is in the same repo and directly tested. I mean, even having an llc test, doesn't stop us from having a Clang one.

cheers,
--renato

rovka added a comment.May 11 2016, 9:03 AM

It seems like it would be better to modify llc as you suggest. Adding an explicitly llc test to Clang is almost certainly the wrong thing to do.

Tim.

Thanks, Tim. I'll try to work on that.

mcrosier resigned from this revision.May 11 2016, 6:21 PM
mcrosier removed a reviewer: mcrosier.
rnk added a subscriber: rnk.May 12 2016, 10:29 AM
rnk added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6792–6796

Can you wrap up most of these calls into a helper like SelectionDAGBuilder::emitInlineAsmError(const Twine &) so that we don't forget to call setValue in the future?

rovka added inline comments.May 13 2016, 9:35 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6792–6796

Good idea. I'll post an updated version of this next week, after we get the new llc diagnostic handler (so I can add the tests too). Thanks.

rovka updated this revision to Diff 57368.May 16 2016, 10:49 AM
rovka updated this object.
rovka edited edge metadata.

Addressed previous comments.

Removed the exit-on-error flag on some of the CodeGen tests, that needed it in order to avoid this assert.

rnk accepted this revision.May 17 2016, 11:13 AM
rnk added a reviewer: rnk.

lgtm

This revision is now accepted and ready to land.May 17 2016, 11:13 AM
rengolin closed this revision.May 17 2016, 12:59 PM

Committed in r269811.