testsuites: Fix RISC-V tests with 'nop' instruction.

Message ID 20250910120916.1103023-1-jiawei@iscas.ac.cn
State New
Headers
Series testsuites: Fix RISC-V tests with 'nop' instruction. |

Commit Message

Jiawei Sept. 10, 2025, 12:09 p.m. UTC
  This patch forces tests containing 'nop'(0x13) instructions to specify the
as parameter. The default as parameter is rv64gc will generation compressed
instruction 'c.nop'(0x1) and broken the result as expected. Using 'rv64i'
without 'C' extension to avoid this.

gas/ChangeLog:

	* testsuite/gas/riscv/dis-partial-insn-byte.d: Limit as parameter.
	* testsuite/gas/riscv/dis-partial-insn-short.d: Ditto.
	* testsuite/gas/riscv/dis-partial-insn-word.d: Ditto.
	* testsuite/gas/riscv/no-relax-align.d: Ditto.
	* testsuite/gas/riscv/odd-padding.d: Ditto.
	* testsuite/gas/riscv/t_insns.d: Ditto.
	* testsuite/gas/riscv/tlsdesc.d: Ditto.

---
 gas/testsuite/gas/riscv/dis-partial-insn-byte.d  | 2 +-
 gas/testsuite/gas/riscv/dis-partial-insn-short.d | 2 +-
 gas/testsuite/gas/riscv/dis-partial-insn-word.d  | 2 +-
 gas/testsuite/gas/riscv/no-relax-align.d         | 2 +-
 gas/testsuite/gas/riscv/odd-padding.d            | 4 ++--
 gas/testsuite/gas/riscv/t_insns.d                | 2 +-
 gas/testsuite/gas/riscv/tlsdesc.d                | 1 +
 7 files changed, 8 insertions(+), 7 deletions(-)
  

Comments

Jan Beulich Sept. 10, 2025, 12:59 p.m. UTC | #1
On 10.09.2025 14:09, Jiawei wrote:
> This patch forces tests containing 'nop'(0x13) instructions to specify the
> as parameter. The default as parameter is rv64gc will generation compressed
> instruction 'c.nop'(0x1) and broken the result as expected.

I don't follow this explanation. There's no breakage right now, and you also
don't alter any of the test expectations. Was the 2nd sentence maybe intended
to start "If the default gas setting is rv64gc ..."? (I.e. if an override of
DEFAULT_RISCV_ARCH_WITH_EXT was in effect.) If so, is NOP really the only thing
we would need to be concerned of?

Jan
  
Nelson Chu Sept. 10, 2025, 2:44 p.m. UTC | #2
On Wed, Sep 10, 2025 at 8:59 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 10.09.2025 14:09, Jiawei wrote:
> > This patch forces tests containing 'nop'(0x13) instructions to specify
> the
> > as parameter. The default as parameter is rv64gc will generation
> compressed
> > instruction 'c.nop'(0x1) and broken the result as expected.
>
> I don't follow this explanation. There's no breakage right now, and you
> also
> don't alter any of the test expectations. Was the 2nd sentence maybe
> intended
> to start "If the default gas setting is rv64gc ..."? (I.e. if an override
> of
> DEFAULT_RISCV_ARCH_WITH_EXT was in effect.)


Yeah I guess that's the problem, but the weird thing is I suppose the
regressions of riscv-gnu-toolchain should test it, but I don't get any
failures...  I think it's fine to make test cases passed for specific
DEFAULT_RISCV_ARCH_WITH_EXT build, but force them to rv64i means we won't
test rv32 for those test cases, which I think original we also wants rv32
were tested.  Does the .option norvc directives work for those testcase?
It seems only the difference between nop and c.nop, and we don't want c.nop
breaks the test cases.

For the dis-partial-insn-byte, rv64i generates nop, and rv64gc generates
c.nop, which breaks the "objdump --start-address 0 --stop-address 1 -d",
the former shows .byte 0x13, but the latter shows .byte 0x01.


> If so, is NOP really the only thing
> we would need to be concerned of?


Well...  Yeah probably more need to be concerned.  Hi Jiawei, if you have
time, could you please help to check out why riscv-gnu-toolchain
regressions don't cause these problems? I remember that when muti-lib is
not built, the toolchains should be built by --with-arch and
--with-isa-spec configure options.  Thanks :-)

Nelson
  
Peter Bergner Sept. 10, 2025, 3:39 p.m. UTC | #3
On 9/10/25 9:44 AM, Nelson Chu wrote:
> Does the .option norvc directives work for those testcase?

That should work.



>>     If so, is NOP really the only thing
>>     we would need to be concerned of?
> 
> 
> Well...  Yeah probably more need to be concerned.

I had to use ".option norvc" for the libffi risc-v static trampoline
support [1] so that the "jr t2" in the commit wouldn't be automatically
converted to c.jr, so yeah we have to be concerned by more than just
nop.

I'll also note gas and LLVM's assembler don't always agree when to convert
an instruction to the compressed form when it is possible.  Maybe different
defaults within the assemblers?


Peter


[1] https://github.com/libffi/libffi/pull/933/commits/0a0897a3ff6deb5fd2afc1b995a7aa515c60dd66
  
Jiawei Sept. 11, 2025, 1:44 a.m. UTC | #4
Hi Jan,

Actually, the regression test will fail on these cases when you update 
your riscv-gnu-toolchain repo into the latest 
version(cfe9d78f8dc8dffc8ce371c46978b257ef2a5c35).

I think you are right, it's not a problem with NOP, it's just a problem 
for no-compressed instruction test now be compressed.


BR,

Jiawei


In 2025/9/10 20:59, Jan Beulich wrote:
> On 10.09.2025 14:09, Jiawei wrote:
>> This patch forces tests containing 'nop'(0x13) instructions to specify the
>> as parameter. The default as parameter is rv64gc will generation compressed
>> instruction 'c.nop'(0x1) and broken the result as expected.
> I don't follow this explanation. There's no breakage right now, and you also
> don't alter any of the test expectations. Was the 2nd sentence maybe intended
> to start "If the default gas setting is rv64gc ..."? (I.e. if an override of
> DEFAULT_RISCV_ARCH_WITH_EXT was in effect.) If so, is NOP really the only thing
> we would need to be concerned of?
>
> Jan
  
Jiawei Sept. 11, 2025, 1:52 a.m. UTC | #5
In 2025/9/10 22:44, Nelson Chu wrote:
>
>
> On Wed, Sep 10, 2025 at 8:59 PM Jan Beulich <jbeulich@suse.com> wrote:
>
>     On 10.09.2025 14:09, Jiawei wrote:
>     > This patch forces tests containing 'nop'(0x13) instructions to
>     specify the
>     > as parameter. The default as parameter is rv64gc will generation
>     compressed
>     > instruction 'c.nop'(0x1) and broken the result as expected.
>
>     I don't follow this explanation. There's no breakage right now,
>     and you also
>     don't alter any of the test expectations. Was the 2nd sentence
>     maybe intended
>     to start "If the default gas setting is rv64gc ..."? (I.e. if an
>     override of
>     DEFAULT_RISCV_ARCH_WITH_EXT was in effect.) 
>
>
> Yeah I guess that's the problem, but the weird thing is I suppose the 
> regressions of riscv-gnu-toolchain should test it, but I don't get any 
> failures...  I think it's fine to make test cases passed for specific 
> DEFAULT_RISCV_ARCH_WITH_EXT build, but force them to rv64i means we 
> won't test rv32 for those test cases, which I think original we also 
> wants rv32 were tested.  Does the .option norvc directives work for 
> those testcase?  It seems only the difference between nop and c.nop, 
> and we don't want c.nop breaks the test cases.
>
> For the dis-partial-insn-byte, rv64i generates nop, and rv64gc 
> generates c.nop, which breaks the "objdump --start-address 0 
> --stop-address 1 -d", the former shows .byte 0x13, but the latter 
> shows .byte 0x01.


Yes, that's the problem, before the riscv-gnu-toolchain update, 
--with-arch option will not be passed into binutils part, but currently 
it does,

https://github.com/riscv-collab/riscv-gnu-toolchain/commit/1faa77ffe8daec705aa1ef266e944c554d838881

so some test will show compressed result and come into fail.

Thanks for your hint, the current revision is indeed one-sided. I think 
maybe add .option norvc in assemble file is a better choice.


BR,

Jiawei


>     If so, is NOP really the only thing
>     we would need to be concerned of?
>
>
> Well...  Yeah probably more need to be concerned.  Hi Jiawei, if you 
> have time, could you please help to check out why riscv-gnu-toolchain 
> regressions don't cause these problems? I remember that when muti-lib 
> is not built, the toolchains should be built by --with-arch and 
> --with-isa-spec configure options.  Thanks :-)
>
> Nelson
  
Jiawei Sept. 11, 2025, 1:53 a.m. UTC | #6
In 2025/9/10 23:39, Peter Bergner wrote:
> On 9/10/25 9:44 AM, Nelson Chu wrote:
>> Does the .option norvc directives work for those testcase?
> That should work.
>
>
>
>>>      If so, is NOP really the only thing
>>>      we would need to be concerned of?
>>
>> Well...  Yeah probably more need to be concerned.
> I had to use ".option norvc" for the libffi risc-v static trampoline
> support [1] so that the "jr t2" in the commit wouldn't be automatically
> converted to c.jr, so yeah we have to be concerned by more than just
> nop.

Thanks for your suggestions, I will try this way.

BR,

Jiawei

>
> I'll also note gas and LLVM's assembler don't always agree when to convert
> an instruction to the compressed form when it is possible.  Maybe different
> defaults within the assemblers?
>
>
> Peter
>
>
> [1] https://github.com/libffi/libffi/pull/933/commits/0a0897a3ff6deb5fd2afc1b995a7aa515c60dd66
  
Nelson Chu Sept. 11, 2025, 2:27 a.m. UTC | #7
On Thu, Sep 11, 2025 at 9:52 AM Jiawei <jiawei@iscas.ac.cn> wrote:

> In 2025/9/10 22:44, Nelson Chu wrote:
>
> On Wed, Sep 10, 2025 at 8:59 PM Jan Beulich <jbeulich@suse.com> wrote:
>
>> On 10.09.2025 14:09, Jiawei wrote:
>> > This patch forces tests containing 'nop'(0x13) instructions to specify
>> the
>> > as parameter. The default as parameter is rv64gc will generation
>> compressed
>> > instruction 'c.nop'(0x1) and broken the result as expected.
>>
>> I don't follow this explanation. There's no breakage right now, and you
>> also
>> don't alter any of the test expectations. Was the 2nd sentence maybe
>> intended
>> to start "If the default gas setting is rv64gc ..."? (I.e. if an override
>> of
>> DEFAULT_RISCV_ARCH_WITH_EXT was in effect.)
>
>
> Yeah I guess that's the problem, but the weird thing is I suppose the
> regressions of riscv-gnu-toolchain should test it, but I don't get any
> failures...  I think it's fine to make test cases passed for specific
> DEFAULT_RISCV_ARCH_WITH_EXT build, but force them to rv64i means we won't
> test rv32 for those test cases, which I think original we also wants rv32
> were tested.  Does the .option norvc directives work for those testcase?
> It seems only the difference between nop and c.nop, and we don't want c.nop
> breaks the test cases.
>
> For the dis-partial-insn-byte, rv64i generates nop, and rv64gc generates
> c.nop, which breaks the "objdump --start-address 0 --stop-address 1 -d",
> the former shows .byte 0x13, but the latter shows .byte 0x01.
>
> Yes, that's the problem, before the riscv-gnu-toolchain update,
> --with-arch option will not be passed into binutils part, but currently it
> does,
>
>
> https://github.com/riscv-collab/riscv-gnu-toolchain/commit/1faa77ffe8daec705aa1ef266e944c554d838881
>
> so some test will show compressed result and come into fail.
>

Thanks!  So we should fix these failures in binutils test cases.


> Thanks for your hint, the current revision is indeed one-sided. I think
> maybe add .option norvc in assemble file is a better choice.
>
Cool.  As the above discussions with Jan and Peter, I think it worth to try
.option norvc first.

Nelson
  
Nelson Chu Sept. 11, 2025, 2:33 a.m. UTC | #8
On Wed, Sep 10, 2025 at 11:39 PM Peter Bergner <bergner@tenstorrent.com>
wrote:

> On 9/10/25 9:44 AM, Nelson Chu wrote:
> > Does the .option norvc directives work for those testcase?
>
> That should work.
>
>
>
> >>     If so, is NOP really the only thing
> >>     we would need to be concerned of?
> >
> >
> > Well...  Yeah probably more need to be concerned.
>
> I had to use ".option norvc" for the libffi risc-v static trampoline
> support [1] so that the "jr t2" in the commit wouldn't be automatically
> converted to c.jr, so yeah we have to be concerned by more than just
> nop.
>
> I'll also note gas and LLVM's assembler don't always agree when to convert
> an instruction to the compressed form when it is possible.  Maybe different
> defaults within the assemblers?
>

I remember there is a new directive .option exact/noexact under community
discussion, it may be what you want ;)

Nelson
  

Patch

diff --git a/gas/testsuite/gas/riscv/dis-partial-insn-byte.d b/gas/testsuite/gas/riscv/dis-partial-insn-byte.d
index f8c149ca881..69252a671c6 100644
--- a/gas/testsuite/gas/riscv/dis-partial-insn-byte.d
+++ b/gas/testsuite/gas/riscv/dis-partial-insn-byte.d
@@ -1,4 +1,4 @@ 
-#as:
+#as: -march=rv64i
 #source: dis-partial-insn.s
 #objdump: --start-address 0 --stop-address 1 -d
 
diff --git a/gas/testsuite/gas/riscv/dis-partial-insn-short.d b/gas/testsuite/gas/riscv/dis-partial-insn-short.d
index 4a7d6d98b82..cddf235771b 100644
--- a/gas/testsuite/gas/riscv/dis-partial-insn-short.d
+++ b/gas/testsuite/gas/riscv/dis-partial-insn-short.d
@@ -1,4 +1,4 @@ 
-#as:
+#as: -march=rv64i
 #source: dis-partial-insn.s
 #objdump: --start-address 0 --stop-address 2 -d
 
diff --git a/gas/testsuite/gas/riscv/dis-partial-insn-word.d b/gas/testsuite/gas/riscv/dis-partial-insn-word.d
index af30c5be3d5..84d1652f278 100644
--- a/gas/testsuite/gas/riscv/dis-partial-insn-word.d
+++ b/gas/testsuite/gas/riscv/dis-partial-insn-word.d
@@ -1,4 +1,4 @@ 
-#as:
+#as: -march=rv64i
 #source: dis-partial-insn.s
 #objdump: --start-address 0 --stop-address 3 -d
 
diff --git a/gas/testsuite/gas/riscv/no-relax-align.d b/gas/testsuite/gas/riscv/no-relax-align.d
index ced07d3c830..431a962d23f 100644
--- a/gas/testsuite/gas/riscv/no-relax-align.d
+++ b/gas/testsuite/gas/riscv/no-relax-align.d
@@ -1,4 +1,4 @@ 
-#as:
+#as: -march=rv64i
 #objdump: -dr
 
 .*:[ 	]+file format .*
diff --git a/gas/testsuite/gas/riscv/odd-padding.d b/gas/testsuite/gas/riscv/odd-padding.d
index b445a74f346..6e712b5cf65 100644
--- a/gas/testsuite/gas/riscv/odd-padding.d
+++ b/gas/testsuite/gas/riscv/odd-padding.d
@@ -1,5 +1,5 @@ 
-#as: -mrelax
-#as: -mno-relax
+#as: -march=rv64i -mrelax
+#as: -march=rv64i -mno-relax
 #objdump: -d
 
 .*:[ 	]+file format .*
diff --git a/gas/testsuite/gas/riscv/t_insns.d b/gas/testsuite/gas/riscv/t_insns.d
index 720f0db2930..0988a3d8f64 100644
--- a/gas/testsuite/gas/riscv/t_insns.d
+++ b/gas/testsuite/gas/riscv/t_insns.d
@@ -1,4 +1,4 @@ 
-#as:
+#as: -march=rv64i
 #objdump: -dr
 
 .*:[ 	]+file format .*
diff --git a/gas/testsuite/gas/riscv/tlsdesc.d b/gas/testsuite/gas/riscv/tlsdesc.d
index 5cd26ac3280..2ff630572f9 100644
--- a/gas/testsuite/gas/riscv/tlsdesc.d
+++ b/gas/testsuite/gas/riscv/tlsdesc.d
@@ -1,3 +1,4 @@ 
+#as: -march=rv64i
 #source: tlsdesc.s
 #objdump: -dr