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

Add try intrinsics #2614

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

liushuyu
Copy link
Contributor

@liushuyu liushuyu commented Sep 9, 2023

This pull request adds the try intrinsic, making it possible to handle panics.

A new option, -frust-panic is added to specify the panic handling strategy.

Note that currently, only try-catch mechanism is added in this pull request.
To correctly handle the panic structures, an unwinder must also be implemented (in the runtime library).

@liushuyu liushuyu force-pushed the add-intrinsics-try branch 2 times, most recently from 39014a4 to a55d157 Compare September 9, 2023 21:58
@dkm
Copy link
Member

dkm commented Sep 11, 2023

You are also adding support for __builtin_expect

Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

this PR looks great to me, thank you @liushuyu! unfortunately I'm not super well versed in our backend so I'll let someone else review it before merging it :)

Comment on lines 1207 to 1302
tree normal_return_stmt
= Backend::return_statement (fndecl, integer_zero_node, BUILTINS_LOCATION);
tree error_return_stmt
= Backend::return_statement (fndecl, integer_one_node, BUILTINS_LOCATION);
Copy link
Member

Choose a reason for hiding this comment

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

did you find these values in the rustc source? could it be possible to add the link to the source here? :)

Comment on lines 1227 to 1333
tree eh_pointer
= build_call_expr (builtin_decl_explicit (BUILT_IN_EH_POINTER), 1,
integer_zero_node);
catch_call = Backend::call_expression (catch_fn, {user_data, eh_pointer},
nullptr, BUILTINS_LOCATION);

tree catch_block = Backend::block (fndecl, enclosing_scope, {},
UNDEF_LOCATION, UNDEF_LOCATION);
Backend::block_add_statements (catch_block,
std::vector<tree>{catch_call,
error_return_stmt});
// TODO(liushuyu): eh_personality needs to be implemented as a runtime thing
auto eh_construct
= Backend::exception_handler_statement (try_block, catch_block, NULL_TREE,
BUILTINS_LOCATION);
Copy link
Member

Choose a reason for hiding this comment

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

this looks good to me, but I have no real idea of what the try intrinsic does or how it is implemented. I also wonder if this is how rustc_codegen_gcc handles it? @antoyo @GuillaumeGomez

Copy link
Contributor

Choose a reason for hiding this comment

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

We do it here if you want to take a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our implementation of panics doesn't work correctly with optimizations enabled, though. Not sure why: I still need to investigate.

gcc/rust/backend/rust-compile-intrinsic.cc Outdated Show resolved Hide resolved
Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

I am in favor of merging this sooner than later its a great starting point

@philberty
Copy link
Member

@liushuyu are you able to rebase this work i think we should merge this as a starting point.

gcc/rust/ChangeLog:
	* backend/rust-compile-intrinsic.cc: add `try` intrinsic handler.
	* lang.opt: add `-frust-panic` option.
	* rust-lang.cc: enable exception handler code generation.
	* rust-session-manager.cc: add getter and setter for panic
	strategy option.
	* rust-session-manager.h: Likewise.

Signed-off-by: Zixing Liu <[email protected]>
@liushuyu liushuyu marked this pull request as ready for review December 2, 2024 17:25
@liushuyu
Copy link
Contributor Author

liushuyu commented Dec 2, 2024

Rebased.

I am terribly sorry for hanging this pull request for so long. Now that I work for a company, I no longer have large chunks of time to work on projects like this. I can, however, talk to the company to see if they could allocate some time slots for me to do that.

gcc/rust/ChangeLog:
	* backend/rust-compile-intrinsic.cc: add the new `catch_unwind` variant
	of the `try` intrinsic: this variant can be seen on Rust 1.78+
	and returns `()` instead of `i32`.
gcc/testsuite/ChangeLog:
	* rust/compile/try-catch-unwind-old.rs: add a test to test the older
	try intrinsics from plain old Rust to v1.78.0
	* rust/compile/try-catch-unwind-new.rs: add a test to test the newer
	catch_unwind instrinsics since Rust v1.78.0
@liushuyu
Copy link
Contributor Author

liushuyu commented Dec 2, 2024

Hi @philberty @CohenArthur:
You might want to re-review this pull request as I have made additional changes to account for Rust standard library API changes since 1.78.

I have also added two unit tests for testing the code generation; since the fine people in the rustc_cg_gcc community had figured out how to prevent GCC from optimizing away the try {...} catch {...} blocks, I just copied and pasted their entire homework here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants