Skip to content

Conversation

@greenhat
Copy link
Contributor

@greenhat greenhat commented Jul 4, 2024

This PR is stacked on the #227 and should be merged after it

This PR temporarily disables emitting assertion on u32 cast to get past #225 that is caused by #174.

Do not merge this PR if #174 is already fixed.

@greenhat greenhat force-pushed the greenhat/restore-emu-in-itests branch from 407e17b to b7d447e Compare July 5, 2024 13:18
@greenhat greenhat force-pushed the greenhat/i225-disable-assert-u32-cast branch from 4253aa3 to 6f02803 Compare July 5, 2024 13:18
Copy link
Collaborator

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

I've had a chance to look into this issue, and found the problem. I'd suggest we implement the correct fix in this PR, rather than closing this one and opening a new one, but I'll leave that up to you.

///
/// See `is_signed` for semantics and stack effects of the signedness check.
#[inline]
pub fn assert_unsigned_int32(&mut self) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've looked into this issue, and the fix is to remove the call to assert_unsigned_int32 from cast when casting from i32 to u32, from u32 to i32, and from i32 to u64; and replace it with a call to a new helper called assert_i32, see the implementation of assert_i64 in emit/int64.rs for how to implement it.

The problem is that the assert_unsigned_int32 helper is intended to assert that the sign bit of a 32-bit value is unset, not to assert that the value is non-negative, which is how we are using it in cast.

All the other uses of assert_unsigned_int32 are correct

Copy link
Contributor Author

@greenhat greenhat Jul 18, 2024

Choose a reason for hiding this comment

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

I have not found assert_i32 in #242 so I made #250

Base automatically changed from greenhat/restore-emu-in-itests to main July 11, 2024 13:44
@bitwalker
Copy link
Collaborator

This can be closed in favor of #242 once it is merged

@greenhat
Copy link
Contributor Author

Closing in favor of #242

@greenhat greenhat closed this Jul 17, 2024
@greenhat greenhat deleted the greenhat/i225-disable-assert-u32-cast branch July 17, 2024 12:28
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.

3 participants