This is an archive of the discontinued LLVM Phabricator instance.

Add LLVM-C interface for Windows x64 SEH instructions
AbandonedPublic

Authored by Wallbraker on Feb 24 2017, 2:35 PM.

Details

Summary

This patch is orignally made by Nick Barnes, adapted by Bernard Helyer and finally submitted in its current form by Jakob Bornecrantz.

The LLVM-C interface provides no means to construct catchswitch, catchpad, catchret, cleanuppad, or cleanupret instructions, or to attach a funclet operand bundle to call or invoke instructions (which is required in cleanuppad blocks).

This patch provides the most basic functionality, and is sufficient for the project I'm working on. More could be done, but this much is required to get anywhere.

Event Timeline

Wallbraker created this revision.Feb 24 2017, 2:35 PM
Wallbraker updated this revision to Diff 89784.Feb 25 2017, 6:03 AM

Updated diff from Bernard.

I didn't know how to not make clang-format chew everything up, so I've tried to match the surrounding format as much as possible. I'm not super knowledgeable on the EH stuff, or LLVM's test suite, so it's very likely that I've made a mistake somewhere in adding to echo.cpp, but everything seems to work.

deadalnix edited edge metadata.Feb 25 2017, 6:13 AM

Please use arc to submit patches, without it there is no contaxt and it is very hard to review.

This comment was removed by Wallbraker.
Wallbraker updated this revision to Diff 89785.Feb 25 2017, 6:31 AM
This comment was removed by Wallbraker.
Wallbraker updated this revision to Diff 89786.Feb 25 2017, 6:33 AM

Adding newline to end of file.

deadalnix added inline comments.Feb 25 2017, 6:39 AM
tools/llvm-c-test/echo.cpp
613

See for instance how it is done for calls.

658

It's actually quite surprising that this works at all. You need to use CloneValue to clone values (!) from the old module to the new one.

Wallbraker added inline comments.Feb 25 2017, 9:17 AM
tools/llvm-c-test/echo.cpp
658

If I change this here to:
Args.push_back(CloneValue(LLVMGetOperand(Src, i)));

I get this from the test, when llvm-c-test --echo
LLVM ERROR: Expected a constant expression

685

And this here to:
Args.push_back(CloneValue(LLVMGetOperand(Src, i)));

Wallbraker updated this revision to Diff 89810.Feb 26 2017, 4:04 AM

Added the CloneValue call and it now passes the test again. I had to add
ConstantTokenNone and ConstantNullPointer to the clone_constant_imple
function.

Now the only thing remaining is if we should keep the non-context version
of the LLVMConstTokenNone.

Wallbraker marked 4 inline comments as done.Feb 26 2017, 4:04 AM
deadalnix requested changes to this revision.Feb 26 2017, 12:09 PM
deadalnix added inline comments.
include/llvm-c/Core.h
1481

Please don't add version without context. This is deprecated.

2843

Please fix indentation.

2850

This name is too generic. Please use a more specific name.

2860

For all these function, the formatting is not right. It should be

LLVMValueRef LLVMBuildCleanupRet(LLVMBuilderRef B, LLVMValueRef CleanupPad,
                                 LLVMBasicBlockRef UnwindBB);

to follow LLVM style. Same goes for others.

3035

dito

lib/IR/Core.cpp
2473

There is no test for this. Is it useful at all ?

2519
unwrap(Args, NumArgs);
2528

dito

2541

This line is more than 80 chars long.

2544

one empty line

test/Bindings/llvm-c/seh.ll
21

There need to be cleanuppad with various combination of option here not just one.

25

dito

28

dito

tools/llvm-c-test/echo.cpp
651

This isn't an int.

654

clone.

664

Clone

665

clone

669

clone

672

Use a meaningful name.

673

clone

675

clone

679

wrong type

682

clone

690

clone

691

clone

This revision now requires changes to proceed.Feb 26 2017, 12:09 PM
Wallbraker added inline comments.Feb 26 2017, 12:57 PM
include/llvm-c/Core.h
1481

What should I call the function that I keep, LLVMConstTokenNoneInContext or LLVMConstTokenNone?

bhelyer added inline comments.Feb 26 2017, 2:35 PM
tools/llvm-c-test/echo.cpp
651

The return value on GetNumOperands is, in fact, an int

http://llvm.org/doxygen/group__LLVMCCoreValueUser.html#ga2ad633a6afc7906f1afe329f244240f6

I can see the argument on GetOperand isn't, so I imagine this is the case of an incorrect declaration, should we fix that in this too?

Wallbraker updated this revision to Diff 89820.Feb 26 2017, 3:56 PM
Wallbraker edited edge metadata.
Wallbraker marked 8 inline comments as done.

Fixes for Core.[cpp|h] files.

Wallbraker added inline comments.Feb 26 2017, 3:56 PM
lib/IR/Core.cpp
2519

That fails to compile, any ideas?

Wallbraker edited the summary of this revision. (Show Details)Feb 26 2017, 3:59 PM
deadalnix added inline comments.Feb 26 2017, 5:38 PM
include/llvm-c/Core.h
1481

I think LLVMConstTokenNone is fine. The InContext is a bit redundant.

deadalnix added inline comments.Feb 26 2017, 5:45 PM
lib/IR/Core.cpp
2519

Never mind, this overload exists only with the downcast version of unwrap. This is fine.

tools/llvm-c-test/echo.cpp
651

Hum, that's surprising, and I don't think anyone is expecting a negative number of argument. We'll fix it in the API at some point, but that's outside the scope of this diff. Let's just use an unsigned here.

bhelyer added inline comments.Feb 28 2017, 5:20 AM
lib/IR/Core.cpp
2473

It seems to mirror LLVMBuildCallInPad. I'd be lying if I said I understood what either is for, and neither is tested. So I'm not sure what to do about it.

deadalnix added inline comments.Feb 28 2017, 5:32 AM
lib/IR/Core.cpp
2473

Let's just remove them. If someone is missing it, then we'll discover what it is for :)

NickBarnes added inline comments.Feb 28 2017, 5:51 AM
lib/IR/Core.cpp
2473

They are for calling an outlined 'finally' clause from inside a cleanuppad. Call and Invoke inside a funclet have to have funclet operand bundles, it says so in the doc.

NickBarnes edited edge metadata.Feb 28 2017, 5:54 AM

Sorry to be off the case for so long; I've been working on another project. Great to see that this is being taken up. The compiler I first wrote this for is now functionally complete, using (broadly) the same patch I originally submitted here on top of 3.9.1. What can I usefully do now that I'm back on this?

NickBarnes added inline comments.Feb 28 2017, 5:56 AM
lib/IR/Core.cpp
2473

http://llvm.org/docs/LangRef.html says:
"If any funclet EH pads have been “entered” but not “exited” (per the description in the EH doc), it is undefined behavior to execute a call or invoke which:

does not have a "funclet" bundle and is not a call to a nounwind intrinsic, or
has a "funclet" bundle whose operand is not the most-recently-entered not-yet-exited funclet EH pad."
deadalnix added inline comments.Feb 28 2017, 6:30 AM
lib/IR/Core.cpp
2473

Is funclet the only thing that can be into the bundle ? Of it is then that's a bit weird a bundle is needed in the first place, and if it isn't I think the API needs to be reworked.

Sorry to be off the case for so long; I've been working on another project. Great to see that this is being taken up. The compiler I first wrote this for is now functionally complete, using (broadly) the same patch I originally submitted here on top of 3.9.1. What can I usefully do now that I'm back on this?

Well, I'm out of my depth regarding the actual test file (seh.ll). It would be much better if someone that actually knew what was involved in exception handling expanded it per @deadalnix's suggestion.

Wallbraker updated this revision to Diff 90103.Feb 28 2017, 4:58 PM

Last update from Bernard.

Wallbraker marked 12 inline comments as done.Feb 28 2017, 5:03 PM
Wallbraker marked 3 inline comments as done.Feb 28 2017, 5:06 PM

I think that's the last of the changes needed for the echo.cpp files, can we agree on something for the LLVMBuildCallInPad function and have somebody that knows more of what the IR should look like comment on the IR file.

Wallbraker updated this revision to Diff 90116.Feb 28 2017, 7:17 PM

Only spotted one case of a Value not being cloned. Others use DeclareBB.

Compiling this with Clang generates a call instruction with a funclet operand bundle, of the same sort that I need to be able to make with LLVMBuildCallInPad(). LLVMBuildInvokeInPad() is analogous.

extern void a(void);
extern void b(void);

void c(void) {
    __try {
        a();
    } __finally {
        b();
    }
}

With Clang 3.9.1, -O3, that produces this. Note the line with "funclet" (the results without -O3 are similar but lengthier).

; ModuleID = 'finally.c'
source_filename = "finally.c"
target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc19.0.24215"

; Function Attrs: nounwind uwtable
define i32 @main() local_unnamed_addr #0 personality i8* bitcast (i32 (...)* @__C_specific_handler to i8*) {
  invoke void @a() #2
          to label %1 unwind label %2

; <label>:1:                                      ; preds = %0
  tail call void @b() #3
  ret i32 0

; <label>:2:                                      ; preds = %0
  %3 = cleanuppad within none []
  tail call void @b() #3 [ "funclet"(token %3) ]
  cleanupret from %3 unwind to caller
}

declare void @b() local_unnamed_addr #1

declare void @a() local_unnamed_addr #1

declare i32 @__C_specific_handler(...)

attributes #0 = { nounwind uwtable "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
attributes #1 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
attributes #2 = { noinline }
attributes #3 = { nounwind }

!llvm.module.flags = !{!0}
!llvm.ident = !{!1}

!0 = !{i32 1, !"PIC Level", i32 2}
!1 = !{!"clang version 3.9.1 (branches/release_39)"}

I am willing to hack on this llvm-c-test, but the LLVM project pages are unclear about the pre-requisites for testing. It says that I need GnuWin32, but does not specify which GnuWin32 packages. There are about 100....

Alright, so it looks like the situation is unclear on the funclet thing. Let split that part out in another diff, so we can get what is already in good shape in.

I think the test file could be improved, by putting various combinations of parameters for the different operations. The overall thing doesn't need to be realistic or be useful in any way, just exercise the different possibilities.

include/llvm-c/Core.h
2844

Ok let's remove this guy and figure out the situation in another diff.

tools/llvm-c-test/echo.cpp
651

This is called OpCount in other cases, let's just use the same name all over the place.

I am willing to hack on this llvm-c-test, but the LLVM project pages are unclear about the pre-requisites for testing. It says that I need GnuWin32, but does not specify which GnuWin32 packages. There are about 100....

llvm-c-test itself will compile just with MSVC. The test suite uses things like diff and generally seems to assume a unixy environment, but you can actually just check that the output matches using fc seh.ll.orig seh.ll.echo in place of the diff line, and then someone else here (me or whoever) can check that the tests all work in a *nix environment.

I was just checking everything was working by running this batch file in the test folder, and once I was happy I checked in a Linux VM.

@echo off
..\..\..\build\Debug\bin\llvm-as.exe < seh.ll | ..\..\..\build\Debug\bin\llvm-dis.exe > seh.ll.orig
..\..\..\build\Debug\bin\llvm-as.exe < seh.ll | ..\..\..\build\Debug\bin\llvm-c-test.exe --echo > seh.ll.echo
fc seh.ll.orig seh.ll.echo

(with the ..\..\..\build\Debug\bin\ replaced with a path to wherever your build folder is)

OK, I've got this working in my tree now. The test doesn't test funclet operand bundles ("...InPad") and if extended to test those then it fails (see comment to line 20 in seh.ll).

test/Bindings/llvm-c/seh.ll
21

Add "call void @g() [ "funclet"(token %0) ]" after this line to test the funclet operand bundle functionality.

Wallbraker abandoned this revision.Aug 6 2018, 7:27 AM

This has is already in the C API now, so closing.