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

[Jan Beulich]

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.

Jan