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

[H.J. Lu]

On Thu, Apr 9, 2026 at 8:07 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.04.2026 13:24, H.J. Lu wrote:
> > 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:
>
> Issue an error ("bad memory operand `%s'") already while parsing the malformed
> operand, i.e. before actually trying to check any of its properties (like it
> being an immediate in the particular case here).
>
> Jan

Why not add a test to verify it?

-- 
H.J.