Skip to content
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

Win exceptions #5591

Closed
wants to merge 6 commits into from
Closed

Win exceptions #5591

wants to merge 6 commits into from

Conversation

txe
Copy link
Contributor

@txe txe commented Jan 15, 2018

I've ported code related to win exceptions from my previous branch and this code works, more or less.
There is one issue so far, you will need to use --no-debug when cross compile code. I'll fix it soon.
cc @RX14

@RX14 RX14 requested a review from asterite January 15, 2018 23:37
@RX14 RX14 added kind:feature platform:windows Windows support based on the MSVC toolchain labels Jan 15, 2018
@RX14
Copy link
Contributor

RX14 commented Jan 15, 2018

The OSX CI is failing because this line needs to be >= 3.8, not 3.8 or 3.9. OSX CI uses LLVM 5.

Since we don't support LLVM 3.5 any more, I say just remove the condition entirely. We should support that method on all LLVM versions we support.

@asterite
Copy link
Member

@txe Thank you for this!

I guess there's no way to catch exceptions in Windows other than rewrite it to a series of if checks, right? Then that's fine by me.

About it failing without --no-debug, I'm pretty sure it's because the nodes you create when you expand the rescue don't have a location. Make sure to set a location for those.

You can use the ASTNode#at method for that. For example:

def transform(node : Rescue)
  # create an `if` node and set its location to the same as the rescue node
  an_if = If.new(...).at(node)
end

You should do that will all nodes that you create. The codegen debug logic needs all nodes to have a location, otherwise sometimes you will get an LLVM error.

@txe
Copy link
Contributor Author

txe commented Jan 17, 2018

Thanks! I knew it was about creating a new node, just didn't know how to set a location.

@txe txe changed the title [WIP] Win exceptions Win exceptions Jan 18, 2018
@txe
Copy link
Contributor Author

txe commented Jan 18, 2018

I think I finished this PR. For some reason, it couldn't pass The Travis CI.

@asterite
Copy link
Member

@txe Master is failing because a recent PR was merged and that broke it (because it missed a rebase). Not a problem with this PR.

@RX14
Copy link
Contributor

RX14 commented Jan 20, 2018

@txe master's fixed now, can you rebase this PR?


# # 2)
# # If there's an else, we take the value from it.
# # Otherwise, the value is taken from the body.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is commented twice.

# # lp_ret_type = llvm_typer.landing_pad_type
# # lp = builder.landing_pad lp_ret_type, main_fun(self.personality_name), [] of LLVM::Value
# # unwind_ex_obj = extract_value lp, 0
# # ex_type_id = extract_value lp, 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused code?

if node_rescues
# if node_ensure
# rescue_ensure_block = new_block "rescue_ensure"
# end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@rescue_block = rescue_ensure_block || @rescue_block

# node_rescues.each do |a_rescue|
# end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Call.global("raise", Var.new(var_name)).at(node),
] of ASTNode), nil, var_name)])
tap_block = Block.new(@args = [] of Var, ensure_body.clone)
return Expressions.from [Call.new(new_handler, "tap", [] of ASTNode, tap_block)] of ASTNode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't use tap for this, creating new dependencies in the stdlib. I'd rather you just implemented this as

___tempvar = begin ... end
ensure_body
___tempvar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the code was like that, I had to use tap to fix another issue. Please, read this comment. Considering the purpose of SimpleRescues, can we leave it as it is?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the code I put above is equivalent to the tap call, since it captures __tempvar and ensures it's the last item of the Expressions so that it has the correct type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right, I just misunderstood the code.

src/raise.cr Outdated

fun __crystal_personality(version : Int32, actions : LibUnwind::Action, exception_class : UInt64, exception_object : LibUnwind::Exception*, context : Void*) : LibUnwind::ReasonCode
LibUnwind::ReasonCode::NO_REASON
#exception.inspect_with_backtrace(STDERR)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused code

@RX14
Copy link
Contributor

RX14 commented Jan 20, 2018

This snippet should print "before" then "after", but on windows it only prints "before" then exits with code 29.

class Foo < Exception
end

p "before"

begin
  raise "foo"
rescue ex
  ex.class == Foo
end

p "after"

@RX14
Copy link
Contributor

RX14 commented Jan 20, 2018

Seems like unwinding a "long way" has a lot of issues too. For example raising in a deeply nested call rarely prints the exception in main.cr as it should. Again, exit code 29.

It's a fantastic step forwards but it seems a little buggy :(

@straight-shoota
Copy link
Member

@txe Are you still working on this? =) Would be awesome to proceed with Windows support. 👍

@txe
Copy link
Contributor Author

txe commented Jul 3, 2018

@straight-shoota Sorry, I don't think I'm going to work on it.

@bararchy
Copy link
Contributor

bararchy commented Jul 4, 2018

@txe sad to hear that, hope you will decide to tackle it or other issues again in the future

@RX14
Copy link
Contributor

RX14 commented Jul 24, 2018

Superseeded by #6419

@RX14 RX14 closed this Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature platform:windows Windows support based on the MSVC toolchain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants