New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support exception handling on windows #6419

Merged
merged 2 commits into from Aug 6, 2018

Conversation

Projects
None yet
8 participants
@RX14
Member

RX14 commented Jul 20, 2018

This was a tough one, but I learned a lot along the way and I'm glad I accomplished it.

@asterite

This comment has been minimized.

Contributor

asterite commented Jul 20, 2018

Wat? You are glad you accomplished it?? You are ****ing amazing, Chris!

@bararchy

This comment has been minimized.

Contributor

bararchy commented Jul 20, 2018

My hero!
Great work 🎉

@ukd1

This comment has been minimized.

ukd1 commented Jul 20, 2018

@RX14 💯 💯 💯 💯 💯 💯 💯

@RX14

This comment has been minimized.

Member

RX14 commented Jul 20, 2018

A lot of this work was @bcardiff and @txe, I stand on the shoulders of giants.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jul 22, 2018

I used a compiler with this patch to compile my local branch with Windows library bindings. It didn't break anything, but the specs don't contain any expectations about exceptions, because that didn't work until now. But I'll work on that.

I also compiled crinja's spec suite. It's one of the largest Crystal projects I know and there are quite a number of exception raised and rescued. And it worked fine 👍 Not all specs passed (which was to be expected), but none fails because of an error in exception handling.

@RX14

This comment has been minimized.

Member

RX14 commented Jul 22, 2018

@straight-shoota thats fantastic. I wouldn't be surprised if there's still quite a few edge cases in here though. The key hint is usually an "invalid instruction" segfault (llvm adds a ud2 instruction at the end of catch blocks where it thinks it should be unreachable) or a module validation failure.

@Qwerp-Derp Qwerp-Derp referenced this pull request Jul 24, 2018

Open

Coordinate porting to Windows #5430

11 of 21 tasks complete

@RX14 RX14 referenced this pull request Jul 24, 2018

Closed

Win exceptions #5591

@bcardiff

This is great @RX14. I am still OOF one more week, but it was too much temptation to go through this PR 😅 .

There are some questions, doubts and ideas on how to avoid loosing track of the decisions and knowledge needed to understand this change.

@@ -76,7 +76,35 @@ module Crystal
builder.inbounds_gep ptr, index0, index1, name
end
delegate ptr2int, int2ptr, and, or, not, call, bit_cast,
def call(func, name : String = "")

This comment has been minimized.

@bcardiff

bcardiff Jul 24, 2018

Member

Is there a reason why the handling of the funclet operand bundle can't be done in codegen_call_or_invoke? That would leave the plain delegation in place and avoid duplication.

This comment has been minimized.

@RX14

RX14 Jul 24, 2018

Member

Because there's too many places where codegen generates a call outside of codegen_call_or_invoke. I originally had it in codegen_call_or_invoke but I didnt feel like duplicating funclet code in the 50 other places that generate calls (think codegen for constants, there's way more though)

@@ -613,6 +623,12 @@ module Crystal
if return_phi = context.return_phi
return_phi.add @last, node_type
elsif catch_pad = @catch_pad

This comment has been minimized.

@bcardiff

bcardiff Jul 24, 2018

Member

So, returns inside a catch_pad section needs to be handled different. I fail to see that the current specs handle return with values. I neither find specs added on this topic, so maybe is something to add in the suite. WDYT?

This comment has been minimized.

@RX14

RX14 Jul 24, 2018

Member

Yeah, I didn't really look at the spec suite since it's impossible to run the compiler spec suite on windows yet because the compiler depends on a bunch of stuff thats not ported.

@@ -41,8 +41,14 @@ module Crystal
value
end
def printf(format, args = [] of LLVM::Value)
call @printf, [global_string_pointer(format)] + args
def printf(format, args = [] of LLVM::Value, catch_pad = nil)

This comment has been minimized.

@bcardiff

bcardiff Jul 24, 2018

Member

call to printf do really need to be handled with the funclet? Up to know the call was never replaced by an invoke, so it was assumed to never raise AFAIK.

This comment has been minimized.

@RX14

RX14 Jul 24, 2018

Member

every call inside a catch pad needs a funclet. This is not about call/invoke and @rescue_block, it's about @catch_pad which is a totally different concern and semantic. This is about telling LLVM which method calls are inside the "catch block", and it applies to every single method call. @rescue_block is about codegenning invoke instructions, and it only matters for calls that raise.

This comment has been minimized.

@bcardiff

bcardiff Jul 24, 2018

Member

Ok. I also missed this was inside the builder, outside the codegen where the handling of the operand bundle is already handled.

@@ -195,6 +203,153 @@ class Crystal::CodeGenVisitor
false
end
private def codegen_windows_seh(node : ExceptionHandler)

This comment has been minimized.

@bcardiff

bcardiff Jul 24, 2018

Member
  1. I think there should be some comments here explaining how the exception data is handled. I know it comes from documentation of llvm seh and trial an error. But there are some decisions w.r.t. how exception is encoded as void* that would help future readers. Also some basic examples of the result in llvm-ir would be great for documentation IMO (l tried to do that at for overflow here in the past).

  2. And I am tempted to suggest some comment or extraction of common parts with the codegen_landing_pad. The handling of multiple ensures is shared. Maybe a strategy struct can be used to encapsulate custom logic for pads vs seh, leaving the existing comments visit(node : ExceptionHandler)/codegen_landing_pad that goes into the details of the handling of the AST. The only real value of this would be to help the reader to understand separate the concepts that are together in this code.

This comment has been minimized.

@RX14

RX14 Jul 24, 2018

Member

I'll definitely look at how to possibly abstract this, if not I'll definitely add some comments.

exception.inspect_with_backtrace(STDERR)
LibC.exit(1)
end
{% if flag?(:debug_raise) %}

This comment has been minimized.

@bcardiff

bcardiff Jul 24, 2018

Member

I would add a TODO note to remote the whole puts here, or just leave it commented for a while.

This comment has been minimized.

@RX14

RX14 Jul 24, 2018

Member

I don't see why we can't leave the flag in indefinitely.

This comment has been minimized.

@bcardiff

bcardiff Jul 24, 2018

Member

If it's going to stay, then it needs to be supported in the other platforms.

This comment has been minimized.

@RX14

RX14 Jul 24, 2018

Member

Oh yeah, my bad.

@@ -912,4 +914,84 @@ class Crystal::CodeGenVisitor
node.value
end
def void_ptr_type_descriptor

This comment has been minimized.

@bcardiff

bcardiff Jul 24, 2018

Member

I'm glad code snippets end up been useful :-). Thanks for tidying them up a bit.

@Qwerp-Derp

This comment has been minimized.

Qwerp-Derp commented Aug 1, 2018

Should this also be on 0.26.0 @bcardiff?

@RX14 RX14 force-pushed the RX14:feature/windows-exceptions branch from 0bab1bc to 0e0e28a Aug 1, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Aug 1, 2018

Pushed an update which merges the windows and linux exception handling code and improves the comments.

@Qwerp-Derp Qwerp-Derp referenced this pull request Aug 3, 2018

Merged

Release 0.26.0 #6475

rescue_block = new_block "rescue"
node_rescues = node.rescues
node_ensure = node.ensure
node_else = node.else
rescue_ensure_block = nil
# Here we transform this:

This comment has been minimized.

@bcardiff

bcardiff Aug 4, 2018

Member

I would say that this whole comment section should stay. Your additions inlines are great, but keeping this sections with numbers and later references would be ideal. They facilitate understanding the later code a lot.

Other than that LGTM.

This comment has been minimized.

@RX14

RX14 Aug 4, 2018

Member

I don't like that block comment because it's very incomplete information, and once you distill it into it's core it's just what I've written below.

Once I start thinking how to improve the comment, I just end up with repeating what i've written below

This comment has been minimized.

@bcardiff

bcardiff Aug 4, 2018

Member

The once you distill time is decreased a lot with a nice introduction like the one that is removed. I think that the intricate nature of the code transformation deserves that kind of introduction. Just restoring those line are enough.

@bcardiff bcardiff added this to the 0.26.0 milestone Aug 4, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Aug 6, 2018

Added a comment @bcardiff

#
# ```
# ```cr

This comment has been minimized.

@Sija

Sija Aug 6, 2018

Contributor

cr here is invalid (that's gfm), it better be empty (like it was before)
ditto all below

This comment has been minimized.

@straight-shoota

straight-shoota Aug 6, 2018

Contributor

It doesn't hurt, though.

This comment has been minimized.

@Sija

Sija Aug 6, 2018

Contributor

It doesn't help either (especially since it was good before that change).

This comment has been minimized.

@RX14

RX14 Aug 6, 2018

Member

who cares

This comment has been minimized.

@Sija

Sija Aug 6, 2018

Contributor

who cares, the comment isn't for anyone but humans

Yeah, and humans know what cr stands for...

This comment has been minimized.

@bcardiff

bcardiff Aug 6, 2018

Member

I agree with @Sija. It's better without the cr. But we can clean up that later (together with one more ```crystal that is somewhere in the code...)

@bcardiff bcardiff merged commit 9a98613 into crystal-lang:master Aug 6, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

omarroth added a commit to omarroth/crystal that referenced this pull request Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment