[PATCH 1/2] x86: refine special casing of insns with MSR as immediate

[H.J. Lu]

On Thu, Apr 9, 2026 at 3:19 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.04.2026 09:13, H.J. Lu wrote:
> > On Thu, Apr 9, 2026 at 2:47 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> PR gas/34028
> >>
> >> i.op[].imms is legitimate to de-reference only when the operand actually
> >> is an immediate. The pointer happens to be non-NULL for most other operand
> >> kinds (i.e. a deref is UB, but would likely not fault), just not for
> >> memory operands without displacement.
> >>
> >> Leverage that the to-be-special-cased insns all only have just a single
> >> immediate, and leverage further that after the immediately preceding
> >> swapping of operands valid immediates will come first. Hence the
> >> conditional can be adjusted to satisfy the criteria above, and no loop is
> >> needed at all.
> >>
> >> With the conditional changed, leverage the property also in optimize_imm()
> >> itself, reducing the number of loop iterations.
> >>
> >> --- a/gas/config/tc-i386.c
> >> +++ b/gas/config/tc-i386.c
> >> @@ -7303,7 +7303,9 @@ i386_assemble (char *line)
> >>           && i.imm_operands && i.operands > i.imm_operands))
> >>        swap_2_operands (0, 1);
> >>
> >> -  if (i.imm_operands)
> >> +  /* All legitimate immediates are placed first now.  Others, if any, will be
> >> +     rejected by match_template() anyway.  */
> >> +  if (operand_type_check (i.types[0], imm))
> >>      {
> >>        /* For USER_MSR and MSR_IMM instructions, imm32 stands for the name of a
> >>          model specific register (MSR). That's an unsigned quantity, whereas all
> >> @@ -7313,10 +7315,7 @@ i386_assemble (char *line)
> >>        if (is_cpu(current_templates.start, CpuUSER_MSR)
> >>           || t->mnem_off == MN_rdmsr
> >>           || t->mnem_off == MN_wrmsrns)
> >> -       {
> >> -         for (j = 0; j < i.imm_operands; j++)
> >> -           i.types[j] = smallest_imm_type (i.op[j].imms->X_add_number);
> >> -       }
> >> +       i.types[0] = smallest_imm_type (i.op[0].imms->X_add_number);
> >>        else
> >>         optimize_imm ();
> >>      }
> >> @@ -8520,7 +8519,7 @@ optimize_imm (void)
> >>                && current_templates.start->mnem_off != MN_jmpabs))
> >>      guess_suffix = LONG_MNEM_SUFFIX;
> >>
> >> -  for (op = i.operands; --op >= 0;)
> >> +  for (op = i.imm_operands; --op >= 0;)
> >>      if (operand_type_check (i.types[op], imm))
> >>        {
> >>         switch (i.op[op].imms->X_op)
> >> --- a/gas/testsuite/gas/i386/x86-64-msr_imm-inval.l
> >> +++ b/gas/testsuite/gas/i386/x86-64-msr_imm-inval.l
> >> @@ -1,5 +1,7 @@
> >>  .* Assembler messages:
> >>  .*:5: Error: operand type mismatch for `rdmsr'
> >>  .*:6: Error: operand type mismatch for `rdmsr'
> >> -.*:7: Error: operand type mismatch for `wrmsrns'
> >> +.*:7: Error: operand type mismatch for `rdmsr'
> >>  .*:8: Error: operand type mismatch for `wrmsrns'
> >> +.*:9: Error: operand type mismatch for `wrmsrns'
> >> +.*:10: Error: operand type mismatch for `wrmsrns'
> >> --- a/gas/testsuite/gas/i386/x86-64-msr_imm-inval.s
> >> +++ b/gas/testsuite/gas/i386/x86-64-msr_imm-inval.s
> >> @@ -4,5 +4,7 @@
> >>  _start:
> >>         rdmsr  $5151515151515151, %r12
> >>         rdmsr  $-515151, %r12
> >> +       rdmsr  (%rax), $0
> >>         wrmsrns  %r12, $5151515151515151
> >>         wrmsrns  %r12, $-515151
> >> +       wrmsrns  $0, (%rax)
> >
> > PR gas/34028 test crashed in 32-bit mode.   Please add a 32-bit test
> > to verify that it no longer happens.
>
> As indicated before, imo that's excessive. The code path is the same, so
> testing where the insn forms can sensibly be used is pretty much sufficient.
>

Such test costs very little.  It is an insurance policy.   Also we should add

wrmsrns $y,%cs:

Otherwise, we have no way to verify the bug is really fixed.

-- 
H.J.