Disassembly from s390_disasm should match what objdump -d spits out. For vector insns that is not the case. E.g. for va: *** mismatch VEX: |va %v6,%v4,%v17,0| vs objdump: |vab %v6,%v4,%v17| *** mismatch VEX: |va %v9,%v4,%v6,1| vs objdump: |vah %v9,%v4,%v6| *** mismatch VEX: |va %v13,%v28,%v23,2| vs objdump: |vaf %v13,%v28,%v23| *** mismatch VEX: |va %v31,%v2,%v7,3| vs objdump: |vag %v31,%v2,%v7| *** mismatch VEX: |va %v5,%v8,%v0,4| vs objdump: |vaq %v5,%v8,%v0|
Created attachment 175790 [details] disassemble cksm, mvcl, and clcl There was no output from the disassembler for these insns: cksm, mvcl, and clcl. Fixed as obvious with this patch.
Created attachment 176081 [details] fix disassembly for BC[R], BR[C]L, and BIC Here are a few samples how the disassembly differed: *** mismatch VEX: |bic 20,0| vs objdump: |bic 0,20| *** mismatch VEX: |bic 20(%r6),0| vs objdump: |bic 0,20(%r6)| *** mismatch VEX: |bic 20(%r4),0| vs objdump: |bic 0,20(%r4,%r0)| *** mismatch VEX: |bih 20(%r12)| vs objdump: |bih 20(%r12,%r0)| *** mismatch VEX: |nopr| vs objdump: |nopr %r6| *** mismatch VEX: |b 12(%r11)| vs objdump: |b 12(%r11,%r0)| *** mismatch VEX: |blh 12(%r11)| vs objdump: |blh 12(%r11,%r0)| *** mismatch VEX: |brc 0,.+0| vs objdump: |jnop c| *** mismatch VEX: |brcl 0,.+0| vs objdump: |jgnop c| All fixed with this patch.
Created attachment 176095 [details] fix disassembly for C[L][G]R[BJ], C[L][G]I[BJ], CL[G]T, CL[FG]IT Replace s390_format_RIEv1 with s390_format_R0UU and s390_format_R0IU. Handling both a signed and unsigned immediate constant field with the same s390_format_... function does not work. Add function s390_format_RSY_R0RD for CLT and CLGT. Those opcodes have extended mnemonics. So adjusting the formerly used s390_format_RSY_RURD wasn't an option as that function is also used for CLM[HY], STCM[HY], and ICM[HY] which don't have extended mnemonics.
Created attachment 176149 [details] fix disassembly for SEL[G]R and SELFHR Register operands were mixed up. No extended mnemonics were used..
Created attachment 176289 [details] fix disassembly for SEL[G]R and SELHFR take #2 The previous version had a silly bug.
Created attachment 176372 [details] check for spec. exceptions / updates asserts This patch is fall-out from working on fixing the disassembly for the vector insns. Finding disassembly that does not match objdump output is done in an automated manner. Now: most vector insns generate a specification exception when a mask field takes on certain reserved values. That means the testcase generator must observe these constraints. So while teaching the generator these constraints I can as well add code to detect those situations when building VEX IR. (In the end we want to generate a SIGILL for a specification exception). Specifically: this patch 1) replaces vassert with s390_insn_assert where appropriate 2) adds missing s390_insn_asserts (many) 3) removes incorrect s390_insn_asserts (few) 4) checks availability of vector opcodes based on hardware capabilities and issues an emulation failure if opcode is not available 5) For s390_vr_get_type and s390_v3_get_ftype remove the mask check as it has already been asserted earlier. Add a vassert for the anxious :) 6) For s390_vr_get_n_elem, add a vassert (the mask was already checked earlier). The function used to return 0 for an out-of-range mask which would have made s390_vector_fp_mulAddOrSub to create side-effect-free VEX IR. This patch depends on the patch from https://bugs.kde.org/show_bug.cgi?id=496950 being applied. No regtest failures.
(In reply to Florian Krohm from comment #6) Thanks for doing this! > ... > Specifically: this patch > 1) replaces vassert with s390_insn_assert where appropriate Yes, many bad vasserts were still present. Looks good. > 2) adds missing s390_insn_asserts (many) Right, quite a few were missing. Looks good. > 3) removes incorrect s390_insn_asserts (few) Looks good. > 4) checks availability of vector opcodes based on hardware capabilities > and issues an emulation failure if opcode is not available I'm not sure about that one. My thought is that issuing an unsupported instruction yields SIGILL on real hardware, so it's reasonable to do the same in Valgrind. Or is there a general guideline when to use EmFail? I honestly don't know, but libvex_emnote.h doesn't contain that many EmFail_* definitions for other architectures, which leads me to believe that we may be abusing this... > 5) For s390_vr_get_type and s390_v3_get_ftype remove the mask check as > it has already been asserted earlier. Add a vassert for the anxious :) That's OK. > 6) For s390_vr_get_n_elem, add a vassert (the mask was already checked > earlier). The function used to return 0 for an out-of-range mask > which would have made s390_vector_fp_mulAddOrSub to create > side-effect-free VEX IR. I don't see this. Is this part missing from the patch? There's a typo in s390_irgen_VFPSO, where the mnemonic is misspelled: > + s390_insn_assert("vfspo", m3 == 3 || (s390_host_has_vxe && m3 >= 2 && m3 <= 4));
(In reply to Andreas Arnez from comment #7) > > 4) checks availability of vector opcodes based on hardware capabilities > > and issues an emulation failure if opcode is not available > I'm not sure about that one. My thought is that issuing an unsupported > instruction yields SIGILL on real hardware, That is not what I observe. This program: int main(void) { asm volatile("cdlgbr %f1,0,%r2,0"); return 0; } compiles and runs just fine on my box. Even though CDLGBR requires the floating-point extension facility, which I do not have (I presume): features : esan3 zarch stfle msa ldisp eimm dfp edat etf3eh highgp rs te vx vxd vxe gs vxe2 vxp sort dflt sie POP says there is an operation exception if the facility is not installed. Apparently, the kernel does not seem to translate that into a SIGILL. > Or is there a general guideline when to use EmFail? commit c6c2bd4b5eb2d45c66c50c4151080a1060f9306d Author: Julian Seward <jseward@acm.org> Date: Fri Jan 20 14:13:55 2006 +0000 Add Ijk_EmFail, a new kind of IR block exit: an emulation failure (fatal error) from which Vex (generated code) cannot recover. s390 is just following suit. > > 6) For s390_vr_get_n_elem, add a vassert (the mask was already checked > > earlier). The function used to return 0 for an out-of-range mask > > which would have made s390_vector_fp_mulAddOrSub to create > > side-effect-free VEX IR. > I don't see this. Is this part missing from the patch? Yes, it is missing, sorry. I wonder... that s390_v3_get_n_elem function has only one use and as its body is effectively a one-liner: return 1 << (4 - m); we can as well inline it and nuke the function. (?) > There's a typo in s390_irgen_VFPSO, where the mnemonic is misspelled: > > + s390_insn_assert("vfspo", m3 == 3 || (s390_host_has_vxe && m3 >= 2 && m3 <= 4)); Yes, good catch!
(In reply to Florian Krohm from comment #8) > features : esan3 zarch stfle msa ldisp eimm dfp edat etf3eh highgp > rs te vx vxd vxe gs vxe2 vxp sort dflt sie HWCAP doesn't list all of the architecture's facilities. On s390/s390x its main purpose is to report hardware facilities that require operating system support. Other facilities are supposed to be queried with STFLE in user space. (OK, just to confuse the reader, a few facilities made their way into HWCAP even though they don't need OS support, but FPEXT is not one of them.) So yes, your system certainly has the FP extension facility. You can verify that by using STFLE, which should report facility bit 37 being set.
(In reply to Andreas Arnez from comment #9) > (In reply to Florian Krohm from comment #8) > HWCAP doesn't list all of the architecture's facilities. My list came from /proc/cpuinfo and I'm pretty sure it shows facilities that do not require kernel support. ldisp comes to mind. But, hey, why should it list all available facilities. That would be too easy. </sarcasm> tests/s390x-features to the rescue. Yes, you're right. My machine does have the fpext facility. And I do get a SIGILL for NNPA, as the corresponding facility is not installed here. So what you describe is correct. Now, sarcasm is rarely constructive. So let me suggest a --list option to s390x-features which lists all facilities (relevant to valgrind) and whether they are supported on the host. As for EmFail vs SIGILL I'd say we stick with the current scheme of using EmFail particularly as we cannot even generate a SIGILL. Generating SIGILL is definitely plan on record as we want to do that for specification exceptions. We can revisit this issue at that time. What do you think.
Created attachment 177165 [details] assert / emfail patch: corrected version - replace vassert with s390_insn_assert where appropriate - add missing s390_insn_asserts (many) - remove incorrect s390_insn_asserts (few) - check availability of vector opcodes based on hardware capabilities and issue an emulation failure if opcode is not available - For s390_vr_get_type and s390_v3_get_ftype remove the mask check as it has already been asserted earlier. Add a vassert for the anxious :) - inline function s390_vr_get_n_elem - fix a typo in a mnemonic: vfspo -> vfpso Note that the patches from #496950 need to go in first.
Created attachment 179051 [details] final bits to fix disassembly for vector insns The tarball contains a set of 45 quite small patches. Each such patch fixes the disassembly for a set of vector insns where the mangling of the base mnemonic to create extended mnemonics is similar enough to be kept in common. This patch set grew incrementally because extended mnemonics for vector insns are quite irregular. So developing a grand plan upfront and then implement it was not going to fly. While doing this two bugs in s390's objdump were found which have been fixed in binutils-2.44.
In bbebdbb2b1094b08ccbd8d09fc65f6fa0b9fecc0 I checked in the "final bits to fix disassembly for vector insns" patch. With that "disasm-test --all --summary --show-spec-exc" reports: *** mismatch VEX: |nop 0(%r4,%r0)| objdump: |nop 0(%r4| *** mismatch VEX: |nop 1(%r4,%r0)| objdump: |nop 1(%r4| *** mismatch VEX: |nop 2(%r12,%r0)| objdump: |nop 2(%r12| *** mismatch VEX: |nop 4095(%r2,%r0)| objdump: |nop 4095(%r2| *** specification exception for insn E66000000007 in vler.dump *** specification exception for insn E66040000007 in vler.dump .... *** specification exception for insn E7640002304D in vrep.dump *** specification exception for insn E764FFFF004D in vrep.dump Total: 16163 tests generated Total: 16142 insns verified Total: 4 disassembly mismatches Total: 21 specification exceptions This disasm-test was using objdump 2.38 under the covers. The mismatches are due to a bug in binutils which has been fixed in the meantime. The specification exceptions occur because not all opcode constraints can be expressed in the test generator. With objdump from current git af829d8b8d5bb0ce658bbad6d33f69dcb534343b there are many miscompares most of which are of this kind: *** mismatch VEX: |bih 1(%r11,%r0)| objdump: |bih 1(%r11,0)| i.e. GPR #0 as base register is now disassembled as 0, formerly %r0. Other miscompares are for the R*SBG family of opcodes.
Fixed in 78b3e5447172090387a981a6d9d6a0aa98455b9b