[1/2] RISC-V: Make .insn tests stricter

  From: Tsukasa OI <research_trasio@irq.a4lg.com>

To make sure that all instruction bits are dumped through ".byte", this
commit makes matching patterns stricter (to cover all instruction bits).

gas/ChangeLog:

	* testsuite/gas/riscv/insn.d: Make pattern stricter.
	* testsuite/gas/riscv/insn-na.d: Likewise.
---
 gas/testsuite/gas/riscv/insn-na.d | 20 ++++++++++----------
 gas/testsuite/gas/riscv/insn.d    | 10 +++++-----
 2 files changed, 15 insertions(+), 15 deletions(-)
  

On 19.11.2022 08:10, Tsukasa OI wrote:
> To make sure that all instruction bits are dumped through ".byte", this
> commit makes matching patterns stricter (to cover all instruction bits).

Hmm, it was deliberate to omit the .<n>byte part of the disassembly,
very specifically because of the inconsistency of using <n> only in
some cases. Personally I would agree with such tightening of the
expectations only if the disassembler was first improved. But of
course it's the maintainers judgement ...

Jan

> --- a/gas/testsuite/gas/riscv/insn-na.d
> +++ b/gas/testsuite/gas/riscv/insn-na.d
> @@ -61,15 +61,15 @@ Disassembly of section .text:
>  [^:]+:[ 	]+022180d7[ 	]+vadd\.vv[ 	]+v1,v2,v3
>  [^:]+:[ 	]+0001[ 	]+c\.addi[ 	]+zero,0
>  [^:]+:[ 	]+00000013[ 	]+addi[ 	]+zero,zero,0
> -[^:]+:[ 	]+001f 0000 0000[ 	].*
> -[^:]+:[ 	]+0000003f 00000000[ 	].*
> -[^:]+:[ 	]+007f 0000 0000 0000 0000[ 	]+[._a-z].*
> -[^:]+:[ 	]+0000107f 00000000 00000000[ 	]+[._a-z].*
> -[^:]+:[ 	]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ 	]+[._a-z].*
> +[^:]+:[ 	]+001f 0000 0000[ 	]+\.byte[ 	]+0x1f, 0x00, 0x00, 0x00, 0x00, 0x00
> +[^:]+:[ 	]+0000003f 00000000[ 	]+\.8byte[ 	]+0x3f
> +[^:]+:[ 	]+007f 0000 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +[^:]+:[ 	]+0000107f 00000000 00000000[ 	]+\.byte[ 	]+0x7f, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +[^:]+:[ 	]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>  [^:]+:[ 	]+0001[ 	]+c\.addi[ 	]+zero,0
>  [^:]+:[ 	]+00000013[ 	]+addi[ 	]+zero,zero,0
> -[^:]+:[ 	]+001f 0000 0000[ 	].*
> -[^:]+:[ 	]+0000003f 00000000[ 	].*
> -[^:]+:[ 	]+007f 0000 0000 0000 0000[ 	]+[._a-z].*
> -[^:]+:[ 	]+0000107f 00000000 00000000[ 	]+[._a-z].*
> -[^:]+:[ 	]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ 	]+[._a-z].*
> +[^:]+:[ 	]+001f 0000 0000[ 	]+\.byte[ 	]+0x1f, 0x00, 0x00, 0x00, 0x00, 0x00
> +[^:]+:[ 	]+0000003f 00000000[ 	]+\.8byte[ 	]+0x3f
> +[^:]+:[ 	]+007f 0000 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +[^:]+:[ 	]+0000107f 00000000 00000000[ 	]+\.byte[ 	]+0x7f, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +[^:]+:[ 	]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> diff --git a/gas/testsuite/gas/riscv/insn.d b/gas/testsuite/gas/riscv/insn.d
> index 2e5d35b39702..cf84f177af39 100644
> --- a/gas/testsuite/gas/riscv/insn.d
> +++ b/gas/testsuite/gas/riscv/insn.d
> @@ -83,12 +83,12 @@ Disassembly of section .text:
>  [^:]+:[ 	]+0000 0000 0000 ?
>  [^:]+:[ 	]+0001[ 	]+nop
>  [^:]+:[ 	]+00000013[ 	]+nop
> -[^:]+:[ 	]+001f 0000 0000[ 	].*
> -[^:]+:[ 	]+0000003f 00000000[ 	].*
> -[^:]+:[ 	]+007f 0000 0000 0000[ 	]+[._a-z].*
> +[^:]+:[ 	]+001f 0000 0000[ 	]+\.byte[ 	]+0x1f, 0x00, 0x00, 0x00, 0x00, 0x00
> +[^:]+:[ 	]+0000003f 00000000[ 	]+\.8byte[ 	]+0x3f
> +[^:]+:[ 	]+007f 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>  [^:]+:[ 	]+0000 ?
> -[^:]+:[ 	]+0000107f 00000000[ 	]+[._a-z].*
> +[^:]+:[ 	]+0000107f 00000000[ 	]+\.byte[ 	]+0x7f, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>  [^:]+:[ 	]+00000000 ?
> -[^:]+:[ 	]+607f 0000 0000 0000[ 	]+[._a-z].*
> +[^:]+:[ 	]+607f 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>  [^:]+:[ 	]+0000 0000 0000 0000 ?
>  [^:]+:[ 	]+0000 0000 0000 ?
  

On 2022/11/21 16:32, Jan Beulich wrote:
> On 19.11.2022 08:10, Tsukasa OI wrote:
>> To make sure that all instruction bits are dumped through ".byte", this
>> commit makes matching patterns stricter (to cover all instruction bits).
> 
> Hmm, it was deliberate to omit the .<n>byte part of the disassembly,
> very specifically because of the inconsistency of using <n> only in
> some cases. Personally I would agree with such tightening of the
> expectations only if the disassembler was first improved. But of
> course it's the maintainers judgement ...
> 
> Jan

Hi,

I can agree most of your opinion except... even if we fix this
inconsistency later (I won't, though), I think we can change the tests
once we have changed the disassembler.  So I don't find any problems
making tests tighter.

Nelson assigned you as the person who makes the final judgement and I
want to hear your thoughts/decision about PATCH 1/2.

p.s.
I made a small change to PATCH 2/2 and I'll reply your feedback to PATCH
2/2 after PATCH v2 is submitted (PATCH v2 1/2 is unchanged from v1).

Thanks,
Tsukasa

> 
>> --- a/gas/testsuite/gas/riscv/insn-na.d
>> +++ b/gas/testsuite/gas/riscv/insn-na.d
>> @@ -61,15 +61,15 @@ Disassembly of section .text:
>>  [^:]+:[ 	]+022180d7[ 	]+vadd\.vv[ 	]+v1,v2,v3
>>  [^:]+:[ 	]+0001[ 	]+c\.addi[ 	]+zero,0
>>  [^:]+:[ 	]+00000013[ 	]+addi[ 	]+zero,zero,0
>> -[^:]+:[ 	]+001f 0000 0000[ 	].*
>> -[^:]+:[ 	]+0000003f 00000000[ 	].*
>> -[^:]+:[ 	]+007f 0000 0000 0000 0000[ 	]+[._a-z].*
>> -[^:]+:[ 	]+0000107f 00000000 00000000[ 	]+[._a-z].*
>> -[^:]+:[ 	]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ 	]+[._a-z].*
>> +[^:]+:[ 	]+001f 0000 0000[ 	]+\.byte[ 	]+0x1f, 0x00, 0x00, 0x00, 0x00, 0x00
>> +[^:]+:[ 	]+0000003f 00000000[ 	]+\.8byte[ 	]+0x3f
>> +[^:]+:[ 	]+007f 0000 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> +[^:]+:[ 	]+0000107f 00000000 00000000[ 	]+\.byte[ 	]+0x7f, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> +[^:]+:[ 	]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>  [^:]+:[ 	]+0001[ 	]+c\.addi[ 	]+zero,0
>>  [^:]+:[ 	]+00000013[ 	]+addi[ 	]+zero,zero,0
>> -[^:]+:[ 	]+001f 0000 0000[ 	].*
>> -[^:]+:[ 	]+0000003f 00000000[ 	].*
>> -[^:]+:[ 	]+007f 0000 0000 0000 0000[ 	]+[._a-z].*
>> -[^:]+:[ 	]+0000107f 00000000 00000000[ 	]+[._a-z].*
>> -[^:]+:[ 	]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ 	]+[._a-z].*
>> +[^:]+:[ 	]+001f 0000 0000[ 	]+\.byte[ 	]+0x1f, 0x00, 0x00, 0x00, 0x00, 0x00
>> +[^:]+:[ 	]+0000003f 00000000[ 	]+\.8byte[ 	]+0x3f
>> +[^:]+:[ 	]+007f 0000 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> +[^:]+:[ 	]+0000107f 00000000 00000000[ 	]+\.byte[ 	]+0x7f, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> +[^:]+:[ 	]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> diff --git a/gas/testsuite/gas/riscv/insn.d b/gas/testsuite/gas/riscv/insn.d
>> index 2e5d35b39702..cf84f177af39 100644
>> --- a/gas/testsuite/gas/riscv/insn.d
>> +++ b/gas/testsuite/gas/riscv/insn.d
>> @@ -83,12 +83,12 @@ Disassembly of section .text:
>>  [^:]+:[ 	]+0000 0000 0000 ?
>>  [^:]+:[ 	]+0001[ 	]+nop
>>  [^:]+:[ 	]+00000013[ 	]+nop
>> -[^:]+:[ 	]+001f 0000 0000[ 	].*
>> -[^:]+:[ 	]+0000003f 00000000[ 	].*
>> -[^:]+:[ 	]+007f 0000 0000 0000[ 	]+[._a-z].*
>> +[^:]+:[ 	]+001f 0000 0000[ 	]+\.byte[ 	]+0x1f, 0x00, 0x00, 0x00, 0x00, 0x00
>> +[^:]+:[ 	]+0000003f 00000000[ 	]+\.8byte[ 	]+0x3f
>> +[^:]+:[ 	]+007f 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>  [^:]+:[ 	]+0000 ?
>> -[^:]+:[ 	]+0000107f 00000000[ 	]+[._a-z].*
>> +[^:]+:[ 	]+0000107f 00000000[ 	]+\.byte[ 	]+0x7f, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>  [^:]+:[ 	]+00000000 ?
>> -[^:]+:[ 	]+607f 0000 0000 0000[ 	]+[._a-z].*
>> +[^:]+:[ 	]+607f 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>  [^:]+:[ 	]+0000 0000 0000 0000 ?
>>  [^:]+:[ 	]+0000 0000 0000 ?
>
  

On 23.11.2022 09:20, Tsukasa OI wrote:
> On 2022/11/21 16:32, Jan Beulich wrote:
>> On 19.11.2022 08:10, Tsukasa OI wrote:
>>> To make sure that all instruction bits are dumped through ".byte", this
>>> commit makes matching patterns stricter (to cover all instruction bits).
>>
>> Hmm, it was deliberate to omit the .<n>byte part of the disassembly,
>> very specifically because of the inconsistency of using <n> only in
>> some cases. Personally I would agree with such tightening of the
>> expectations only if the disassembler was first improved. But of
>> course it's the maintainers judgement ...
> 
> I can agree most of your opinion except... even if we fix this
> inconsistency later (I won't, though), I think we can change the tests
> once we have changed the disassembler.  So I don't find any problems
> making tests tighter.
> 
> Nelson assigned you as the person who makes the final judgement and I
> want to hear your thoughts/decision about PATCH 1/2.

Hmm, I have to admit that I found Nelson's reply a little odd. For
arch-specific code I think the arch-specific maintainers are in far
better position to approve of patches, potentially also knowing
what longer term plans there are, and potentially also knowing of
reasons for the present behavior which I may be unaware of.
Therefore I'd like to make clear that my response was an opinion,
but not really meant to be an objection. Nevertheless some further
background (largely from experience with working on other
architectures, mostly x86): Once putting in specific expectations
for a particular test, the barrier to change those expectations
should be higher. So generally I would always advocate for putting
in as relaxed expectations as possible (but of course as strict as
necessary to fulfill the purpose of a test) when a particular
aspect of behavior may not be meant to remain that way long term.

Hence in the case here my intention with the original expectations
in the test in question was for them to remain valid once the
.<n>byte behavior was sanitized (which sooner or later I may get
to). Therefore I'd prefer (but not insist) that this remains to be
the case even after your adjustments. And I'd also prefer (but not
insist) that you, Nelson, at least voice an opinion, if not take on
again the role of being the one to eventually approve of the patch.

Jan