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

  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.
  

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.

> --- a/gas/testsuite/gas/i386/x86-64-user_msr-inval.l
> +++ b/gas/testsuite/gas/i386/x86-64-user_msr-inval.l
> @@ -3,7 +3,9 @@
>  .*:6: Error: operand type mismatch for `urdmsr'
>  .*:7: Error: operand type mismatch for `urdmsr'
>  .*:8: Error: operand type mismatch for `urdmsr'
> -.*:9: Error: operand type mismatch for `uwrmsr'
> +.*:9: Error: operand type mismatch for `urdmsr'
>  .*:10: Error: operand type mismatch for `uwrmsr'
>  .*:11: Error: operand type mismatch for `uwrmsr'
>  .*:12: Error: operand type mismatch for `uwrmsr'
> +.*:13: Error: operand type mismatch for `uwrmsr'
> +.*:14: Error: operand type mismatch for `uwrmsr'
> --- a/gas/testsuite/gas/i386/x86-64-user_msr-inval.s
> +++ b/gas/testsuite/gas/i386/x86-64-user_msr-inval.s
> @@ -6,7 +6,9 @@ _start:
>         urdmsr  $-32767, %r14
>         urdmsr  $-2147483648, %r14
>         urdmsr  $0x7fffffffffffffff, %r14
> +       urdmsr  (%rax), $0
>         uwrmsr  %r12, $-1
>         uwrmsr  %r12, $-32767
>         uwrmsr  %r12, $-2147483648
>         uwrmsr  %r12, $0x7fffffffffffffff
> +       uwrmsr  $0, (%rax)
>
  

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.

Jan
  

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.
  

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
  

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:
  

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
  

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?