From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf2e.google.com (mail-qv1-xf2e.google.com [IPv6:2607:f8b0:4864:20::f2e]) by sourceware.org (Postfix) with ESMTPS id 706E03858D37 for ; Wed, 17 Aug 2022 20:24:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 706E03858D37 Received: by mail-qv1-xf2e.google.com with SMTP id mn10so6158722qvb.10 for ; Wed, 17 Aug 2022 13:24:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=mSTSm5vtoKE7GlvR12nM49dSNREAj8SIhILgTE6meCA=; b=Br5GMVrYgN5XtqKZnbLrWfSp0DlPy1/srxAZEfU9SQqlgXEvGgKkCp2ryjQZBiuLAP j89NGisOwfctBvZz215HN7l3ekrD1fjPjockM1nlU6O2jpIgjQRgBRG3q0nZvqY1UbDl +eErnjecmDN88+EybiO/j6Qc0K9HYeCdY3beQVJc6KqcW6T7aIs4HaichEzlvgVJp1NI 1FrS+TySLLJw45/0zEleXEiPez/7lZnXPKGhnHHU3TFusGr/sKkvUG1iaBU5RfyVkyZD 5QEPwWQ5sEHE5AI++DWMdFxMWf7bKsP6K618ZsKJqKyGjWounP4x/GTspg3vpLFXJT8+ t2Vg== X-Gm-Message-State: ACgBeo1/RF/N4TgzzEXIMW5pXFo6xnHI+ATXHXpxL1PeuLA+uWfSfbcz cJ3uccO4goMB4/TSz48nUEsqVA0vLdL1J5yqSpgYildcMYQ= X-Google-Smtp-Source: AA6agR76HhwKYJA6AcRw0/fK5YZKvc918HciOLZ4Y1htxzyPmSm7aZmHHp1mmKuZEZhk8oGsmqX3NDEr5wgFwa9HSLM= X-Received: by 2002:a05:6214:d82:b0:477:3d7c:1081 with SMTP id e2-20020a0562140d8200b004773d7c1081mr23461143qve.28.1660767895649; Wed, 17 Aug 2022 13:24:55 -0700 (PDT) MIME-Version: 1.0 References: <32216291-fd1f-4579-87de-d24cb7190894@suse.com> <0ac5121d-5318-b065-57c6-b94085ed6b23@suse.com> In-Reply-To: <0ac5121d-5318-b065-57c6-b94085ed6b23@suse.com> From: "H.J. Lu" Date: Wed, 17 Aug 2022 13:24:19 -0700 Message-ID: Subject: Re: [PATCH 4/7] x86: improve match_template()'s diagnostics To: Jan Beulich Cc: Binutils Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3018.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Aug 2022 20:24:58 -0000 On Tue, Aug 16, 2022 at 12:32 AM Jan Beulich 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.