Skip to content

s390x: Fix instruction encoding and disassembly format bugs#5786

Merged
jameysharp merged 1 commit intobytecodealliance:mainfrom
uweigand:s390x-asm-fixes
Feb 15, 2023
Merged

s390x: Fix instruction encoding and disassembly format bugs#5786
jameysharp merged 1 commit intobytecodealliance:mainfrom
uweigand:s390x-asm-fixes

Conversation

@uweigand
Copy link
Member

  • Fix encoding of the AHY instruction.
  • Fix disassembly format of FIEBR, FIDBR, and LEDBRA instructions.

CC @cfallin @elliottt - this fixes s390x issues detected here: #5780

- Fix encoding of the AHY instruction.
- Fix disassembly format of FIEBR, FIDBR, and LEDBRA instructions.
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Feb 15, 2023
@afonso360
Copy link
Contributor

Hey, I'm trying to understand why fuzzgen didn't catch this when you ran it a few weeks ago. This encoding error only triggers when we try to add a 16bit value directly from memory to a 32 bit register right?

I think fuzzgen can already generate code like that, although we only ever load from the stack, does s390x select a different encoding in those cases?

@uweigand
Copy link
Member Author

Hey, I'm trying to understand why fuzzgen didn't catch this when you ran it a few weeks ago. This encoding error only triggers when we try to add a 16bit value directly from memory to a 32 bit register right?

Yes, and in addition the memory access must use a displacement of 4096 or larger. For smaller displacements, the (correctly encoded) AH instruction is used instead of AHY. This is probably a rare case - not sure if the fuzzer tries to enforce large displacements.

@afonso360
Copy link
Contributor

afonso360 commented Feb 15, 2023

Oh! That explains it, We constrain the stack size quite a lot. We should probably increase that. Thanks!

Edit: Just looked it up, our max stack size is 1024, which is really low.

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

I believe you that this is the correct encoding. 👍

@jameysharp jameysharp merged commit 305000d into bytecodealliance:main Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants