Bug 498942 - s390x: s390_disasm rework
Summary: s390x: s390_disasm rework
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: vex (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Florian Krohm
URL:
Keywords:
Depends on:
Blocks: 495817
  Show dependency treegraph
 
Reported: 2025-01-20 23:10 UTC by Florian Krohm
Modified: 2025-03-15 23:21 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
s390_disasm rework patch (190.11 KB, patch)
2025-01-20 23:12 UTC, Florian Krohm
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Krohm 2025-01-20 23:10:44 UTC
Essentially, the way extended mnemonics are communicated to s390_disasm is a
bit messy and, more importantly, error prone, as we have seen recently. One
reason is that extended mnemonics were added late in the game and sort of
force-fitted on top of the existing scheme.

old:     s390_disasm(ENC3(MNM, GPR, UINT), mnm, r1, i2);
new:   S390_DISASM(MNM(mnm), GPR(r1), UINT(i2));

old:     s390_disasm(ENC2(XMNM, SDXB), S390_XMNM_BIC, r1, dh2, dl2, x2, b2);
new:   S390_DISASM(XMNM(mnm, bic_disasm), MASK(r1), SDXB(dh2, dl2, x2, b2));

The implementation in s390_disasm.h is straight forward.
The order of S390_DISASM arguments is  exactly as it is specified in POP.
For opcodes that have extended mnemonics the 1st argument is an XMNM ctor
taking the base mnemonic and a function pointer as arguments. The function
is responsible for writing out the complete disassembled insn (not just the
extended mnemonic).

The changes in guest_s390_toIR.c and host_s390_defs.c are purely mechanical.
But as I had to change essentially all s390_format_...  and s390_emit_... I've
taken the liberty to fix a few obviously incorrect things:

In guest_s390_toIR.c:

Fix core dump when disassembling an MVCRL insn. MNM is missing.
    s390_disasm(ENC2(UDXB, UDXB), mnm, d1, 0, b1, d2, 0, b2);

Fix s390_format_VRRa_VVVMMM: m6 is not written because an UINT is missing
    s390_disasm(ENC6(MNM, VR, VR, VR, UINT, UINT),
                mnm, v1, v2, v3, m4, m5, m6);
    Affects: VFCE, VFCH, VFCHE,  VFMIN, VFMAX

In host_s390_defs.c:

Fix s390_emit_VLR:
   s390_disasm(ENC3(MNM, VR, UDXB), "vlr", v1, v2);  -->
   s390_disasm(ENC3(MNM, VR, VR), "vlr", v1, v2);

Fix s390_emit_LOC:
    s390_disasm(ENC4(MNM, GPR, UINT, SDXB), "loc", r1, m3, dh2, dl2, 0, b2);
    - m3 is last operand
    - opcode has extended mnemonic

Fix s390_emit_LOCG likewise

Fix s390_emit_LOCGR:
    s390_disasm(ENC4(MNM, GPR, GPR, UINT), "locgr", r1, r2, m3);
    - opcode has extended mnemonic

Fix s390_emit_LOCGHI:
    s390_disasm(ENC4(MNM, GPR, INT, UINT), "locghi", r1, (Int)(Short)i2, m3)
    - opcode has extended mnemonic

Fix s390_emit_MVHHI:
    s390_disasm(ENC3(MNM, UDXB, INT), "mvhhi", d1, 0, b1, i2);
    - i2, an UShort, needs to be sign extended

Tweak s390_emit_TM:
    s390_disasm(ENC3(MNM, UDXB, INT), "tm", d1, 0, b1, i2);
    - i2 is a mask; UINT is less confusing (makes no difference here)

Fix s390_emit_TMY:
    s390_disasm(ENC3(MNM, SDXB, INT), "tmy", dh1, dl1, 0, b1, (Int)(Char)i2);
    - i2 is a mask; UINT is less confusing

I did not bother with vector insns at this time. There are several issues there
to be fixed. E.g. for s390_emit_VCEQ operand m5 is missing which isn't even
available in that function...
I'll do that once I have sorted out the equivalence classes wrt
disassembly for the vector opcodes.

Some overly long S390_DISASM lines are created. This is on purpose because it
simplifies cross checking that the S390_DISASM invocations for a given opcode
in guest_s390_toIR.c and host_s390_defs.c are identical.

This patch needs C11 features (anonymous structs and unions).
Comment 1 Florian Krohm 2025-01-20 23:12:15 UTC
Created attachment 177561 [details]
s390_disasm rework patch
Comment 2 Florian Krohm 2025-03-15 23:21:35 UTC
Fixed in c3049684e26ee4300c797a11c9b88708223771f3