-
Notifications
You must be signed in to change notification settings - Fork 197
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.
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. |
7e79af7 to
d9c0e76
Compare
d9c0e76 to
8592239
Compare
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
e14f017 to
c2a70d2
Compare
Yeah completed as per your suggestions,waiting for your review @philberty |
|
you need to fix the commit format CI is failing for you. |
c999c67 to
c218485
Compare
Yep, done @philberty |
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.
yeah that condition isnt correct you still need to make the loop evaluate the expression you should be using -fdump-tree-gimple to verify the logic
| if (predicate_type->get_kind () == TyTy::TypeKind::NEVER) | ||
| { | ||
| 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.
i dont think this is right to just override to boolean_true_node it means the never expression will never get evaluated
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 ctx->add_statement takes care of that
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.
OK I think this is ready but before we merge can you show an example of -fdump-tree-gimple in the git commit message of what we are doing here for this senario? But also put it in the github description
Sure will do. |
c218485 to
87e912c
Compare
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. Proof of fix using -fdump-tree-gimple: __attribute__((cdecl)) struct () test::main () { struct () D.107; <D.101>: <D.103>: { <D.104>: { <D.102>: goto <D.102>; // Side-effect correctly preserved if (0 != 0) goto <D.105>; else goto <D.106>; <D.106>: { } } goto <D.104>; <D.105>: } goto <D.103>; return D.107; } 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. * typecheck/rust-hir-type-check-expr.cc: Update handling of break/continue. gcc/testsuite/ChangeLog: * rust/compile/issue-3977.rs: New test. Signed-off-by: Harishankar <[email protected]>
87e912c to
d8f51c9
Compare
Fixes ICE when continue/break/return/loop are used as while conditions.
Fixes #3977
The predicate expression must be evaluated before type checking to ensure side effects occur even when the predicate has
Nevertype. This prevents skipping function calls, panics, or other side effects in diverging predicates.Proof of fix using
-fdump-tree-gimpleAs requested, here is the GIMPLE dump for the test case
while loop {} {}. It confirms that the infinite loop side-effect is preserved correctly: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.