riscv: Remove ECALL tests from insttest

Information
Submitter:	Alec Roelke
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11808:1c38f8e541ba
---------------------------
riscv: Remove ECALL tests from insttest

The system calls tested in rv64i.cpp in RISC-V's insttest suite have
different behavior depending on the operating system and file system they
are run on. This patch ignores the output of those tests and only
ensures that the instructions in RV64I complete successfully.

[Change deletion of ECALL test to block comment.]
[Restore ECALL test but remove test output to test only for completion
without error.]
[Update patch description and again try to push EMPTY files for rv64i
tests.]

Testing Done

tests/test-progs/insttest/src/riscv/rv64i.cpp (Diff revision 1)

Can you block-comment instead of delete? I think we'll want to add this back in sometime in the relatively near future (e.g., when we have functional-only tests).

Jason Lowe-Power Jan. 18, 2017, 8:59 a.m. (Jan. 18, 2017, 8:59 a.m.)
```
Thanks!
```

Description:

~		Changeset 11796:d66318827fe0
	~	Changeset 11796:aaf914ea5a83

		riscv: Remove ECALL tests from insttest

		The system calls tested in rv64i.cpp in RISC-V's insttest suite have
		different behavior depending on the operating system and file system they
		are run on. This patch removes those tests from the regression.
	+
	+	[Change deletion of ECALL test to block comment.]

Diff:

Revision 2 (+1562 -1616)

Show changes

	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/config.json
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/config.ini
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/simout
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/stats.txt
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/config.json
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/config.ini
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/simout
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/stats.txt
	9 more

Another option is to remove the stats.txt from the stats results directory. This will turn the test into a functional-only test.

In fact that may be there better option here as we also don't have to worry about distributing a new binary.

Andreas Sandberg Jan. 23, 2017, 10:18 a.m. (Jan. 23, 2017, 10:18 a.m.)

If you don't need to diff anything, you can just remove all of the files in the reference directory. In this case, you need to make sure that the directory is created by the version control system by adding an empty file called EMPTY (this is ignored by the test framework). These functional tests succeed as long as gem5 completes with exit code 0 and fail otherwise (e.g., if gem5 aborts). We already use this for some of ARM tests.

Alec Roelke Jan. 23, 2017, 11:39 a.m. (Jan. 23, 2017, 11:39 a.m.)

I ended up removing all of the output results from the tests but restoring the ECALL tests since the output may also be file system dependent (it calls stat and fstat and outputs the results to stdout).

Description:

~		Changeset 11796:aaf914ea5a83
	~	Changeset 11796:3cec8d75cd13

		riscv: Remove ECALL tests from insttest

		The system calls tested in rv64i.cpp in RISC-V's insttest suite have
		different behavior depending on the operating system and file system they
		are run on. This patch removes those tests from the regression.

~		[Change deletion of ECALL test to block comment.]
	~	[Change deletion of ECALL test to block comment.]
	+	[Restore ECALL test but remove test output to test only for completion
	+	without error.]

Diff:

Revision 3 (+2 -9178)

Show changes

	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/config.json
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/config.ini
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/simerr
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/simout
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/stats.txt
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/config.json
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/config.ini
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/simerr
	13 more

Ship It!

Did you remove all files from the reference output directory? In that case, you'll need to add a an empty placeholder file called EMPTY in the reference directories. Some VCS systems (git being one of them) don't track empty directories, which would interfere with test discovery.

Alec Roelke Jan. 24, 2017, 8:34 a.m. (Jan. 24, 2017, 8:34 a.m.)

I did add them; I'm not sure why it didn't put them into the patch. Does the file need to contain text for it to work?

Andreas Sandberg Jan. 25, 2017, 3:28 a.m. (Jan. 25, 2017, 3:28 a.m.)

It isn't a problem for the VCS, but it might be for ReviewBoard. Don't worry about it if the files show up in the patch.

Description:

~		Changeset 11796:3cec8d75cd13
	~	Changeset 11796:1c03f7dcaa8c

		riscv: Remove ECALL tests from insttest

		The system calls tested in rv64i.cpp in RISC-V's insttest suite have
		different behavior depending on the operating system and file system they
		are run on. This patch removes those tests from the regression.

		[Change deletion of ECALL test to block comment.]
		[Restore ECALL test but remove test output to test only for completion
		without error.]

Diff:

Revision 4 (-9178)

Show changes

	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/config.json
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/config.ini
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/simerr
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/simout
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/stats.txt
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/config.json
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/config.ini
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/simerr
	12 more

Ship It!

Andreas Hansson Feb. 12, 2017, 9:42 a.m. (Feb. 12, 2017, 9:42 a.m.)

What was the verdict? Is it just a matter of an updated patch description/summary and then this should be pushed?

Andreas Sandberg Feb. 13, 2017, 5:19 a.m. (Feb. 13, 2017, 5:19 a.m.)

Yes, the description needs to be updated. I'm not sure what happened to the placeholder files in the test directories. Instead of the simulator output, there should be an empty file called EMPTY in the output directory to make sure that it is created by the VCS. It's not showing up in the raw diff, so I suspect it wasn't added to the commit. Another possibility is that RB/HG is silly, which wouldn't be the first time.

Alec Roelke Feb. 13, 2017, 7:03 a.m. (Feb. 13, 2017, 7:03 a.m.)

I just updated the patch with a new description. My local version of the patch correctly creates EMPTY files in the linux-rv64i output directory, but I still don't see them here.

Jason Lowe-Power Feb. 13, 2017, 7:20 a.m. (Feb. 13, 2017, 7:20 a.m.)

The EMPTY file thing is definitely and issue with reviewboard. You're not doing anything wrong, Alec. :)

We can coordinate off-list on me getting the right patch, and I'll push it.

Description:

~		Changeset 11796:1c03f7dcaa8c
	~	Changeset 11808:1c38f8e541ba

		riscv: Remove ECALL tests from insttest

		The system calls tested in rv64i.cpp in RISC-V's insttest suite have
		different behavior depending on the operating system and file system they
~		are run on. This patch removes those tests from the regression.
	~	are run on. This patch ignores the output of those tests and only
	+	ensures that the instructions in RV64I complete successfully.

		[Change deletion of ECALL test to block comment.]
		[Restore ECALL test but remove test output to test only for completion
~		without error.]
	~	without error.]
	+	[Update patch description and again try to push EMPTY files for rv64i
	+	tests.]

Diff:

Revision 5 (-9178)

Show changes

	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/config.json
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/config.ini
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/simerr
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/simout
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/minor-timing/stats.txt
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/config.json
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/config.ini
	tests/quick/se/02.insttest/ref/riscv/linux-rv64i/simple-atomic/simerr
	12 more

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Description:

Changeset 11796:d66318827fe0

Changeset 11796:aaf914ea5a83

Diff:

Description:

Changeset 11796:aaf914ea5a83

Changeset 11796:3cec8d75cd13

Diff:

Description:

Changeset 11796:3cec8d75cd13

Changeset 11796:1c03f7dcaa8c

Diff:

Description:

Changeset 11796:1c03f7dcaa8c

Changeset 11808:1c38f8e541ba

Diff:

Status: Closed (submitted)