-
Notifications
You must be signed in to change notification settings - Fork 193
gccrs:fix ICE with continue/break/return in while condition #4270
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: master
Are you sure you want to change the base?
Conversation
2f08a77 to
6e26f4c
Compare
ea6ad41 to
bd1e92a
Compare
bd1e92a to
e90c207
Compare
|
Having a predicate of type |
|
@powerboat9 Thanks for the feedback! I dont really know how did i miss this.. I've updated the fix to properly handle never-type coercion. Instead of rejecting continue as a value, the compiler now: Detects when the while predicate has type ! (never) |
188c257 to
e1e3018
Compare
philberty
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.
i think this can be simplified a fair bit but good job.
| tree condition; | ||
| if (is_diverging){ | ||
| condition = boolean_true_node; | ||
| }else{ |
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.
the code formatting looks wrong you need to use clang format
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.
Fixed.
| TyTy::BaseType *predicate_type = nullptr; | ||
| bool ok = ctx->get_tyctx()->lookup_type(predicate.get_mappings().get_hirid(), &predicate_type); | ||
| bool is_diverging = false; | ||
| if (ok && predicate_type !=nullptr){ |
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.
i would do rust_assert (ok and != nullptr) this would be an ICE if it happened
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.
Fixed.
| bool ok = ctx->get_tyctx()->lookup_type(predicate.get_mappings().get_hirid(), &predicate_type); | ||
| bool is_diverging = false; | ||
| if (ok && predicate_type !=nullptr){ | ||
| is_diverging = (predicate_type->get_kind() == TyTy::TypeKind::NEVER |
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.
not sure why your checking for error surely we just use error_mark_node if that happens?
| || predicate_type->get_kind() == TyTy::TypeKind::ERROR); | ||
| } | ||
| tree condition; | ||
| if (is_diverging){ |
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.
i think this becomes:
if (predicate_type->isTyTy::NeverType ())
I think reads better
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.
fixed
| condition = boolean_true_node; | ||
| }else{ | ||
| condition = CompileExpr::Compile(predicate,ctx); | ||
| if(condition == error_mark_node || TREE_TYPE(condition) == NULL_TREE){ |
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.
hmm checking for TREE_TYPE null seems odd.. surely its just if error_mark_node? if not you should add 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.
I was overcautious.. i guess.But i agree, with you. It was unwanted.
Fixed it.
| block_expr->as_string ().c_str ()); | ||
| "expected %<()%> got %s", | ||
| block_expr->as_string ().c_str ()); | ||
| infered = new TyTy::ErrorType (expr.get_mappings ().get_hirid ()); |
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.
i think the type check expression defaults infered to be an error type anyways I don't think you need this
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.
fixed.
| if (block_expr->get_kind () == TyTy::TypeKind::ERROR) | ||
| { | ||
| infered = new TyTy::ErrorType (expr.get_mappings ().get_hirid ()); | ||
| context->pop_loop_context (); |
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.
same here the Resolve call checks for nullptr and makes an error type if you read the top of this file
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.
Fixed.
| = TypeCheckExpr::Resolve (expr.get_predicate_expr ()); | ||
| if (predicate_type->get_kind () == TyTy::TypeKind::ERROR) | ||
| { | ||
| infered = new TyTy::ErrorType (expr.get_mappings ().get_hirid ()); |
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.
yeah just pop and return dont need to explicitly set error type here
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.
Fixed
58fd25a to
9daabe7
Compare
9daabe7 to
7e79af7
Compare
|
@philberty LMK, if any changes required. Applied all the ones which you pointed out. :) |
@powerboat9 This should do, lmk your thoughts. |
| tree condition; | ||
| if (predicate_type->get_kind () == TyTy::TypeKind::NEVER) | ||
| { | ||
| condition = boolean_true_node; |
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.
The predicate expression should still be evaluated, as otherwise code that executes but never returns is skipped and continued past.
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.
Hmm, interesting, i guess even though it returns nothing, might have side effects (like function calls) that should still execute, even though it never returns.I guess that is what you meant right @powerboat9
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.
Yep. For example,
fn foo() {
while panic!("stop immediately") {
break
}
never_ever_call_this ();
}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.
@powerboat9 understood, will fix it and let you know !!
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.
@powerboat9 I have done the changes, tested it out with few testcases and examples, works !! Sorry for the multiple request notif..Some bug within my side from github
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.
It's alright. It looks like the code is still broken though -- if the condition doesn't actually make it into the final tree output, it won't make it into the final compiled program.
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.
@powerboat9 Ah !! Sorry i pushed the wrong code.
#4270 (comment)
My solution was l
if (condition == error_mark_node)
{
condition = boolean_true_node;
}
else if (predicate_type->get_kind() == TyTy::TypeKind::NEVER)
{
// The predicate was compiled and added to the tree.
// For NEVER type, we need to ensure it's in the output, so explicitly add it
ctx->add_statement(condition);
condition = boolean_true_node;
}
Haven't committed or updated yet.. But yeah go through this and lmk !!
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.
7e79af7 to
d9c0e76
Compare
d9c0e76 to
8592239
Compare
| tree condition; | ||
| if (predicate_type->get_kind () == TyTy::TypeKind::NEVER) | ||
| { | ||
| condition = boolean_true_node; |
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.
It's alright. It looks like the code is still broken though -- if the condition doesn't actually make it into the final tree output, it won't make it into the final compiled program.
8592239 to
06051e0
Compare
0007616 to
e14f017
Compare
philberty
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.
nearly there
| = ctx->get_tyctx ()->lookup_type (predicate.get_mappings ().get_hirid (), | ||
| &predicate_type); | ||
| rust_assert (ok && predicate_type != nullptr); | ||
| tree condition; |
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.
you dont need to declare the variable seperate just do tree condition = CompileExpr::Compile (predicate, ctx)
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.
@philberty Haha yeah, that was ugly, fixed.
| rust_assert (ok && predicate_type != nullptr); | ||
| tree condition; | ||
| condition = CompileExpr::Compile (predicate, ctx); | ||
| if (condition == error_mark_node) |
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.
you can just ignore the case of condition == error mark node... because it will show up in -fdump-tree-gimple, not sure how it could error something else should spit out and error message so i would just do:
tree condition = CompileExpr::Compile (predicate, ctx);
if (predicate_type->get_kind() == TyTy::TypeKind::NEVER)
{
// The predicate was compiled and added to the tree.
// For NEVER type, we need to ensure it's in the output, so explicitly add it
ctx->add_statement(condition);
condition = boolean_true_node;
}
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.
@philberty Resolved.
Fixes Rust-GCC#3977 The predicate expression must be evaluated before type checking to ensure side effects occur even when the predicate has never type. This prevents skipping function calls, panics, or other side effects in diverging predicates. gcc/rust/ChangeLog: * backend/rust-compile-expr.cc (CompileExpr::visit): Always evaluate predicate expression before checking for never type to preserve side effects in while loop conditions. gcc/testsuite/ChangeLog: * rust/compile/issue-3977.rs: New test. Signed-off-by: Harishankar <harishankarpp7@gmail.com>
e14f017 to
c2a70d2
Compare

Fixes ICE when continue/break/return/loop are used as while conditions. The compiler now emits a proper error message instead of crashing.
Fixes #3977
Thank you for making Rust GCC better!
If your PR fixes an issue, you can add "Fixes #issue_number" into this
PR description and the git commit message. This way the issue will be
automatically closed when your PR is merged. If your change addresses
an issue but does not fully fix it please mark it as "Addresses #issue_number"
in the git commit message.
Here is a checklist to help you with your PR.
make check-rustpasses locallyclang-formatgcc/testsuite/rust/Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.
*Please write a comment explaining your change. This is the message
that will be part of the merge commit.