Skip to content

AArch64: Fix register access for LSE atomics#2953

Open
oleavr wants to merge 1 commit into
capstone-engine:v5from
frida:fix/aarch64-atomics-reg-access
Open

AArch64: Fix register access for LSE atomics#2953
oleavr wants to merge 1 commit into
capstone-engine:v5from
frida:fix/aarch64-atomics-reg-access

Conversation

@oleavr
Copy link
Copy Markdown
Contributor

@oleavr oleavr commented Jun 3, 2026

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

The AArch64 LSE atomics (CAS, CASP, SWP, and the LD/ST families) had empty operand-access
entries, so cs_regs_access() reported neither the value registers read nor the destination written. E.g.
swp w0, w1, [x2] reported only x2 read, nothing written.

Fixes:

  1. Populate the operand-access table in AArch64MappingInsnOp.inc.
  2. Handle CASP register pairs in AArch64_reg_access() — the printer leaves the second pair element
    without access info. Operands 0–1 are read+written, 2–3 read.

No API/struct changes.

Test plan

suite/regress/test_arm64_atomics.py asserts the read/written sets from regs_access() across the affected
families. Fails before the fix, passes after.

python3 suite/regress/test_arm64_atomics.py

Closing issues

None.

@oleavr oleavr changed the base branch from next to v5 June 3, 2026 08:44
@oleavr
Copy link
Copy Markdown
Contributor Author

oleavr commented Jun 3, 2026

The base was briefly wrong when I opened this, so the labeler applied every arch/binding label. Base is now v5; only AArch64 and Python apply. I can't edit labels myself — could a maintainer clear the rest? Thanks!

@Rot127 Rot127 removed X86 Arch ARM Arch labels Jun 3, 2026
Copy link
Copy Markdown
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.

Thanks! See the comment.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This needs to be added to the bindings/python/Makefile on the v5 branch. Otherwise it is not run in the CI.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — added a REGRESS_TESTS entry to the check target so CI runs it.

Note: the existing loop ran ./$t from bindings/python/, but the tests live in tests/, so it was already failing before any regress test. Fixed it to cd tests first. Can split that out if you'd rather.

The LSE atomics (CAS, CASP, SWP and the LD<op>/ST<op> families)
had empty operand access, so cs_regs_access reported neither the
value registers read nor the destination written.

Populate the access table, and handle the CASP register pairs in
AArch64_reg_access, whose second element gets no access info from
the printer.
@oleavr oleavr force-pushed the fix/aarch64-atomics-reg-access branch from cf1b633 to c3c0c7c Compare June 3, 2026 10:50
@Rot127
Copy link
Copy Markdown
Collaborator

Rot127 commented Jun 3, 2026

Let me merge this one first: #2954

@Rot127
Copy link
Copy Markdown
Collaborator

Rot127 commented Jun 3, 2026

Freaking v5 testing. I am so close to rewrite it.

Anyways, the makefile way is broken and I removed that step from the workflow.

Can you please revert the Makefile changes and add the test to ./bindings/python/tests/test_all.py please?
Then rebase and we can merge.

Sorry for all the back and forth. I haven't looked at the v5 testing for a long time and forgot where is what done.

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.

2 participants