[PATCH 4/7] x86: improve match_template()'s diagnostics
H.J. Lu
hjl.tools@gmail.com
Wed Aug 17 20:24:19 GMT 2022
On Tue, Aug 16, 2022 at 12:32 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> At the example of
>
> extractps $0, %xmm0, %xmm0
> insertps $0, %xmm0, %eax
>
> (both having respectively the same mistake of using the wrong kind of
> destination register) it is easy to see that current behavior is far
> from ideal: The former results in "unsupported instruction" for 32-bit
> code simply because the 2nd template we have is a Cpu64 one. Instead we
> should aim at emitting the "best" possible error, which will typically
> be the one where we passed the largest number of checks. Generalize the
> original "specific_error" approach by making it apply to the entire
> matching loop, utilizing that line numbers increase as we pass further
> checks.
> ---
> As to the inval-tls testcase: Why is KMOV special? Are e.g. VMOV or
> other vector insns (legacy or EVEX-encoded) any different? Shouldn't the
> use of the respective reloc types be limited to _exactly_ the insns they
> are intended to be used with? Furthermore having this check in
> match_template() is unhelpful, as the resulting diagnostic isn't aiding
> in understanding what's wrong. Template matching should be left alone
> here, and the issue be diagnosed later, say directly in md_assemble()
> (alongside the various further consistency checks there) or in
> process_operands().
GCC may generate invalid TLS code sequences with KMOV, not other
instructions. We want to catch them by assembler. It is easier to disallow
the invalid instructions.
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -2083,12 +2083,7 @@ operand_size_match (const insn_template
> }
>
> if (!t->opcode_modifier.d)
> - {
> - mismatch:
> - if (!match)
> - i.error = operand_size_mismatch;
> - return match;
> - }
> + return match;
>
> /* Check reverse. */
> gas_assert ((i.operands >= 2 && i.operands <= 3)
> @@ -2105,19 +2100,19 @@ operand_size_match (const insn_template
>
> if (t->operand_types[j].bitfield.class == Reg
> && !match_operand_size (t, j, given))
> - goto mismatch;
> + return match;
>
> if (t->operand_types[j].bitfield.class == RegSIMD
> && !match_simd_size (t, j, given))
> - goto mismatch;
> + return match;
>
> if (t->operand_types[j].bitfield.instance == Accum
> && (!match_operand_size (t, j, given)
> || !match_simd_size (t, j, given)))
> - goto mismatch;
> + return match;
>
> if ((i.flags[given] & Operand_Mem) && !match_mem_size (t, j, given))
> - goto mismatch;
> + return match;
> }
>
> return match | MATCH_REVERSE;
> @@ -6386,6 +6381,17 @@ VEX_check_encoding (const insn_template
> return 0;
> }
>
> +/* Helper function for the progress() macro in match_template(). */
> +static INLINE enum i386_error progress (enum i386_error new,
> + enum i386_error last,
> + unsigned int line, unsigned int *line_p)
> +{
> + if (line <= *line_p)
> + return last;
> + *line_p = line;
> + return new;
> +}
> +
> static const insn_template *
> match_template (char mnem_suffix)
> {
> @@ -6397,8 +6403,9 @@ match_template (char mnem_suffix)
> i386_opcode_modifier suffix_check;
> i386_operand_type operand_types [MAX_OPERANDS];
> int addr_prefix_disp;
> - unsigned int j, size_match, check_register;
> - enum i386_error specific_error = 0;
> + unsigned int j, size_match, check_register, errline = __LINE__;
> + enum i386_error specific_error = number_of_operands_mismatch;
> +#define progress(err) progress(err, specific_error, __LINE__, &errline)
Need a space before (.
>
> #if MAX_OPERANDS != 5
> # error "MAX_OPERANDS must be 5."
> @@ -6436,36 +6443,33 @@ match_template (char mnem_suffix)
> suffix_check.no_ldsuf = 1;
> }
>
> - /* Must have right number of operands. */
> - i.error = number_of_operands_mismatch;
> -
> for (t = current_templates->start; t < current_templates->end; t++)
> {
> addr_prefix_disp = -1;
> found_reverse_match = 0;
>
> + /* Must have right number of operands. */
> if (i.operands != t->operands)
> continue;
>
> /* Check processor support. */
> - i.error = unsupported;
> + specific_error = progress (unsupported);
> if (cpu_flags_match (t) != CPU_FLAGS_PERFECT_MATCH)
> continue;
>
> /* Check Pseudo Prefix. */
> - i.error = unsupported;
> if (t->opcode_modifier.pseudovexprefix
> && !(i.vec_encoding == vex_encoding_vex
> || i.vec_encoding == vex_encoding_vex3))
> continue;
>
> /* Check AT&T mnemonic. */
> - i.error = unsupported_with_intel_mnemonic;
> + specific_error = progress (unsupported_with_intel_mnemonic);
> if (intel_mnemonic && t->opcode_modifier.attmnemonic)
> continue;
>
> /* Check AT&T/Intel syntax. */
> - i.error = unsupported_syntax;
> + specific_error = progress (unsupported_syntax);
> if ((intel_syntax && t->opcode_modifier.attsyntax)
> || (!intel_syntax && t->opcode_modifier.intelsyntax))
> continue;
> @@ -6491,7 +6495,7 @@ match_template (char mnem_suffix)
> }
>
> /* Check the suffix. */
> - i.error = invalid_instruction_suffix;
> + specific_error = progress (invalid_instruction_suffix);
> if ((t->opcode_modifier.no_bsuf && suffix_check.no_bsuf)
> || (t->opcode_modifier.no_wsuf && suffix_check.no_wsuf)
> || (t->opcode_modifier.no_lsuf && suffix_check.no_lsuf)
> @@ -6500,6 +6504,7 @@ match_template (char mnem_suffix)
> || (t->opcode_modifier.no_ldsuf && suffix_check.no_ldsuf))
> continue;
>
> + specific_error = progress (operand_size_mismatch);
> size_match = operand_size_match (t);
> if (!size_match)
> continue;
> @@ -6510,11 +6515,9 @@ match_template (char mnem_suffix)
>
> as the case of a missing * on the operand is accepted (perhaps with
> a warning, issued further down). */
> + specific_error = progress (operand_type_mismatch);
> if (i.jumpabsolute && t->opcode_modifier.jump != JUMP_ABSOLUTE)
> - {
> - i.error = operand_type_mismatch;
> - continue;
> - }
> + continue;
>
> for (j = 0; j < MAX_OPERANDS; j++)
> operand_types[j] = t->operand_types[j];
> @@ -6522,6 +6525,8 @@ match_template (char mnem_suffix)
> /* In general, don't allow
> - 64-bit operands outside of 64-bit mode,
> - 32-bit operands on pre-386. */
> + specific_error = progress (mnem_suffix ? invalid_instruction_suffix
> + : operand_size_mismatch);
> j = i.imm_operands + (t->operands > i.imm_operands + 1);
> if (((i.suffix == QWORD_MNEM_SUFFIX
> && flag_code != CODE_64BIT
> @@ -6550,7 +6555,7 @@ match_template (char mnem_suffix)
> {
> if (VEX_check_encoding (t))
> {
> - specific_error = i.error;
> + specific_error = progress (i.error);
> continue;
> }
>
> @@ -6711,6 +6716,8 @@ match_template (char mnem_suffix)
> i.types[1],
> operand_types[1])))
> {
> + specific_error = progress (i.error);
> +
> /* Check if other direction is valid ... */
> if (!t->opcode_modifier.d)
> continue;
> @@ -6735,6 +6742,7 @@ match_template (char mnem_suffix)
> operand_types[0])))
> {
> /* Does not match either direction. */
> + specific_error = progress (i.error);
> continue;
> }
> /* found_reverse_match holds which of D or FloatR
> @@ -6773,7 +6781,10 @@ match_template (char mnem_suffix)
> operand_types[3],
> i.types[4],
> operand_types[4]))
> - continue;
> + {
> + specific_error = progress (i.error);
> + continue;
> + }
> /* Fall through. */
> case 4:
> overlap3 = operand_type_and (i.types[3], operand_types[3]);
> @@ -6788,7 +6799,10 @@ match_template (char mnem_suffix)
> operand_types[2],
> i.types[3],
> operand_types[3])))
> - continue;
> + {
> + specific_error = progress (i.error);
> + continue;
> + }
> /* Fall through. */
> case 3:
> overlap2 = operand_type_and (i.types[2], operand_types[2]);
> @@ -6803,7 +6817,10 @@ match_template (char mnem_suffix)
> operand_types[1],
> i.types[2],
> operand_types[2])))
> - continue;
> + {
> + specific_error = progress (i.error);
> + continue;
> + }
> break;
> }
> }
> @@ -6814,14 +6831,14 @@ match_template (char mnem_suffix)
> /* Check if vector operands are valid. */
> if (check_VecOperands (t))
> {
> - specific_error = i.error;
> + specific_error = progress (i.error);
> continue;
> }
>
> /* Check if VEX/EVEX encoding requirements can be satisfied. */
> if (VEX_check_encoding (t))
> {
> - specific_error = i.error;
> + specific_error = progress (i.error);
> continue;
> }
>
> @@ -6829,11 +6846,13 @@ match_template (char mnem_suffix)
> break;
> }
>
> +#undef progress
> +
> if (t == current_templates->end)
> {
> /* We found no match. */
> const char *err_msg;
> - switch (specific_error ? specific_error : i.error)
> + switch (specific_error)
> {
> default:
> abort ();
> --- a/gas/testsuite/gas/i386/inval-tls.l
> +++ b/gas/testsuite/gas/i386/inval-tls.l
> @@ -1,3 +1,3 @@
> .*: Assembler messages:
> -.*:3: Error: operand size mismatch for `kmovd'
> -.*:4: Error: operand size mismatch for `kmovd'
> +.*:3: Error: .* `kmovd'
> +.*:4: Error: .* `kmovd'
> --- a/gas/testsuite/gas/i386/noavx512-1.l
> +++ b/gas/testsuite/gas/i386/noavx512-1.l
> @@ -1,14 +1,14 @@
> .*: Assembler messages:
> -.*:25: Error: .*unsupported instruction.*
> +.*:25: Error: .*operand size mismatch.*
> .*:26: Error: .*unsupported masking.*
> .*:27: Error: .*unsupported masking.*
> -.*:47: Error: .*unsupported instruction.*
> +.*:47: Error: .*operand size mismatch.*
> .*:48: Error: .*unsupported masking.*
> .*:49: Error: .*unsupported masking.*
> .*:50: Error: .*not supported.*
> .*:51: Error: .*not supported.*
> .*:52: Error: .*not supported.*
> -.*:69: Error: .*unsupported instruction.*
> +.*:69: Error: .*operand size mismatch.*
> .*:70: Error: .*unsupported masking.*
> .*:71: Error: .*unsupported masking.*
> .*:72: Error: .*not supported.*
> @@ -17,7 +17,7 @@
> .*:75: Error: .*not supported.*
> .*:76: Error: .*not supported.*
> .*:77: Error: .*not supported.*
> -.*:91: Error: .*unsupported instruction.*
> +.*:91: Error: .*operand size mismatch.*
> .*:92: Error: .*unsupported masking.*
> .*:93: Error: .*unsupported masking.*
> .*:94: Error: .*not supported.*
> @@ -27,7 +27,7 @@
> .*:98: Error: .*not supported.*
> .*:99: Error: .*not supported.*
> .*:100: Error: .*not supported.*
> -.*:113: Error: .*unsupported instruction.*
> +.*:113: Error: .*operand size mismatch.*
> .*:114: Error: .*unsupported masking.*
> .*:115: Error: .*unsupported masking.*
> .*:116: Error: .*not supported.*
> @@ -40,7 +40,7 @@
> .*:126: Error: .*not supported.*
> .*:127: Error: .*not supported.*
> .*:128: Error: .*not supported.*
> -.*:135: Error: .*unsupported instruction.*
> +.*:135: Error: .*operand size mismatch.*
> .*:136: Error: .*unsupported masking.*
> .*:137: Error: .*unsupported masking.*
> .*:138: Error: .*not supported.*
> @@ -54,7 +54,7 @@
> .*:149: Error: .*not supported.*
> .*:150: Error: .*not supported.*
> .*:151: Error: .*not supported.*
> -.*:157: Error: .*unsupported instruction.*
> +.*:157: Error: .*operand size mismatch.*
> .*:158: Error: .*unsupported masking.*
> .*:159: Error: .*unsupported masking.*
> .*:160: Error: .*not supported.*
> --- a/gas/testsuite/gas/i386/noavx512-2.l
> +++ b/gas/testsuite/gas/i386/noavx512-2.l
> @@ -1,12 +1,12 @@
> .*: Assembler messages:
> -.*:26: Error: .*unsupported instruction.*
> -.*:27: Error: .*unsupported instruction.*
> +.*:26: Error: .*unsupported masking.*
> +.*:27: Error: .*unsupported masking.*
> .*:29: Error: .*unsupported instruction.*
> .*:30: Error: .*unsupported instruction.*
> .*:32: Error: .*unsupported instruction.*
> .*:33: Error: .*unsupported instruction.*
> -.*:36: Error: .*unsupported instruction.*
> -.*:37: Error: .*unsupported instruction.*
> +.*:36: Error: .*unsupported masking.*
> +.*:37: Error: .*unsupported masking.*
> .*:39: Error: .*unsupported instruction.*
> .*:40: Error: .*unsupported instruction.*
> .*:43: Error: .*unsupported instruction.*
> --- a/gas/testsuite/gas/i386/x86-64-branch-4.l
> +++ b/gas/testsuite/gas/i386/x86-64-branch-4.l
> @@ -1,19 +1,19 @@
> .*: Assembler messages:
> .*:2: Error: invalid instruction suffix for `call'
> .*:3: Error: invalid instruction suffix for `call'
> -.*:4: Error: operand type mismatch for `jmp'
> +.*:4: Error: operand (size|type) mismatch for `jmp'
> .*:5: Error: invalid instruction suffix for `jmp'
> .*:6: Error: invalid instruction suffix for `jmp'
> .*:7: Error: invalid instruction suffix for `ret'
> .*:8: Error: invalid instruction suffix for `ret'
> -.*:11: Error: operand type mismatch for `call'
> +.*:11: Error: operand (size|type) mismatch for `call'
> .*:12: Error: invalid instruction suffix for `call'
> .*:13: Error: invalid instruction suffix for `call'
> -.*:14: Error: operand size mismatch for `call'
> -.*:15: Error: operand type mismatch for `jmp'
> +.*:14: Error: operand (size|type) mismatch for `call'
> +.*:15: Error: operand (size|type) mismatch for `jmp'
> .*:16: Error: invalid instruction suffix for `jmp'
> .*:17: Error: invalid instruction suffix for `jmp'
> -.*:18: Error: operand size mismatch for `jmp'
> +.*:18: Error: operand (size|type) mismatch for `jmp'
> .*:19: Error: invalid instruction suffix for `ret'
> .*:20: Error: invalid instruction suffix for `ret'
> GAS LISTING .*
> --- a/gas/testsuite/gas/i386/x86-64-branch-5.l
> +++ b/gas/testsuite/gas/i386/x86-64-branch-5.l
> @@ -1,19 +1,19 @@
> .*: Assembler messages:
> -.*:2: Error: unsupported syntax for `lcall'
> -.*:3: Error: unsupported syntax for `lfs'
> -.*:4: Error: unsupported syntax for `lfs'
> -.*:5: Error: unsupported syntax for `lgs'
> -.*:6: Error: unsupported syntax for `lgs'
> -.*:7: Error: unsupported syntax for `ljmp'
> -.*:8: Error: unsupported syntax for `lss'
> -.*:9: Error: unsupported syntax for `lss'
> -.*:12: Error: unsupported syntax for `call'
> -.*:13: Error: unsupported syntax for `lfs'
> -.*:14: Error: unsupported syntax for `lfs'
> -.*:15: Error: unsupported syntax for `lgs'
> -.*:16: Error: unsupported syntax for `lgs'
> -.*:17: Error: unsupported syntax for `jmp'
> -.*:18: Error: unsupported syntax for `lss'
> -.*:19: Error: unsupported syntax for `lss'
> +.*:2: Error: invalid instruction suffix for `lcall'
> +.*:3: Error: operand size mismatch for `lfs'
> +.*:4: Error: invalid instruction suffix for `lfs'
> +.*:5: Error: operand size mismatch for `lgs'
> +.*:6: Error: invalid instruction suffix for `lgs'
> +.*:7: Error: invalid instruction suffix for `ljmp'
> +.*:8: Error: operand size mismatch for `lss'
> +.*:9: Error: invalid instruction suffix for `lss'
> +.*:12: Error: operand (size|type) mismatch for `call'
> +.*:13: Error: operand size mismatch for `lfs'
> +.*:14: Error: operand size mismatch for `lfs'
> +.*:15: Error: operand size mismatch for `lgs'
> +.*:16: Error: operand size mismatch for `lgs'
> +.*:17: Error: operand (size|type) mismatch for `jmp'
> +.*:18: Error: operand size mismatch for `lss'
> +.*:19: Error: operand size mismatch for `lss'
> GAS LISTING .*
> #pass
> --- a/gas/testsuite/gas/i386/x86-64-inval-tls.l
> +++ b/gas/testsuite/gas/i386/x86-64-inval-tls.l
> @@ -1,3 +1,3 @@
> .*: Assembler messages:
> -.*:3: Error: operand size mismatch for `kmovq'
> -.*:4: Error: operand size mismatch for `kmovq'
> +.*:3: Error: .* `kmovq'
> +.*:4: Error: .* `kmovq'
>
--
H.J.
More information about the Binutils
mailing list