target/i386/cpu.h | 6 +- target/i386/insn.h | 381 ++++++++ target/i386/translate.c | 2032 ++++++++++++++++++++++++++++++++------- 3 files changed, 2095 insertions(+), 324 deletions(-) create mode 100644 target/i386/insn.h
This is a v2 of the patch series posted in [1]. Patches 1-9 are just cleanups; patches 10-39 are something actually interesting. Compared to v1, I started using preprocessor more extensively to generate repetitive boilerplate code; opinions/alternatives are welcome and appreciated. I tried to eliminate as many errors reported by scripts/checkpatch.pl as I could, but there are still some left; AFAICT they appear to be non-applicable false positives caused by preprocessor macros. There is a known flaw of M* operands documented in patches 25 and 39; it will be addressed in v3. (It has some design implications which require larger changes, so that's why I'm not including them right away, but I already have a good idea of how to address this.) Cheers, -Jan Changes from v1: There is in fact little overlap with v1, apart from the minor cleanup patches; I tried a different approach this time. References: 1. https://lists.nongnu.org/archive/html/qemu-devel/2019-07/msg07041.html Jan Bobek (36): target/i386: reduce scope of variable aflag target/i386: use dflag from DisasContext target/i386: use prefix from DisasContext target/i386: use pc_start from DisasContext target/i386: make variable b1 const target/i386: make variable is_xmm const target/i386: add vector register file alignment constraints target/i386: introduce gen_(ld,st)d_env_A0 target/i386: introduce gen_sse_ng target/i386: disable unused function warning temporarily target/i386: introduce mnemonic aliases for several gvec operations target/i386: introduce function ck_cpuid target/i386: introduce instruction operand infrastructure target/i386: introduce helpers for decoding modrm fields target/i386: introduce modifier for direct-only operand decoding target/i386: introduce generic operand alias target/i386: introduce generic load-store operand target/i386: introduce insn.h target/i386: introduce code generators target/i386: introduce instruction translator macros target/i386: introduce Ib (immediate) operand target/i386: introduce M* (memptr) operands target/i386: introduce G*, R*, E* (general register) operands target/i386: introduce RdMw operand target/i386: introduce P*, N*, Q* (MMX) operands target/i386: introduce helper-based code generator macros target/i386: introduce gvec-based code generator macros target/i386: introduce MMX translators target/i386: introduce MMX code generators target/i386: introduce MMX instructions to insn.h target/i386: introduce V*, U*, W* (SSE/AVX) operands target/i386: introduce UdqMq operand target/i386: introduce SSE translators target/i386: introduce SSE code generators target/i386: introduce SSE instructions to insn.h target/i386: introduce memory-pointer operand read/write workarounds Richard Henderson (3): target/i386: Push rex_r into DisasContext target/i386: Push rex_w into DisasContext target/i386: Simplify gen_exception arguments target/i386/cpu.h | 6 +- target/i386/insn.h | 381 ++++++++ target/i386/translate.c | 2032 ++++++++++++++++++++++++++++++++------- 3 files changed, 2095 insertions(+), 324 deletions(-) create mode 100644 target/i386/insn.h -- 2.20.1
Patchew URL: https://patchew.org/QEMU/20190810041255.6820-1-jan.bobek@gmail.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [RFC PATCH v2 00/39] rewrite MMX/SSE instruction translation
Message-id: 20190810041255.6820-1-jan.bobek@gmail.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update] patchew/1565232570-29296-1-git-send-email-bmeng.cn@gmail.com -> patchew/1565232570-29296-1-git-send-email-bmeng.cn@gmail.com
- [tag update] patchew/1565335544-23584-1-git-send-email-bmeng.cn@gmail.com -> patchew/1565335544-23584-1-git-send-email-bmeng.cn@gmail.com
- [tag update] patchew/20190806165429.19327-1-brijesh.singh@amd.com -> patchew/20190806165429.19327-1-brijesh.singh@amd.com
- [tag update] patchew/20190808164117.23348-1-alex.bennee@linaro.org -> patchew/20190808164117.23348-1-alex.bennee@linaro.org
- [tag update] patchew/20190809091940.1223-1-alex.bennee@linaro.org -> patchew/20190809091940.1223-1-alex.bennee@linaro.org
- [tag update] patchew/20190809154153.31763-1-richard.henderson@linaro.org -> patchew/20190809154153.31763-1-richard.henderson@linaro.org
* [new tag] patchew/20190810041255.6820-1-jan.bobek@gmail.com -> patchew/20190810041255.6820-1-jan.bobek@gmail.com
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp'
Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out 'ba1ab360eebe6338bb8d7d83a9220ccf7e213af3'
Cloning into 'roms/edk2'...
Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54'
Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'
Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl'
Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'...
Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'CryptoPkg/Library/OpensslLib/openssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687'
Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl'
Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5'
Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography'
Cloning into 'boringssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f'
Cloning into 'krb5'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168'
Cloning into 'pyca-cryptography'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/opensbi'...
Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4'
Cloning into 'roms/seabios-hppa'...
Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
Cloning into 'roms/skiboot'...
Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff'
Cloning into 'roms/u-boot-sam460ex'...
Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
Cloning into 'slirp'...
Submodule path 'slirp': checked out '126c04acbabd7ad32c2b018fe10dfac2a3bc1210'
Cloning into 'tests/fp/berkeley-softfloat-3'...
Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'tests/fp/berkeley-testfloat-3'...
Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
Cloning into 'ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
Switched to a new branch 'test'
04e3211 target/i386: introduce memory-pointer operand read/write workarounds
887dd17 target/i386: introduce SSE instructions to insn.h
a0c5752 target/i386: introduce SSE code generators
858e3c5 target/i386: introduce SSE translators
5406d1f target/i386: introduce UdqMq operand
e4cdf59 target/i386: introduce V*, U*, W* (SSE/AVX) operands
7444e6d target/i386: introduce MMX instructions to insn.h
da28d62 target/i386: introduce MMX code generators
94dc39a target/i386: introduce MMX translators
ba278a5 target/i386: introduce gvec-based code generator macros
79af504 target/i386: introduce helper-based code generator macros
09fa8db target/i386: introduce P*, N*, Q* (MMX) operands
bb71c26 target/i386: introduce RdMw operand
118d273 target/i386: introduce G*, R*, E* (general register) operands
ba3f6d7 target/i386: introduce M* (memptr) operands
f3c2db4 target/i386: introduce Ib (immediate) operand
62f91f7 target/i386: introduce instruction translator macros
9b08111 target/i386: introduce code generators
1f3c49c target/i386: introduce insn.h
e585f94 target/i386: introduce generic load-store operand
5c2d1fe target/i386: introduce generic operand alias
d0acbb3 target/i386: introduce modifier for direct-only operand decoding
851452a target/i386: introduce helpers for decoding modrm fields
11aa099 target/i386: introduce instruction operand infrastructure
caf31ee target/i386: introduce function ck_cpuid
0eb2363 target/i386: introduce mnemonic aliases for several gvec operations
7b7f4a4 target/i386: disable unused function warning temporarily
065a819 target/i386: introduce gen_sse_ng
7388055 target/i386: introduce gen_(ld, st)d_env_A0
2b3eda7 target/i386: add vector register file alignment constraints
ad5da8c target/i386: make variable is_xmm const
76dbe83 target/i386: make variable b1 const
82b90e5 target/i386: use pc_start from DisasContext
b84b85a target/i386: Simplify gen_exception arguments
6957a05 target/i386: use prefix from DisasContext
65c1b82 target/i386: use dflag from DisasContext
e4bb0c7 target/i386: reduce scope of variable aflag
7f9aa75 target/i386: Push rex_w into DisasContext
9ffb757 target/i386: Push rex_r into DisasContext
=== OUTPUT BEGIN ===
1/39 Checking commit 9ffb757ff5c8 (target/i386: Push rex_r into DisasContext)
2/39 Checking commit 7f9aa75cf627 (target/i386: Push rex_w into DisasContext)
3/39 Checking commit e4bb0c7a0fc4 (target/i386: reduce scope of variable aflag)
4/39 Checking commit 65c1b822caba (target/i386: use dflag from DisasContext)
5/39 Checking commit 6957a05b4bca (target/i386: use prefix from DisasContext)
6/39 Checking commit b84b85a62db1 (target/i386: Simplify gen_exception arguments)
7/39 Checking commit 82b90e53d4a4 (target/i386: use pc_start from DisasContext)
WARNING: line over 80 characters
#64: FILE: target/i386/translate.c:6387:
+ gen_repz_scas(s, ot, s->pc_start - s->cs_base, s->pc - s->cs_base, 1);
WARNING: line over 80 characters
#67: FILE: target/i386/translate.c:6389:
+ gen_repz_scas(s, ot, s->pc_start - s->cs_base, s->pc - s->cs_base, 0);
WARNING: line over 80 characters
#76: FILE: target/i386/translate.c:6399:
+ gen_repz_cmps(s, ot, s->pc_start - s->cs_base, s->pc - s->cs_base, 1);
WARNING: line over 80 characters
#79: FILE: target/i386/translate.c:6401:
+ gen_repz_cmps(s, ot, s->pc_start - s->cs_base, s->pc - s->cs_base, 0);
WARNING: line over 80 characters
#197: FILE: target/i386/translate.c:7062:
+ gen_interrupt(s, EXCP03_INT3, s->pc_start - s->cs_base, s->pc - s->cs_base);
ERROR: line over 90 characters
#483: FILE: target/i386/translate.c:7693:
+ gen_svm_check_intercept(s, s->pc_start, (b & 2) ? SVM_EXIT_INVD : SVM_EXIT_WBINVD);
WARNING: line over 80 characters
#501: FILE: target/i386/translate.c:8090:
+ gen_svm_check_intercept(s, s->pc_start, SVM_EXIT_WRITE_DR0 + reg);
WARNING: line over 80 characters
#509: FILE: target/i386/translate.c:8097:
+ gen_svm_check_intercept(s, s->pc_start, SVM_EXIT_READ_DR0 + reg);
total: 1 errors, 7 warnings, 464 lines checked
Patch 7/39 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/39 Checking commit 76dbe834b68d (target/i386: make variable b1 const)
9/39 Checking commit ad5da8ce24e9 (target/i386: make variable is_xmm const)
10/39 Checking commit 2b3eda73bd32 (target/i386: add vector register file alignment constraints)
11/39 Checking commit 7388055473c0 (target/i386: introduce gen_(ld, st)d_env_A0)
12/39 Checking commit 065a819bc768 (target/i386: introduce gen_sse_ng)
13/39 Checking commit 7b7f4a4bc38e (target/i386: disable unused function warning temporarily)
14/39 Checking commit 0eb2363cd44c (target/i386: introduce mnemonic aliases for several gvec operations)
15/39 Checking commit caf31eefbce8 (target/i386: introduce function ck_cpuid)
16/39 Checking commit 11aa0990bcc5 (target/i386: introduce instruction operand infrastructure)
ERROR: spaces required around that '*' (ctx:WxV)
#35: FILE: target/i386/translate.c:4560:
+ int modrm, insnop_t(opT) *op) \
^
ERROR: spaces required around that '*' (ctx:WxV)
#41: FILE: target/i386/translate.c:4566:
+ int modrm, insnop_t(opT) *op) \
^
ERROR: spaces required around that '*' (ctx:WxV)
#47: FILE: target/i386/translate.c:4572:
+ int modrm, insnop_t(opT) *op) \
^
total: 3 errors, 0 warnings, 47 lines checked
Patch 16/39 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
17/39 Checking commit 851452a3a801 (target/i386: introduce helpers for decoding modrm fields)
18/39 Checking commit d0acbb315247 (target/i386: introduce modifier for direct-only operand decoding)
19/39 Checking commit 5c2d1fec9e5d (target/i386: introduce generic operand alias)
20/39 Checking commit e585f948be20 (target/i386: introduce generic load-store operand)
21/39 Checking commit 1f3c49c0e649 (target/i386: introduce insn.h)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#16:
new file mode 100644
WARNING: line over 80 characters
#82: FILE: target/i386/insn.h:62:
+# define INSN_GRPMEMB_WRRR(grpname, mnem, opcode, feat, opW1, opR1, opR2, opR3)
total: 0 errors, 2 warnings, 87 lines checked
Patch 21/39 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
22/39 Checking commit 9b081118d7bd (target/i386: introduce code generators)
23/39 Checking commit 62f91f70636c (target/i386: introduce instruction translator macros)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#187: FILE: target/i386/translate.c:4882:
+#define INSN_GRPMEMB(grpname, mnem, opcode, feat) \
+ case opcode: \
+ translate_insn( \
+ env, s, CK_CPUID_ ## feat, \
+ gen_insn(mnem)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#193: FILE: target/i386/translate.c:4888:
+#define INSN_GRPMEMB_R(grpname, mnem, opcode, feat, opR1) \
+ case opcode: \
+ translate_insn_r(opR1)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_r(mnem, opR1)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#199: FILE: target/i386/translate.c:4894:
+#define INSN_GRPMEMB_RR(grpname, mnem, opcode, feat, opR1, opR2) \
+ case opcode: \
+ translate_insn_rr(opR1, opR2)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_rr(mnem, opR1, opR2)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#205: FILE: target/i386/translate.c:4900:
+#define INSN_GRPMEMB_W(grpname, mnem, opcode, feat, opW1) \
+ case opcode: \
+ translate_insn_w(opW1)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_w(mnem, opW1)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#211: FILE: target/i386/translate.c:4906:
+#define INSN_GRPMEMB_WR(grpname, mnem, opcode, feat, opW1, opR1) \
+ case opcode: \
+ translate_insn_wr(opW1, opR1)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_wr(mnem, opW1, opR1)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#217: FILE: target/i386/translate.c:4912:
+#define INSN_GRPMEMB_WRR(grpname, mnem, opcode, feat, opW1, opR1, opR2) \
+ case opcode: \
+ translate_insn_wrr(opW1, opR1, opR2)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_wrr(mnem, opW1, opR1, opR2)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#223: FILE: target/i386/translate.c:4918:
+#define INSN_GRPMEMB_WRRR(grpname, mnem, opcode, feat, opW1, opR1, opR2, opR3) \
+ case opcode: \
+ translate_insn_wrrr(opW1, opR1, opR2, opR3)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_wrrr(mnem, opW1, opR1, opR2, opR3)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#229: FILE: target/i386/translate.c:4924:
+#define INSN_GRP_END(grpname) \
+ default: \
+ gen_illegal_opcode(s); \
+ return; \
+ } \
+ \
+ g_assert_not_reached(); \
+ }
ERROR: spaces required around that ':' (ctx:VxE)
#230: FILE: target/i386/translate.c:4925:
+ default: \
^
ERROR: Macros with complex values should be enclosed in parenthesis
#254: FILE: target/i386/translate.c:4953:
+#define CASES_LEG_NP_0F_W0(opcode) \
+ case opcode | M_0F | W_0:
ERROR: Macros with complex values should be enclosed in parenthesis
#256: FILE: target/i386/translate.c:4955:
+#define CASES_LEG_NP_0F_W1(opcode) \
+ case opcode | M_0F | W_1:
ERROR: Macros with complex values should be enclosed in parenthesis
#258: FILE: target/i386/translate.c:4957:
+#define CASES_LEG_F3_0F_W0(opcode) \
+ case opcode | M_0F | P_F3 | W_0:
ERROR: Macros with complex values should be enclosed in parenthesis
#260: FILE: target/i386/translate.c:4959:
+#define CASES_LEG_F3_0F_W1(opcode) \
+ case opcode | M_0F | P_F3 | W_1:
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#265: FILE: target/i386/translate.c:4964:
+#define INSN(mnem, cases, opcode, feat) \
+ cases(opcode) \
+ translate_insn( \
+ env, s, CK_CPUID_ ## feat, \
+ gen_insn(mnem)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#271: FILE: target/i386/translate.c:4970:
+#define INSN_R(mnem, cases, opcode, feat, opR1) \
+ cases(opcode) \
+ modrm = x86_ldub_code(env, s); \
+ translate_insn_r(opR1)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_r(mnem, opR1)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#278: FILE: target/i386/translate.c:4977:
+#define INSN_RR(mnem, cases, opcode, feat, opR1, opR2) \
+ cases(opcode) \
+ modrm = x86_ldub_code(env, s); \
+ translate_insn_rr(opR1, opR2)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_rr(mnem, opR1, opR2)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#285: FILE: target/i386/translate.c:4984:
+#define INSN_W(mnem, cases, opcode, feat, opW1) \
+ cases(opcode) \
+ modrm = x86_ldub_code(env, s); \
+ translate_insn_wr(opW1)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_wr(mnem, opW1)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#292: FILE: target/i386/translate.c:4991:
+#define INSN_WR(mnem, cases, opcode, feat, opW1, opR1) \
+ cases(opcode) \
+ modrm = x86_ldub_code(env, s); \
+ translate_insn_wr(opW1, opR1)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_wr(mnem, opW1, opR1)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#299: FILE: target/i386/translate.c:4998:
+#define INSN_WRR(mnem, cases, opcode, feat, opW1, opR1, opR2) \
+ cases(opcode) \
+ modrm = x86_ldub_code(env, s); \
+ translate_insn_wrr(opW1, opR1, opR2)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_wrr(mnem, opW1, opR1, opR2)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#306: FILE: target/i386/translate.c:5005:
+#define INSN_WRRR(mnem, cases, opcode, feat, opW1, opR1, opR2, opR3) \
+ cases(opcode) \
+ modrm = x86_ldub_code(env, s); \
+ translate_insn_wrrr(opW1, opR1, opR2, opR3)( \
+ env, s, modrm, CK_CPUID_ ## feat, \
+ gen_insn_wrrr(mnem, opW1, opR1, opR2, opR3)); \
+ return;
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#313: FILE: target/i386/translate.c:5012:
+#define INSN_GRP(grpname, cases, opcode) \
+ cases(opcode) \
+ modrm = x86_ldub_code(env, s); \
+ translate_group(grpname)(env, s, modrm); \
+ return;
total: 21 errors, 0 warnings, 309 lines checked
Patch 23/39 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
24/39 Checking commit f3c2db48a44b (target/i386: introduce Ib (immediate) operand)
25/39 Checking commit ba3f6d7b1968 (target/i386: introduce M* (memptr) operands)
26/39 Checking commit 118d273fcee6 (target/i386: introduce G*, R*, E* (general register) operands)
27/39 Checking commit bb71c265978f (target/i386: introduce RdMw operand)
28/39 Checking commit 09fa8dbff00a (target/i386: introduce P*, N*, Q* (MMX) operands)
29/39 Checking commit 79af504812e8 (target/i386: introduce helper-based code generator macros)
30/39 Checking commit ba278a5bc345 (target/i386: introduce gvec-based code generator macros)
31/39 Checking commit 94dc39a1e46f (target/i386: introduce MMX translators)
32/39 Checking commit da28d6295f87 (target/i386: introduce MMX code generators)
33/39 Checking commit 7444e6d9e560 (target/i386: introduce MMX instructions to insn.h)
34/39 Checking commit e4cdf59a44a5 (target/i386: introduce V*, U*, W* (SSE/AVX) operands)
35/39 Checking commit 5406d1ff66b3 (target/i386: introduce UdqMq operand)
36/39 Checking commit 858e3c56faa7 (target/i386: introduce SSE translators)
37/39 Checking commit a0c5752663d6 (target/i386: introduce SSE code generators)
38/39 Checking commit 887dd17910dc (target/i386: introduce SSE instructions to insn.h)
39/39 Checking commit 04e3211a003f (target/i386: introduce memory-pointer operand read/write workarounds)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190810041255.6820-1-jan.bobek@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On 8/9/19 9:12 PM, Jan Bobek wrote:
> This is a v2 of the patch series posted in [1]. Patches 1-9 are just
> cleanups; patches 10-39 are something actually interesting. Compared
> to v1, I started using preprocessor more extensively to generate
> repetitive boilerplate code; opinions/alternatives are welcome and
> appreciated.
This is tricky. I'm not keen on code entirely expanded via macros because it
becomes extremely difficult to debug. All statements get recorded at the same
line of the location of the expansion, which makes the gdb "step" command
finish the entire function because there is no next line.
Once upon a time I wrote some code that's extremely macro crazy:
https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=soft-fp/op-common.h;hb=HEAD
It has been extremely difficult to maintain over the years.
We have just recently gotten rid of some of the macros in the softmmu code
https://patchwork.ozlabs.org/project/qemu-devel/list/?series=105441
replacing most of them with inline functions.
A lot of what you have needs very little adjustment to address the debugging
problem. E.g.
> +#define INSNOP_INIT(opT, init_stmt) \
> + static int insnop_init(opT)(CPUX86State *env, DisasContext *s, \
> + int modrm, insnop_t(opT) *op) \
> + { \
> + init_stmt; \
> + }
....
> +INSNOP(
> + M, TCGv,
> + do {
> + if (decode_modrm_mod(env, s, modrm) == 3) {
> + INSNOP_INIT_FAIL;
> + } else {
> + INSNOP_INIT_OK(s->A0);
> + }
> + } while (0),
> + do {
> + assert(*op == s->A0);
> + gen_lea_modrm(env, s, modrm);
> + } while (0),
> + INSNOP_FINALIZE_NOOP)
Rearrange this as
#define INSNOP_INIT(OPT) \
static bool insnop_##OPT##_init(CPUX86State *env, DisasContext *s, \
int modrm, insnop_##OPT##_t *op)
#define INSNOP_PREPARE(OPT) \
static void insnop_##OPT##_prepare(CPUX86State *env, DisasContext *s, \
int modrm, insnop_##OPT##_t *op)
INSNOP_INIT(M)
{
if (decode_modrm_mod(env, s, modrm) == 3) {
INSNOP_INIT_FAIL;
} else {
INSNOP_INIT_OK(s->A0);
}
}
INSNOP_PREPARE(M)
{
assert(*op == s->A0);
gen_lea_modrm(env, s, modrm);
}
etc and suddenly the entire expansion does not occur on a single line.
Further specific commentary to follow.
r~
On 8/10/19 7:35 PM, Richard Henderson wrote:
> On 8/9/19 9:12 PM, Jan Bobek wrote:
>> This is a v2 of the patch series posted in [1]. Patches 1-9 are just
>> cleanups; patches 10-39 are something actually interesting. Compared
>> to v1, I started using preprocessor more extensively to generate
>> repetitive boilerplate code; opinions/alternatives are welcome and
>> appreciated.
>
> This is tricky. I'm not keen on code entirely expanded via macros because it
> becomes extremely difficult to debug. All statements get recorded at the same
> line of the location of the expansion, which makes the gdb "step" command
> finish the entire function because there is no next line.
>
> Once upon a time I wrote some code that's extremely macro crazy:
>
> https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=soft-fp/op-common.h;hb=HEAD
>
> It has been extremely difficult to maintain over the years.
Thank you, that's exactly the feedback I'm looking for! I've played
with the preprocessor in the past just to try out what's possible, but
I've never maintained code that uses it as extensively as this
series. It didn't occur to me that there would be a problem with
stepping it in gdb, for example, but now it seems obvious.
> We have just recently gotten rid of some of the macros in the softmmu code
>
> https://patchwork.ozlabs.org/project/qemu-devel/list/?series=105441
>
> replacing most of them with inline functions.
I'll have to look at it and see how exactly it's done; perhaps I'll
find something that's applicable to my case, too.
> A lot of what you have needs very little adjustment to address the debugging
> problem. E.g.
>
>> +#define INSNOP_INIT(opT, init_stmt) \
>> + static int insnop_init(opT)(CPUX86State *env, DisasContext *s, \
>> + int modrm, insnop_t(opT) *op) \
>> + { \
>> + init_stmt; \
>> + }
> ....
>> +INSNOP(
>> + M, TCGv,
>> + do {
>> + if (decode_modrm_mod(env, s, modrm) == 3) {
>> + INSNOP_INIT_FAIL;
>> + } else {
>> + INSNOP_INIT_OK(s->A0);
>> + }
>> + } while (0),
>> + do {
>> + assert(*op == s->A0);
>> + gen_lea_modrm(env, s, modrm);
>> + } while (0),
>> + INSNOP_FINALIZE_NOOP)
>
> Rearrange this as
>
> #define INSNOP_INIT(OPT) \
> static bool insnop_##OPT##_init(CPUX86State *env, DisasContext *s, \
> int modrm, insnop_##OPT##_t *op)
>
> #define INSNOP_PREPARE(OPT) \
> static void insnop_##OPT##_prepare(CPUX86State *env, DisasContext *s, \
> int modrm, insnop_##OPT##_t *op)
>
> INSNOP_INIT(M)
> {
> if (decode_modrm_mod(env, s, modrm) == 3) {
> INSNOP_INIT_FAIL;
> } else {
> INSNOP_INIT_OK(s->A0);
> }
> }
>
> INSNOP_PREPARE(M)
> {
> assert(*op == s->A0);
> gen_lea_modrm(env, s, modrm);
> }
>
> etc and suddenly the entire expansion does not occur on a single line.
That makes complete sense, thank you! I'll keep the debugging issue in
mind.
> Further specific commentary to follow.
Looking forward to it!
-Jan
© 2016 - 2024 Red Hat, Inc.