-
-
Notifications
You must be signed in to change notification settings - Fork 115
Implement support for typed catch clauses #610
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
base: main
Are you sure you want to change the base?
Conversation
jeaye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work, Jianling. This is not ready for any more review, I think.
To start with, we have a LOT of new code here and not much documentation to follow it. Also, a lot of the new code doesn't match jank's style, for things you know we do. CI is also failing, since the tests don't pass.
The testing changes include a large amount of over-testing, which I suspect comes from an LLM rather than a conceived testing plan. I tried to identify the redundancies, but honestly there's so much that I know I missed some.
I know this feature is complex. That is all the more reason why we need to carefully craft it, though.
|
|
||
| do_ref body; | ||
| jtl::option<catch_> catch_body{}; | ||
| native_vector<jtl::option<catch_>> catch_bodies{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need for an option here, now that we're using a vector. Before, we could have 0 or 1 catch body, so we used an option. Now we can have 0 or N, which the vector affords us.
|
|
||
| template <typename T> | ||
| object_ref to_runtime_data(native_vector<jtl::option<T>> const &m) | ||
| { | ||
| runtime::detail::native_persistent_vector ret; | ||
| for(auto const &e : m) | ||
| { | ||
| (void)ret.push_back(to_runtime_data(e)); | ||
| } | ||
| return make_box<obj::persistent_vector>(ret); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed by not using an option for catch bodies anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have a "to_runtime_data" for native_vector, so we still need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, in that case we need to fix the code. The (void) you added is to silence an error telling you that you're not using the data structure correctly. This is a persistent vector. You cannot change it. So push_back returns a new vector, which you're ignoring. In this code, ret starts empty and remains empty through the loop.
If you create a transient vector, you can mutate that before turning it into a persistent vector.
| * exact type for "char". */ | ||
| if(sym == "char") | ||
| { | ||
| if(auto const res = resolve_literal_type("char"); res.is_ok()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This is a very heavy-weight option. Resolving literal types like this requires JIT compiling C++ code, which is the slowest thing jank can do. What about just using
Cpp::GetType("char")? - Please use direct initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we cannot. using Cpp::GetType("char") would fail the following test:
(cpp/raw "void throw_char_z() { throw 'z'; }")
(assert
(= \z
(try
(cpp/throw_char_z)
(catch cpp/int i
(println "Wrong: caught int")
\0)
(catch cpp/char c c))))
and the IR generated is using a "signed char"
invoke void @clojure_core_G__24202(ptr null, i32 0, ptr null, ptr null)
to label %invoke.cxx.normal unwind label %lpad
lpad: ; preds = %entry
%1 = landingpad { ptr, i32 }
catch ptr @_ZTIi
catch ptr @_ZTIa # <-- signed char
if we use resolve_literal_type("char"), then the IR we got is:
invoke void @clojure_core_G__24203(ptr null, i32 0, ptr null, ptr null)
to label %invoke.cxx.normal unwind label %lpad
lpad: ; preds = %entry
%1 = landingpad { ptr, i32 }
catch ptr @_ZTIi
catch ptr @_ZTIc # <-- char
| jtl::string_result<jtl::ptr<void>> resolve_literal_type(jtl::immutable_string const &literal); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be needed, since it's declared in the header file, which we include.
| return error::analyze_invalid_try( | ||
| "A symbol is required after 'catch', which is used as the binding to " | ||
| "hold the exception value.", | ||
| "Catch clause requires a type, a symbol, and a body.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct English anymore. Error messages must be very clear, with correct grammar. Also, a body is not required for a catch clause. Maybe something like:
Each 'catch' form requires the type of exception to catch and a symbol for the name of the exception value.
| } | ||
|
|
||
| void | ||
| llvm_processor::impl::register_parent_catch_clauses(llvm::LandingPadInst *landing_pad, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pointer can be const.
| llvm::BasicBlock *normal_dest | ||
| = llvm::BasicBlock::Create(*llvm_ctx, | ||
| "invoke.cxx.normal", | ||
| ctx->builder->GetInsertBlock()->getParent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please always use direct initialization.
| return ret; | ||
| } | ||
|
|
||
| void llvm_processor::impl::route_unhandled_exception(llvm::Value *ex_ptr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting here, we have nearly 300 lines of code which are not documented. There are a few comments, but they all say WHAT, not WHY. We need to do more here.
| { | ||
| auto const catch_type{ catch_clause.unwrap().type }; | ||
| auto const exception_rtti{ Cpp::MangleRTTI(catch_type) }; | ||
| if constexpr(jtl::current_platform == jtl::platform::macos_like) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only macOS?
| auto const typeid_fn{ llvm_module->getOrInsertFunction("llvm.eh.typeid.for.p0", | ||
| typeid_fn_type) }; | ||
|
|
||
| std::vector<llvm::BasicBlock *> catch_blocks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pleas use native_vector unless you're interacting with an API which requires std::vector.
This change updates the try special form to support multiple, typed catch clauses, enabling more granular exception handling and C++ interop. Key changes: Syntax: catch clauses now require an explicit type argument (e.g., (catch cpp/jank.runtime.object_ref e ...)). Parser: Updated parse_try to handle multiple catch clauses with their corresponding types. Codegen: Extended LLVM IR generation to support multiple catch blocks. Tests: Updated existing tests to use the new explicit catch syntax and added new tests for multiple catches, nested try-catch-finally blocks, etc..
**Object Slicing Prevention (lines 2766-2783)** - Added automatic promotion of C++ class/struct catch types to lvalue references - Prevents object slicing when catching derived classes by base type - Preserves polymorphic behavior (virtual methods work correctly) - Exception: jank::runtime types (like object_ref) remain by-value (needs to figure out why) **Duplicate Catch Detection (lines 2785-2796)** - Added compile-time validation to detect duplicate catch clauses **Fixed Pointer Exception Handling (lines 1797-1826)** - Corrected exception value extraction for pointer types - Pointer exceptions: Use caught_ptr directly (it IS the pointer value) - Non-pointer exceptions: Dereference caught_ptr to get the value - Fixes crashes when catching void*, int*, and other pointer types - Added support for reference types and class/struct types **Enhanced Load Detection (line 543)** - Extended auto-load logic to handle pointer and reference types - Ensures proper value extraction from alloca instructions
c6de074 to
6e0df99
Compare
This change updates the try special form to support multiple, typed catch clauses, enabling more granular exception handling and C++ interop.
Key changes:
Syntax: catch clauses now require an explicit type argument (e.g., (catch cpp/jank.runtime.object_ref e ...)).
Parser: Updated parse_try to handle multiple catch clauses with their corresponding types.
Codegen: Extended LLVM IR generation to support multiple catch blocks.
Tests: Updated existing tests to use the new explicit catch syntax and added new tests for multiple catches, nested try-catch-finally blocks, etc..