-
Notifications
You must be signed in to change notification settings - Fork 358
[tools] Fix tools/test and not for user-mode emulation #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
b74e2f1
6474814
b6645ef
321227a
6156870
9bc637e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
# 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") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps name this variable
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I changed the name and moved it to the root |
||||||
else() | ||||||
set(NOT_HELPER "not") | ||||||
endif() | ||||||
|
||||||
llvm_test_executable_no_test(${target} ${main} ${others}) | ||||||
if (expect_error) | ||||||
llvm_test_run( | ||||||
EXECUTABLE "%b/not --crash %S/${target}" | ||||||
EXECUTABLE "%b/${NOT_HELPER} --crash %S/${target}" | ||||||
WORKDIR "%S/${working_dir_name}") | ||||||
else () | ||||||
llvm_test_run(WORKDIR "%S/${working_dir_name}") | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -29,6 +29,10 @@ | |||||
#include <sys/wait.h> | ||||||
#endif | ||||||
|
||||||
#ifdef TEST_SUITE_RUN_UNDER | ||||||
#include <vector> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please move this include with the other system includes at the top of this file |
||||||
#endif | ||||||
|
||||||
int main(int argc, char* const* argv) { | ||||||
bool expectCrash = false; | ||||||
|
||||||
|
@@ -54,6 +58,19 @@ int main(int argc, char* const* argv) { | |||||
if (argc == 0) | ||||||
return 1; | ||||||
|
||||||
#ifdef TEST_SUITE_RUN_UNDER | ||||||
// 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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, fixed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to apologize :-) |
||||||
std::vector<char *> argvbuf = {TEST_SUITE_RUN_UNDER}; | ||||||
for (char *const *argp = argv; *argp != NULL; ++argp) | ||||||
argvbuf.push_back(*argp); | ||||||
argvbuf.push_back(NULL); | ||||||
argv = argvbuf.data(); | ||||||
argc = argvbuf.size() - 1; | ||||||
#endif | ||||||
|
||||||
int result; | ||||||
|
||||||
#if defined(__unix__) || defined(__APPLE__) | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,15 +13,26 @@ add_dependencies(ret0 not) | |||||
llvm_test_run(EXECUTABLE "$<TARGET_FILE:not>" "$<TARGET_FILE:not>" "$<TARGET_FILE:ret0>") | ||||||
llvm_add_test_for_target(ret0) | ||||||
|
||||||
# Check that expected crashes are handled correctly. | ||||||
llvm_test_executable_no_test(abrt abort.c) | ||||||
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' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure what tests you are referring to here. If you mean the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the |
||||||
# spawns a subprocess outside the emulator and the check_env test | ||||||
# runs the host python interpreter under the emulator for the target. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does passing environment variables work with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
if(NOT(TEST_SUITE_USER_MODE_EMULATION AND TEST_SUITE_RUN_UNDER)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The negation of the conditional is a bit hard to read. Could the branches be switched with a "positive" conditional instead?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the |
||||||
# Check that expected crashes are handled correctly. | ||||||
llvm_test_executable_no_test(abrt abort.c) | ||||||
add_dependencies(abrt not) | ||||||
llvm_test_run(EXECUTABLE "$<TARGET_FILE:not>" "--crash" "$<TARGET_FILE:abrt>") | ||||||
llvm_add_test_for_target(abrt) | ||||||
|
||||||
# Check that not passes environment variables to the called executable. | ||||||
find_package(Python COMPONENTS Interpreter) | ||||||
llvm_test_executable_no_test(check_env check_env.c) | ||||||
add_dependencies(check_env not) | ||||||
llvm_test_run(EXECUTABLE ${Python_EXECUTABLE} "%b/test/test_not.py" "$<TARGET_FILE:not>" "$<TARGET_FILE:check_env>") | ||||||
llvm_add_test_For_target(check_env) | ||||||
# Check that not passes environment variables to the called executable. | ||||||
find_package(Python COMPONENTS Interpreter) | ||||||
llvm_test_executable_no_test(check_env check_env.c) | ||||||
add_dependencies(check_env not) | ||||||
llvm_test_run(EXECUTABLE ${Python_EXECUTABLE} "%b/test/test_not.py" "$<TARGET_FILE:not>" "$<TARGET_FILE:check_env>") | ||||||
llvm_add_test_For_target(check_env) | ||||||
else() | ||||||
# Check that expected crashes are handled correctly under user-mode emulation. | ||||||
llvm_test_executable_no_test(abrt abort.c) | ||||||
add_dependencies(abrt not-spawning-emulator) | ||||||
llvm_test_run(EXECUTABLE "$<TARGET_FILE:not-spawning-emulator>" "--crash" "$<TARGET_FILE:abrt>") | ||||||
llvm_add_test_for_target(abrt) | ||||||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, fixed.