Skip to content

Conversation

@csoren
Copy link
Contributor

@csoren csoren commented Dec 29, 2025

Your checklist for this pull request

  • I've documented or updated the documentation of every API function and struct this PR changes.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

There were several issues with the M68K disassembler and instruction printer.

Integer displacements were generally printed as unsigned integers, while they are signed in the architecture. This was mainly due to the 16 bit displacement not being sign extended by the disassembler, and the printer not handling negative displacements. The m68k_op_mem.in_disp and .out_disp fields have been changed to int32_t. Please note that this will require updating contrib/objdump/objdump-m68k.py, which currently does a fair bit of two's complement wrangling. It looks like it can be simplified greatly after updating the fields' types, but I am not comfortable with doing that.

The M68K_AM_PCI_INDEX_BASE_DISP addressing mode was not disassembled correctly.

There were several issues with suppressed index registers (which printed as "invalid") and the PC register (which printed as "a16") in the instruction printer.

All in all, this pull request has the following effects:

Before Pull request
lea.l ([$ffd6, a1, d2.w], $ffa9), a3 lea.l ([-$2a, a1, d2.w], -$57), a3
lea.l ([a1, d2.w], $ffa9), a3 lea.l ([a1, d2.w], -$57), a3
lea.l ([$ffd6, a1, d2.w]), a3 lea.l ([-$2a, a1, d2.w]), a3
lea.l ([$ffd6, a1], d2.w, $ffa9), a3 lea.l ([-$2a, a1], d2.w, -$57), a3
lea.l $ffd6(a1, d2.w), a3 lea.l -$2a(a1, d2.w), a3
lea.l $f9ce(a16, d2.w), a3 lea.l $9f2(pc, d2.w), a3
lea.l $fffff9c8(a16, d2.w), a3 lea.l $9f2(pc, d2.w), a3
lea.l $fffff9c0(a16, invalid.w), a3 lea.l $9f2(pc), a3
lea.l $ffd6(d2.l), a3 lea.l -$2a(d2.l), a3

Test plan

All existing tests pass, and tests covering the table above have been added to tests/details/m68k.yaml.

@github-actions github-actions bot added M68K Arch Python Bindings labels Dec 29, 2025
Copy link
Collaborator

@jiegec jiegec left a comment

Choose a reason for hiding this comment

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

Looks good to me. For the python part, maybe we can do this in a subsequent pr.

Copy link
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Looks like a great fix! Thanks a lot! Please see the comments.
Also it is very important that you add a note about the API change in https://github.com/capstone-engine/capstone/blob/next/docs/cs_v6_release_guide.md

require updating contrib/objdump/objdump-m68k.py,

I wasn't even aware of this file. It can be ignored. Everything in contrib is from external contributors and they would need to maintain it, if they have interest in it.
Also, the file wasn't touched since 2016. So ignore it I'd say.

@Rot127
Copy link
Collaborator

Rot127 commented Jan 3, 2026

Of course, please run clang-format-17 again.

@Rot127 Rot127 merged commit 1bc76ea into capstone-engine:next Jan 4, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants