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

[H.J. Lu]

On Thu, Apr 9, 2026 at 4:45 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.04.2026 10:41, H.J. Lu wrote:
> > 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.
>
> Well, I've voiced my opinion.
>
> >   Also we should add
> >
> > wrmsrns $y,%cs:
> >
> > Otherwise, we have no way to verify the bug is really fixed.
>
> No, that'll be rejected earlier by patch 2. Adding such a test here just
> to remove it again in patch 2 is useless.
>

What does the fixed assembler do on

wrmsrns $y,%cs:


-- 
H.J.