Skip to content

xtensa: sign-extend scaled offsets in decodeOffset_* helpers#2997

Open
Samin061 wants to merge 1 commit into
capstone-engine:nextfrom
Samin061:xtensa-offset-sign-extend
Open

xtensa: sign-extend scaled offsets in decodeOffset_* helpers#2997
Samin061 wants to merge 1 commit into
capstone-engine:nextfrom
Samin061:xtensa-offset-sign-extend

Conversation

@Samin061

Copy link
Copy Markdown
Contributor

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 decodeOffset_* helpers in arch/Xtensa/XtensaDisassembler.c decode the scaled memory offset of the ESP32-S3 ee.* instructions. Each one scales the field only when it is not already aligned, and never sign extends it:

if ((Imm & 0xf) != 0)
	MCOperand_CreateImm0(Inst, (Imm << 4));
else
	MCOperand_CreateImm0(Inst, (Imm));

These offsets are signed, and two generated artifacts in the tree pin down the exact shape. The decoder tables give each field's width, and the printer gives the range it accepts. decodeOffset_64_16Operand reads a 6 bit field and printOffset_64_16_AsmOperand accepts [-512,496] in steps of 16, which is exactly the 64 values of SignExtend64(Imm << 4, 10). The rest of the family lines up the same way: 4 bit against [-128,112], and three 8 bit fields against [-1024,1016], [-2048,2032] and [-512,508]. decodeOffset_128_1Operand and decodeOffset_128_2Operand are the unsigned ones ([0,127] and [0,254]), so they only need the scale.

Two things follow from the missing sign extension. When the decoded value lands outside the printer's range, CS_ASSERT_RET drops the operand, so release builds print the instruction with an operand missing and assert builds abort in the printer. When it happens to land inside the range, it is silently wrong.

cstool esp32 2f71014e before:

ee.vmulas.u16.accx.ld.ip.qup	q4, a2, , q5, q6, q0, q1

and after:

ee.vmulas.u16.accx.ld.ip.qup	q4, a2, -0xf0, q5, q6, q0, q1

The 6 bit field here is 49, so the offset is SignExtend64(49 << 4, 10), or -240. The old code produced 784 and the printer rejected it.

I also tightened each CS_ASSERT_RET_VAL to the field width the tables actually pass. Imm is the raw unsigned field, so the previous isIntN(8, Imm) / isIntN(16, Imm) guards were true for every input. This does touch the line from #2986: the swapped-argument fix there was right, the guard just never matched the field.

Test plan

Added both cases to tests/issues/issues.yaml (the 64_16 sign extension and the 128_2 scaling) and test_xtensa_offset_sign_extension to tests/integration/test_poc.c. On an ASAN/UBSan build test_poc aborts before the fix at XtensaInstPrinter.c:643 with Invalid argument, value must be in range [-512,496], and exits clean with the fix.

To check the change does not move anything it should not, I diffed cstool over 12000 random 4 byte words in esp32 and esp32s2 against an unpatched build. 574 outputs differ: 506 recover an operand the printer had dropped, and 68 correct a silently wrong offset, for example ee.vldbc.16.ip q7, a7, 0x60 becoming 0xc0 and ee.vst.l.64.ip q7, a3, 0x50 becoming 0x280. None of them change a value that was already correct. The existing Xtensa cases under tests/ decode identically before and after, including the #2986 word, whose offset field is 0.

Closing issues

@github-actions github-actions Bot added the Xtensa Arch label Jul 17, 2026
@Rot127

Rot127 commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

@b1llow Please take a look.

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.

Revert this. The unit test below covers it already.

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, reverted. The two cases in issues.yaml cover both paths (the 64_16 sign extension and the 128_2 scaling), so test_poc.c was redundant.

@Samin061
Samin061 force-pushed the xtensa-offset-sign-extend branch from f13d675 to 06f9808 Compare July 19, 2026 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants