Page MenuHomePhabricator

Support unwinding from inline assembly
Needs ReviewPublic

Authored by cynecx on Jan 30 2021, 11:20 AM.

Details

Reviewers
rnk
nikic
deadalnix
Group Reviewers
Restricted Project
Restricted Project
Summary

I haven't received any feedback about the approach I've taken yet (https://lists.llvm.org/pipermail/llvm-dev/2021-January/148175.html).
So I thought I'd try my luck here.

A splitted patchset can be found here: https://github.com/cynecx/llvm-project/commits/asmunwind-final

I've taken the following steps to add unwinding support from inline assembly:

  1. Add a new unwind "attribute" (like sideeffect) to the asm syntax:
invoke void asm sideeffect unwind "call thrower", "~{dirflag},~{fpsr},~{flags}"()
    to label %exit unwind label %uexit

2.) Add Bitcode writing/reading support + LLVM-IR parsing.

3.) Emit EHLabels around inline assembly lowering (SelectionDAGBuilder + GlobalISel) when InlineAsm::canThrow is enabled.

4.) Tweak InstCombineCalls/InlineFunction pass to not mark inline assembly "calls" as nounwind.

5.) Add clang support by introducing a new clobber: "unwind", which lower to the canThrow being enabled.

6.) Don't allow unwinding callbr.

Diff Detail

Event Timeline

cynecx created this revision.Jan 30 2021, 11:20 AM
cynecx requested review of this revision.Jan 30 2021, 11:20 AM

Tests are missing right now. But I'd like to get some feedback first whether this approach goes in the right direction.

cynecx edited the summary of this revision. (Show Details)Jan 31 2021, 10:03 AM
cynecx edited the summary of this revision. (Show Details)
cynecx edited the summary of this revision. (Show Details)
cynecx edited the summary of this revision. (Show Details)
cynecx updated this revision to Diff 321479.Feb 4 2021, 9:08 AM

clang-format changes

cynecx edited reviewers, added: rnk, nikic; removed: deadalnix.Feb 4 2021, 9:11 AM
rnk added a comment.Feb 4 2021, 2:22 PM

Basically, this seems totally reasonable and I think this is the right direction.

cynecx updated this revision to Diff 325323.Feb 21 2021, 10:37 AM
cynecx edited the summary of this revision. (Show Details)

Added tests and updated splitted patchset.

@rnk This is ready for a proper review :)

cynecx updated this revision to Diff 325328.Feb 21 2021, 11:44 AM

clang-format

cynecx added a comment.Mar 3 2021, 8:34 AM

Weekly ping :)

Weekly ping!

Weekly Ping!

The new operand to InlineAsm needs to be handled in llvm/lib/Transforms/Utils/ValueMapper.cpp otherwise you will end up with a bug similar to https://bugs.llvm.org/show_bug.cgi?id=45291.

Some tests with other unwinding models such as SJLJ and SEH would be nice.

clang/lib/CodeGen/CGStmt.cpp
2522

This should be a compiler error diagnostic in SemaAsmStmt.cpp rather than an assert.

llvm/lib/AsmParser/LLParser.cpp
5471
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
2857
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2446

Is this fast path actually useful? The frontend will almost never emit an invoke instruction for inline asm that can't unwind.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
8347

This code is likely to get out of sync with the one in visitInvokable. It would be nice to share the code in a single place, but if that is not practical then at least add a comment in visitInvokable to remind anyone modifying that code to apply the same changes here as well.