-
Notifications
You must be signed in to change notification settings - Fork 22
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
fix: abi transformation tests #284
Conversation
This commit ensures the default result type for ops that return u32 is bitcasted to i32, as that is the default integral type on Wasm.
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.
Looking great!
@@ -336,31 +342,29 @@ export.realign_dw # [chunk_hi, chunk_mid, chunk_lo, offset] | |||
swap.1 # [x_hi, x_lo] |
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.
@bitwalker I think the issue is that the hi and lo parts of the value are in reverse order of what is expected by the test assertion. I see it on the stack after the last swap.1
instruction in the realign_dw
function. I have a gut feeling that there is either one extra hi-lo parts swap along the load/store
way or one is missing.
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 argument values in realign_dw
call has mid and lo parts swapped. I believe this is due to the store_dw
target memory layout (# target memory layout: [hi, ..], [..lo, mid]
) in the last else
block differs from the expected memory layout in load_dw
(# memory layout: [hi, ..], [<unused>, <unused>, mid, lo]
). The mid
and lo
are swapped.
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 in c010bae
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.
Nice catch! Looks like that fixes it, I'll follow up once I've validated get_inputs
and blake3
tests!
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've verified (and fixed up/re-enabled the get_inputs
test), looking into blake3 now
memory intrinsics.
Both of the ABI transformation tests are now passing, so once CI is green, I'll merge this and we can close out the alpha milestone. There's a few administrative things to take care of, namely documentation and a final release containing these fixes, but those are minor tasks. |
This PR contains the fixes necessary to get the ABI transformation tests working again (namely
get_inputs
andblake3
).@greenhat I'm still working on this, but am adding each fix I make as a separate commit for review. The last remaining (I think) issue that needs to be addressed, is I've found an off-by-one error involving the operand stack that results in a zero value being used as an address, instead of the correct one. I haven't yet isolated where it is being introduced, but it occurs immediately after returning from the MASM
get_inputs
procedure, prior to which everything is working as expected. Might be a coincidence, but I've got to unfortunately do the manual work necessary to figure out what the expected stack state is at each step and then find where it diverges before I can make further progress.As you can see from the commit list though, I found a handful of small issues, all of which caused bugs during testing in different ways, but I'm pretty happy with where things are at now, this last annoying bug is the last thing standing in the way.