This is an archive of the discontinued LLVM Phabricator instance.

MS inline asm: Allow __asm blocks to set a return value
ClosedPublic

Authored by rnk on Sep 3 2014, 2:04 PM.

Details

Summary

If control falls off the end of a function after an __asm block, MSVC
assumes that the inline assembly filled the EAX and possibly EDX
registers with an appropriate return value. This functionality is used
in inline functions returning 64-bit integers in system headers, so we
need some amount of compatibility.

This is implemented in Clang by adding extra output constraints to every
inline asm block, and storing the resulting output registers into the
return value slot. If we see an asm block somewhere in the function
body, we emit a normal epilogue instead of marking the end of the
function with a return type unreachable.

Normal returns in functions not using this functionality will overwrite
the return value slot, and in most cases LLVM should be able to
eliminate the dead stores.

Fixes PR17201.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 13224.Sep 3 2014, 2:04 PM
rnk retitled this revision from to MS inline asm: Allow __asm blocks to set a return value.
rnk updated this object.
rnk added a reviewer: majnemer.
rnk added a subscriber: Unknown Object (MLST).
majnemer accepted this revision.Sep 3 2014, 3:00 PM
majnemer edited edge metadata.

Wonderful! Does this correctly handle the 'int main' implicit return case? The asm blob should win if it is present, not the implicit return 0. Perhaps a test should be added?

This revision is now accepted and ready to land.Sep 3 2014, 3:00 PM
rsmith added a subscriber: rsmith.Sep 3 2014, 3:07 PM

I think this should do the right thing for int main; we store the implicit 0 into the return value at the start of the function, so the asm should overwrite it. But a test would be great too :)

rnk updated this revision to Diff 13229.Sep 3 2014, 3:25 PM
rnk edited edge metadata.
  • Fix tests. This resulted in something horrible that I'm not happy with, but it works. We should try to figure out something better than rewriting the asm string with complex string matching.
rnk updated this revision to Diff 13230.Sep 3 2014, 3:33 PM
  • Add implicit zero return test case.
majnemer added inline comments.Sep 3 2014, 3:53 PM
lib/CodeGen/TargetInfo.cpp
644 ↗(On Diff #13230)

Pos == DigitStart, they point to the first non dollar character.

645 ↗(On Diff #13230)

If the character at DigitStart is not a number, DigitStart == DigitEnd.

657 ↗(On Diff #13230)

If DigitStart == DigitEnd, wouldn't Pos be unchanged at this point? I guess that would be handled by the next iteration of the loop.

rnk added a comment.Sep 4 2014, 11:31 AM

Thanks! It sounds like this rewrite step is still our best idea for handling this. I'm going to land this then.

lib/CodeGen/TargetInfo.cpp
645 ↗(On Diff #13230)

That should be OK, getAsInteger will return an error.

657 ↗(On Diff #13230)

We should be fine. getAsInteger should fail, so we hit the else clause, stream an empty string, and continue the next iteration looking for dollars. That said, this would already be a malformed inline asm string, which Sema shouldn't produce.

Of course, you never know with stringly typed data structures. :)

rnk closed this revision.Sep 4 2014, 1:14 PM
rnk updated this revision to Diff 13281.

Closed by commit rL217187 (authored by @rnk).