Page MenuHomePhabricator

Fix assertion in inline assembler IR gen
ClosedPublic

Authored by amusman on Sep 18 2015, 2:03 AM.

Details

Summary

Several inputs may not refer to one output constraint in inline assembler insertions, clang fails on assertion on such test case here:

llvm/lib/IR/InlineAsm.cpp:46: llvm::InlineAsm::InlineAsm(llvm::FunctionType*, const string&, const string&, bool, bool, llvm::InlineAsm::AsmDialect): Assertion `Verify(getFunctionType(), constraints) && "Function type not legal for constraints!"' failed.

I suggest to check this case in Sema.

Thanks,
Alexander

Diff Detail

Event Timeline

amusman updated this revision to Diff 35068.Sep 18 2015, 2:03 AM
amusman retitled this revision from to Fix assertion in inline assembler IR gen.
amusman updated this object.
amusman added reviewers: rsmith, aaron.ballman, ABataev.
amusman added a subscriber: cfe-commits.
aaron.ballman edited edge metadata.Sep 18 2015, 2:19 PM

Generally LGTM, with one question about guarding against overflow.

lib/Sema/SemaStmtAsm.cpp
447

Is it possible for InputConstraintsInfos.size() to be greater than OutputConstrainInfos.size()? Basically, I'm worried about buffer overflow here with TiedTo.

amusman added inline comments.Sep 21 2015, 3:38 AM
lib/Sema/SemaStmtAsm.cpp
447

The check above (which emits error err_asm_invalid_input_constraint) seems to capture this case so I always get correct TiedTo value here.
What if I add an assertion that TiedTo>=0 && TiedTo <InputMatchedToOutput.size()?

amusman updated this revision to Diff 35222.Sep 21 2015, 3:42 AM
amusman edited edge metadata.

Added an assertion for TiedTo value.

aaron.ballman accepted this revision.Sep 21 2015, 5:53 AM
aaron.ballman edited edge metadata.

With one minor change, LGTM! Thank you for working on this.

~Aaron

lib/Sema/SemaStmtAsm.cpp
447

No need for the TiedTo >= 0 since it's an unsigned value anyway. Our usual way of writing this sort of assert is:

assert(TiedTo < InputMatchedToOutput.size() && "TiedTo value out of range");
This revision is now accepted and ready to land.Sep 21 2015, 5:53 AM
amusman closed this revision.Sep 22 2015, 12:38 AM

Thanks, I've commited the changes in revision 248158 (but forgot to add link in svn commit message).
Alexander