fix: do not panic on ref types while safepoint disabled #1196
Conversation
| jump block2(v1) | ||
| block2(v3: r64): | ||
| return v3 | ||
| } |
|
@bjorn3 where should I expect an error with |
|
I think you should remove the |
But no error triggered after removing. |
github-actions
bot
added
the
cranelift
Subscribe to Label ActionThis issue or pull request has been labeled: "c", "r", "a", "n", "e", "l", "i", "f", "t" To subscribe or unsubscribe from this label, edit the |
2 similar comments
Subscribe to Label ActionThis issue or pull request has been labeled: "c", "r", "a", "n", "e", "l", "i", "f", "t" To subscribe or unsubscribe from this label, edit the |
Subscribe to Label ActionThis issue or pull request has been labeled: "c", "r", "a", "n", "e", "l", "i", "f", "t" To subscribe or unsubscribe from this label, edit the |
Subscribe to Label ActionThis issue or pull request has been labeled: "cranelift" Users Subscribed to "cranelift"To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Agreed that the test safepoint ought to be removed in theory.
So this issue is just a bit more complicated than I imagined. Right now the code that checks for unused reference types iterates over all the values; while this is a correct test, there's no right place where it puts an error message, since it's not tied to a particular instruction. In fact, if we iterated over all the instructions, and we'd put the error on the first instruction result type that's a ref type, then we'd have a place where to put the error message. And then we could add the error: annotation in the clif test case. Does it make sense?
| let ty = func.dfg.value_type(val); | ||
| if ty.lane_type().is_ref() { | ||
| panic!("reference types were found but safepoints were not enabled."); | ||
| for block in func.layout.blocks() { |
There was a problem hiding this comment.
What about using references as function parameters, but not using them anywhere? This would skip them.
There was a problem hiding this comment.
Good point! I think a few tests actually need to be added:
- for function parameters
- for EBB params
- for value types in signature refs
Can you add the code and tests for this, please? Thanks!
| @@ -0,0 +1,12 @@ | |||
| test verifier | |||
There was a problem hiding this comment.
The error is reported during regalloc, not during the verifier pass.
| function %test(r64) -> r64 { | ||
| block0(v0: r64): | ||
| v1 = null.r64 | ||
| v2 = is_null v0 |
There was a problem hiding this comment.
There should be an error annotation in this test, so it'd check that the verifier asserts because a ref type is used yet ref types aren't enabled, right?
|
This PR depends on the old x86 backend. Should it be closed or updated? |
|
It looks like this is a fix to the old regalloc, which is now deleted, so I think we can close. Thanks! |
Closes #1156
r? @bnjbvr