[tools] Fix tools/test and not for user-mode emulation #212

iamlouk · 2025-02-25T17:00:03Z

Add preliminary support for user-mode emulation to the not tool
used in some fortran tests by adding a new mode where it spawns
the subprocess under the emulator. The original non-emulator-spawning
version is still needed for tests that use it to verify the output
using the host's diff. In the new version, the TEST_SUITE_RUN_UNDER
cmake option contents is prepended to the arguments then used to spawn
a subprocess.

Also disables the check_env test in tools/test in case of user-mode
emulation because it would run the host python interpreter in the
target emulator.

iamlouk · 2025-02-25T17:04:55Z

@tarunprabhu Could you maybe have a look at this? Without these changes, some tests in tools/test always fails when using -DTEST_SUITE_USER_MODE_EMULATION=On and e.g. -DTEST_SUITE_RUN_UNDER="qemu-<...> -L <...>"

tarunprabhu

Do we need a separate executable for this? Could we prepend the arguments when calling not instead? Or perhaps pass these on the command line? It seems as if users have to check TEST_SUITE_USER_MODE_EMULATION and TEST_SUITE_RUN_UNDER and use a different tool anyway, so user code does have to be changed.

tarunprabhu · 2025-02-25T19:02:43Z

tools/not.cpp

+  // In case of user-mode emulation, before spawning a new subprocess, the
+  // emulator needs to be preprended to the argv vector for the child.
+  // TEST_SUITE_RUN_UNDER will be defined to a comma-separated list of
+  // string litterals.


NIT:

Suggested change

// string litterals.

// string literals.

Sorry, fixed.

No need to apologize :-)

tarunprabhu · 2025-02-25T19:03:05Z

Fortran/gfortran/CMakeLists.txt

@@ -716,10 +716,18 @@ function(gfortran_add_execute_test expect_error main others fflags ldflags)
  gfortran_make_working_dir("${target}" working_dir)
  get_filename_component(working_dir_name "${working_dir}" NAME)

+  # When running in a emulator, use the version of 'not' that will


NIT:

Suggested change

# When running in a emulator, use the version of 'not' that will

# When running in an emulator, use the version of 'not' that will

Sorry, fixed.

tarunprabhu · 2025-02-25T19:04:49Z

Fortran/gfortran/CMakeLists.txt

+  # When running in a emulator, use the version of 'not' that will
+  # spawn the subprocess under that emulator as well.
+  if(TEST_SUITE_USER_MODE_EMULATION AND TEST_SUITE_RUN_UNDER)
+    set(NOT_HELPER "not-spawning-emulator")


Perhaps name this variable NOT_TOOL. It's not really a helper.

Suggested change

set(NOT_HELPER "not-spawning-emulator")

set(NOT_TOOL "not-spawning-emulator")

Thanks, I changed the name and moved it to the root CMakeLists.txt next to the fpcmp utility name assignment.

tarunprabhu · 2025-02-25T19:12:56Z

tools/test/CMakeLists.txt

-add_dependencies(abrt not)
-llvm_test_run(EXECUTABLE "$<TARGET_FILE:not>" "--crash" "$<TARGET_FILE:abrt>")
-llvm_add_test_for_target(abrt)
+# These test will always fail under user-mode emulation because 'not'


I am not sure what tests you are referring to here. If you mean the check_env test that you have removed, then perhaps this comment could be moved after the abort test instead.

With the NOT_TOOL assigned now and the check_env test being modified so that it now works under emulation, this is almost back to how it was before (just with ${NOT_TOOL instead of not).

tarunprabhu · 2025-02-25T19:14:00Z

tools/test/CMakeLists.txt

+# These test will always fail under user-mode emulation because 'not'
+# spawns a subprocess outside the emulator and the check_env test
+# runs the host python interpreter under the emulator for the target.
+if(NOT(TEST_SUITE_USER_MODE_EMULATION AND TEST_SUITE_RUN_UNDER))


The negation of the conditional is a bit hard to read. Could the branches be switched with a "positive" conditional instead?

Suggested change

if(NOT(TEST_SUITE_USER_MODE_EMULATION AND TEST_SUITE_RUN_UNDER))

if(TEST_SUITE_USER_MODE_EMULATION AND TEST_SUITE_RUN_UNDER)

With the NOT_TOOL assigned now and the check_env test being modified so that it now works under emulation, this is almost back to how it was before (just with ${NOT_TOOL instead of not).

tarunprabhu · 2025-02-25T19:20:24Z

tools/test/CMakeLists.txt

-llvm_add_test_for_target(abrt)
+# These test will always fail under user-mode emulation because 'not'
+# spawns a subprocess outside the emulator and the check_env test
+# runs the host python interpreter under the emulator for the target.


Does passing environment variables work with not-spawning-emulator? Is it only the test here that cannot be run? It would be good to have a test for this if possible.

Yes, the two user-mode emulator I know/use (mostly QEMU) do forward environment variables. I replaced the python script that sets the environment variable by a small C program inspired by not.cpp, and with that, the test now also works under emulation.

iamlouk · 2025-02-28T08:47:13Z

Do we need a separate executable for this? Could we prepend the arguments when calling not instead? Or perhaps pass these on the command line?

That was my first idea as well, and I can do that if its preferred. This just seamed easier to me, and other tools like fpcmp are also compiled twice (although the context/needs are different as it is compiled for different architectures).

Would a syntax similar to not [--crash] [--run-under <emulator> <emulator arg1> <emulator arg2 ...> --] <test binary> <test arg1> <test arg2 ...> be fine?

…ulation

tarunprabhu · 2025-02-28T18:31:10Z

Thanks for the changes.

That was my first idea as well, and I can do that if its preferred. This just seamed easier to me, and other tools like fpcmp are also compiled twice (although the context/needs are different as it is compiled for different architectures).

I think it might be better to stick with a single tool for this. I am guessing that fpcmp needs different CPU and GPU versions. That's not really the case here. This is really just adding a "wrapper" around the invocation of test_binary.

Would a syntax similar to not [--crash] [--run-under <emulator> <emulator arg1> <emulator arg2 ...> --] <test binary> <test arg1> <test arg2 ...> be fine?

I assume that <emulator> <emulator args> ... would be in quotes. Otherwise, how would we distinguish between the last <emulator arg> and the <test_binary>?

iamlouk · 2025-03-03T17:30:47Z

I just pushed a commit with the proposed solution so that you can have a look (single not binary, but with a flag to optionally spawn the subcommand under the emulator). The <emulator> <emulator args> ... is not in quotes, because that would require spiting arguments in the not tool. Instead, a -- (double dash) is used, a bit like commands like git checkout or so sometimes use to disambiguate in similar situations. I just tested this with and without user-mode emulation and with extra flags to the emulator.

tarunprabhu

Thank you for all the changes

tarunprabhu · 2025-03-07T13:13:46Z

tools/test/CMakeLists.txt

+if(TEST_SUITE_USER_MODE_EMULATION AND TEST_SUITE_RUN_UNDER)
+  # ${TEST_SUITE_RUN_UNDER} is needed here because test_not is not itself
+  # user-mode emulation aware, unlike the not tool.
+  llvm_test_run(EXECUTABLE ${Python_EXECUTABLE} "$<TARGET_FILE:test_not>" ${TEST_SUITE_RUN_UNDER} ${NOT_TOOL} "$<TARGET_FILE:check_env>")


Is ${Python_EXECUTABLE} required here?

Thanks for noticing, not it is not. I just tested in a clean build to make sure.

tarunprabhu · 2025-03-07T13:16:44Z

CMakeLists.txt

+# Shortcut for the path to the not executable
+set(NOT_TOOL "$<TARGET_FILE:not>")
+if (TEST_SUITE_USER_MODE_EMULATION AND TEST_SUITE_RUN_UNDER)
+  # Because 'not' spawns a subprocess for its argument, it needs to know about the


Nit:

The Github interface doesn't show columns, but does this comment fit within 80 characters? The rules for line width in cmake are not as strict as source files, but it would be good to stick to that width where we can.

I reformatted this and the following two lines to make it fit.

tarunprabhu · 2025-03-07T13:18:06Z

tools/not.cpp

+//     subcommand. If --crash is used as well, it has to come after the
+//     '--run-under <...> ...' arguments. The double-dash is used to separate
+//     emulator arguments from cmd arguments. This order makes it easier to
+//     use the not tool in the test-suite.


Could you add sentence clarifying what the return value will be in this case.

Just did that. This works as expected with QEMU and another emulator I use downstream, I will double check if this is also the behavior of other user-mode emulators such as Gem5 SE mode or RISC-V Spike.

tarunprabhu · 2025-03-07T13:20:24Z

tools/not.cpp

@@ -29,17 +35,52 @@
 #include <sys/wait.h>
 #endif

+#include <vector>


Please move this include with the other system includes at the top of this file

tarunprabhu · 2025-03-07T13:24:18Z

tools/not.cpp

+      ++argv;
+    }
+
+    // If present, the crash flag is between the emulator


Nit:

Does this comment need to be split across two lines? clang-format can be used to "fit" comments.

tarunprabhu

Thanks for all the changes and for seeing this through.

tarunprabhu · 2025-03-07T16:30:40Z

tools/not.cpp

+//     with the same exit status/signal than the emulated binary in case of
+//     a crash, and the --crash flag will behave the same way.


Nit

Suggested change

// with the same exit status/signal than the emulated binary in case of

// a crash, and the --crash flag will behave the same way.

// with the same exit status/signal as the emulated binary. The --crash flag

// will behave the same way.

iamlouk added 2 commits February 25, 2025 19:50

[tools] Disable tools/test under user-mode emulation

b74e2f1

[tools/not] Basic support for user-mode emulation

6474814

tarunprabhu reviewed Feb 25, 2025

View reviewed changes

iamlouk force-pushed the lk/tools-for-emulation branch from 01ab296 to 321227a Compare February 28, 2025 08:37

iamlouk added 2 commits February 28, 2025 10:58

Review comments

b6645ef

Replace test_not.cpp by test_not.py so that it can be tested under em…

321227a

…ulation

Add new not argument instead of different target

6156870

tarunprabhu reviewed Mar 7, 2025

View reviewed changes

tarunprabhu approved these changes Mar 7, 2025

View reviewed changes

Address review comments

9bc637e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tools] Fix tools/test and not for user-mode emulation #212

[tools] Fix tools/test and not for user-mode emulation #212

iamlouk commented Feb 25, 2025

iamlouk commented Feb 25, 2025

tarunprabhu left a comment

tarunprabhu Feb 25, 2025

iamlouk Feb 28, 2025

tarunprabhu Feb 28, 2025

tarunprabhu Feb 25, 2025

iamlouk Feb 28, 2025

tarunprabhu Feb 25, 2025

iamlouk Feb 28, 2025

tarunprabhu Feb 25, 2025

iamlouk Feb 28, 2025

tarunprabhu Feb 25, 2025

iamlouk Feb 28, 2025

tarunprabhu Feb 25, 2025

iamlouk Feb 28, 2025

iamlouk commented Feb 28, 2025

tarunprabhu commented Feb 28, 2025

iamlouk commented Mar 3, 2025

tarunprabhu left a comment

tarunprabhu Mar 7, 2025

iamlouk Mar 7, 2025

tarunprabhu Mar 7, 2025

iamlouk Mar 7, 2025

tarunprabhu Mar 7, 2025

iamlouk Mar 7, 2025

tarunprabhu Mar 7, 2025

tarunprabhu Mar 7, 2025

tarunprabhu left a comment

tarunprabhu Mar 7, 2025

	# When running in a emulator, use the version of 'not' that will
	# When running in an emulator, use the version of 'not' that will

	set(NOT_HELPER "not-spawning-emulator")
	set(NOT_TOOL "not-spawning-emulator")

	if(NOT(TEST_SUITE_USER_MODE_EMULATION AND TEST_SUITE_RUN_UNDER))
	if(TEST_SUITE_USER_MODE_EMULATION AND TEST_SUITE_RUN_UNDER)

		// with the same exit status/signal than the emulated binary in case of
		// a crash, and the --crash flag will behave the same way.

[tools] Fix tools/test and not for user-mode emulation #212

Are you sure you want to change the base?

[tools] Fix tools/test and not for user-mode emulation #212

Conversation

iamlouk commented Feb 25, 2025

iamlouk commented Feb 25, 2025

tarunprabhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamlouk commented Feb 28, 2025

tarunprabhu commented Feb 28, 2025

iamlouk commented Mar 3, 2025

tarunprabhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarunprabhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment