-
Notifications
You must be signed in to change notification settings - Fork 343
[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 5 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 |
---|---|---|
|
@@ -10,6 +10,12 @@ | |
// Will return true if cmd doesn't crash and returns false. | ||
// not --crash cmd | ||
// Will return true if cmd crashes (e.g. for testing crash reporting). | ||
// not --run-under ...<emulator and args>... -- cmd | ||
// Will prepend the emulator and its arguments to the spawn of the | ||
// 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. | ||
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. Could you add sentence clarifying what the return value will be in this case. 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. 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. |
||
|
||
// This file is a stripped down version of not.cpp from llvm/utils. This does | ||
// not depend on any LLVM library. | ||
|
@@ -29,17 +35,52 @@ | |
#include <sys/wait.h> | ||
#endif | ||
|
||
#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 |
||
|
||
int main(int argc, char* const* argv) { | ||
bool expectCrash = false; | ||
|
||
++argv; | ||
--argc; | ||
|
||
if (argc > 0 && std::string(argv[0]) == "--crash") { | ||
// If necessary, prepend the command and arguments for a user-mode | ||
// emulator such as QEMU to the command line that will be used | ||
// to then spawn a subcommand. | ||
std::vector<char *> argvbuf; | ||
if (argc > 0 && std::string(argv[0]) == "--run-under") { | ||
++argv; | ||
--argc; | ||
while (argc > 0) { | ||
--argc; | ||
if (std::string(argv[0]) == "--") { | ||
++argv; | ||
break; | ||
} | ||
|
||
argvbuf.push_back(argv[0]); | ||
++argv; | ||
} | ||
|
||
// If present, the crash flag is between the 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. Nit: Does this comment need to be split across two lines? |
||
// arguments and the cmd. | ||
if (argc > 0 && std::string(argv[0]) == "--crash") { | ||
++argv; | ||
--argc; | ||
expectCrash = true; | ||
} | ||
|
||
for (char *const *argp = argv; *argp != NULL; ++argp) | ||
argvbuf.push_back(*argp); | ||
argvbuf.push_back(NULL); | ||
argv = argvbuf.data(); | ||
argc = argvbuf.size() - 1; | ||
} else if (argc > 0 && std::string(argv[0]) == "--crash") { | ||
++argv; | ||
--argc; | ||
expectCrash = true; | ||
} | ||
|
||
if (expectCrash) { | ||
// Crash is expected, so disable crash report and symbolization to reduce | ||
// output and avoid potentially slow symbolization. | ||
#ifdef _WIN32 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,3 @@ | ||
# Copy these files to the build directory so that the tests can be run even | ||
# without the source directory. | ||
configure_file(test_not.py test_not.py | ||
COPYONLY) | ||
|
||
llvm_test_executable_no_test(ret1 ret1.c) | ||
add_dependencies(ret1 not) | ||
llvm_test_run(EXECUTABLE "$<TARGET_FILE:not>" "$<TARGET_FILE:ret1>") | ||
|
@@ -13,15 +8,21 @@ 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. | ||
# Check that expected crashes are handled correctly under user-mode emulation. | ||
llvm_test_executable_no_test(abrt abort.c) | ||
add_dependencies(abrt not) | ||
llvm_test_run(EXECUTABLE "$<TARGET_FILE:not>" "--crash" "$<TARGET_FILE:abrt>") | ||
llvm_test_run(EXECUTABLE ${NOT_TOOL} "--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(test_not test_not.cpp) | ||
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>") | ||
add_dependencies(check_env not test_not) | ||
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>") | ||
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. Is 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 for noticing, not it is not. I just tested in a clean build to make sure. |
||
else() | ||
llvm_test_run(EXECUTABLE ${Python_EXECUTABLE} "$<TARGET_FILE:test_not>" ${NOT_TOOL} "$<TARGET_FILE:check_env>") | ||
endif() | ||
llvm_add_test_For_target(check_env) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
//===- test_not.cpp - Test for enviroment variables in the not tool -------===// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#include <cstdlib> | ||
#include <iostream> | ||
#include <sstream> | ||
|
||
#ifdef _WIN32 | ||
#define WIN32_LEAN_AND_MEAN | ||
#define NOMINMAX | ||
#include <windows.h> | ||
#endif | ||
|
||
#if defined(__unix__) || defined(__APPLE__) | ||
#include <spawn.h> | ||
#include <sys/wait.h> | ||
#endif | ||
|
||
int main(int argc, char *const *argv) { | ||
++argv; | ||
--argc; | ||
if (argc == 0) | ||
return EXIT_FAILURE; | ||
|
||
#ifdef _WIN32 | ||
SetEnvironmentVariableA("SET_IN_PARENT", "something"); | ||
#else | ||
setenv("SET_IN_PARENT", "something", 0); | ||
#endif | ||
|
||
int result; | ||
|
||
#if defined(__unix__) || defined(__APPLE__) | ||
pid_t pid; | ||
extern char **environ; | ||
if (posix_spawn(&pid, argv[0], NULL, NULL, argv, environ)) | ||
return EXIT_FAILURE; | ||
if (waitpid(pid, &result, WUNTRACED | WCONTINUED) == -1) | ||
return EXIT_FAILURE; | ||
#else | ||
std::stringstream ss; | ||
ss << argv[0]; | ||
for (int i = 1; i < argc; ++i) | ||
ss << " " << argv[i]; | ||
std::string cmd = ss.str(); | ||
result = std::system(cmd.c_str()); | ||
#endif | ||
|
||
int retcode = 0; | ||
int signal = 0; | ||
|
||
#ifdef _WIN32 | ||
// Handle abort() in msvcrt -- It has exit code as 3. abort(), aka | ||
// unreachable, should be recognized as a crash. However, some binaries use | ||
// exit code 3 on non-crash failure paths, so only do this if we expect a | ||
// crash. | ||
if (errno) { | ||
// If the command interpreter was not found, errno will be set and 0 will | ||
// be returned. It is unlikely that this will happen in our use case, but | ||
// check anyway. | ||
retcode = 1; | ||
signal = 1; | ||
} else { | ||
// On Windows, result is the exit code, except for the special case above. | ||
retcode = result; | ||
signal = 0; | ||
} | ||
#elif defined(WIFEXITED) && defined(WEXITSTATUS) && defined(WIFSIGNALED) && \ | ||
defined(WTERMSIG) | ||
// On POSIX systems and Solaris, result is a composite value of the exit code | ||
// and, potentially, the signal that caused termination of the command. | ||
if (WIFEXITED(result)) | ||
retcode = WEXITSTATUS(result); | ||
if (WIFSIGNALED(result)) | ||
signal = WTERMSIG(result); | ||
#else | ||
#error "Unsupported system" | ||
#endif | ||
|
||
if (!signal && retcode == EXIT_SUCCESS) | ||
return EXIT_SUCCESS; | ||
return EXIT_FAILURE; | ||
} |
This file was deleted.
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:
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.
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.
I reformatted this and the following two lines to make it fit.