From bd839ca6b5706388049fb044c27323a8b46c17f6 Mon Sep 17 00:00:00 2001 From: Paul Thomson Date: Thu, 3 Oct 2019 16:21:05 +0100 Subject: [PATCH] reduce/fuzz: improve command line args (#2932) * reduce: add -o. * reduce: add --temp-file-prefix. * reduce: add interestingness test args. * Detect bad args with one dash e.g. -a. * reduce: fix validator args. * Add = to args that require it. * More consistent naming/style across fuzz/reduce. * Change some 0 exit codes to 1. --- tools/fuzz/fuzz.cpp | 45 ++++++------- tools/reduce/reduce.cpp | 145 +++++++++++++++++++++++++--------------- 2 files changed, 113 insertions(+), 77 deletions(-) diff --git a/tools/fuzz/fuzz.cpp b/tools/fuzz/fuzz.cpp index f474b30d..083ed9f4 100644 --- a/tools/fuzz/fuzz.cpp +++ b/tools/fuzz/fuzz.cpp @@ -73,7 +73,7 @@ void PrintUsage(const char* program) { USAGE: %s [options] -o USAGE: %s [options] -o \ - --shrink= -- + --shrink= -- [args...] The SPIR-V binary is read from . If is also present, facts about the SPIR-V binary are read from this file. @@ -82,11 +82,11 @@ The transformed SPIR-V binary is written to . Human-readable and binary representations of the transformations that were applied are written to and , respectively. -When passing --shrink= an +When passing --shrink= an must also be provided; this is the path to a script that returns 0 if and only if a given SPIR-V binary is interesting. The SPIR-V binary will be passed to -the script as an argument after any other provided arguments. The "--" -characters are optional but denote that all arguments that follow are +the script as an argument after any other provided arguments [args...]. The +"--" characters are optional but denote that all arguments that follow are positional arguments and thus will be forwarded to the interestingness script, and not parsed by %s. @@ -99,17 +99,17 @@ Options (in lexicographical order): --replay File from which to read a sequence of transformations to replay (instead of fuzzing) - --seed + --seed= Unsigned 32-bit integer seed to control random number generation. - --shrink + --shrink= File from which to read a sequence of transformations to shrink (instead of fuzzing) - --shrinker-step-limit + --shrinker-step-limit= Unsigned 32-bit integer specifying maximum number of steps the shrinker will take before giving up. Ignored unless --shrink is used. - --shrinker-temp-file-prefix + --shrinker-temp-file-prefix= Specifies a temporary file prefix that will be used to output temporary shader files during shrinking. A number and .spv extension will be added. The default is "temp_", which will @@ -141,7 +141,7 @@ void FuzzDiagnostic(spv_message_level_t level, const char* /*source*/, FuzzStatus ParseFlags(int argc, const char** argv, std::string* in_binary_file, std::string* out_binary_file, std::string* replay_transformations_file, - std::vector* interestingness_function, + std::vector* interestingness_test, std::string* shrink_transformations_file, std::string* shrink_temp_file_prefix, spvtools::FuzzerOptions* fuzzer_options) { @@ -197,11 +197,12 @@ FuzzStatus ParseFlags(int argc, const char** argv, std::string* in_binary_file, *shrink_temp_file_prefix = std::string(split_flag.second); } else if (0 == strcmp(cur_arg, "--")) { only_positional_arguments_remain = true; - } else if ('\0' == cur_arg[1]) { - // We do not support fuzzing from standard input. We could support - // this if there was a compelling use case. + } else { + std::stringstream ss; + ss << "Unrecognized argument: " << cur_arg << std::endl; + spvtools::Error(FuzzDiagnostic, nullptr, {}, ss.str().c_str()); PrintUsage(argv[0]); - return {FuzzActions::STOP, 0}; + return {FuzzActions::STOP, 1}; } } else if (positional_arg_index == 0) { // Binary input file name @@ -209,7 +210,7 @@ FuzzStatus ParseFlags(int argc, const char** argv, std::string* in_binary_file, *in_binary_file = std::string(cur_arg); positional_arg_index++; } else { - interestingness_function->push_back(std::string(cur_arg)); + interestingness_test->push_back(std::string(cur_arg)); } } @@ -233,8 +234,7 @@ FuzzStatus ParseFlags(int argc, const char** argv, std::string* in_binary_file, return {FuzzActions::STOP, 1}; } - if (shrink_transformations_file->empty() && - !interestingness_function->empty()) { + if (shrink_transformations_file->empty() && !interestingness_test->empty()) { spvtools::Error(FuzzDiagnostic, nullptr, {}, "Too many positional arguments specified; extra positional " "arguments are used as the interestingness function, which " @@ -242,8 +242,7 @@ FuzzStatus ParseFlags(int argc, const char** argv, std::string* in_binary_file, return {FuzzActions::STOP, 1}; } - if (!shrink_transformations_file->empty() && - interestingness_function->empty()) { + if (!shrink_transformations_file->empty() && interestingness_test->empty()) { spvtools::Error( FuzzDiagnostic, nullptr, {}, "The --shrink option requires an interestingness function."); @@ -264,9 +263,9 @@ FuzzStatus ParseFlags(int argc, const char** argv, std::string* in_binary_file, if (!shrink_transformations_file->empty()) { // The tool is being invoked in shrink mode. - assert(!interestingness_function->empty() && + assert(!interestingness_test->empty() && "An error should have been raised if --shrink was provided without " - "an interestingness function."); + "an interestingness test."); return {FuzzActions::SHRINK, 0}; } @@ -413,7 +412,7 @@ int main(int argc, const char** argv) { std::string in_binary_file; std::string out_binary_file; std::string replay_transformations_file; - std::vector interestingness_function; + std::vector interestingness_test; std::string shrink_transformations_file; std::string shrink_temp_file_prefix = "temp_"; @@ -421,7 +420,7 @@ int main(int argc, const char** argv) { FuzzStatus status = ParseFlags( argc, argv, &in_binary_file, &out_binary_file, - &replay_transformations_file, &interestingness_function, + &replay_transformations_file, &interestingness_test, &shrink_transformations_file, &shrink_temp_file_prefix, &fuzzer_options); if (status.action == FuzzActions::STOP) { @@ -480,7 +479,7 @@ int main(int argc, const char** argv) { } if (!Shrink(target_env, fuzzer_options, binary_in, initial_facts, shrink_transformations_file, shrink_temp_file_prefix, - interestingness_function, &binary_out, + interestingness_test, &binary_out, &transformations_applied)) { return 1; } diff --git a/tools/reduce/reduce.cpp b/tools/reduce/reduce.cpp index d439726d..930fa5dd 100644 --- a/tools/reduce/reduce.cpp +++ b/tools/reduce/reduce.cpp @@ -58,20 +58,24 @@ void PrintUsage(const char* program) { // NOTE: Please maintain flags in lexicographical order. printf( R"(%s - Reduce a SPIR-V binary file with respect to a user-provided - interestingness test. +interestingness test. -USAGE: %s [options] +USAGE: %s [options] -o -- [args...] -The SPIR-V binary is read from . +The SPIR-V binary is read from . The reduced SPIR-V binary is +written to . -Whether a binary is interesting is determined by , which -should be the path to a script. +Whether a binary is interesting is determined by , which +should be the path to a script. The "--" characters are optional but denote +that all arguments that follow are positional arguments and thus will be +forwarded to the interestingness test, and not parsed by %s. * The script must be executable. - * The script should take the path to a SPIR-V binary file (.spv) as its single + * The script should take the path to a SPIR-V binary file (.spv) as an argument, and exit with code 0 if and only if the binary file is - interesting. + interesting. The binary will be passed to the script as an argument after + any other provided arguments [args...]. * Example: an interestingness test for reducing a SPIR-V binary file that causes tool "foo" to exit with error code 1 and print "Fatal error: bar" to @@ -95,9 +99,15 @@ Options (in lexicographical order): SPIR-V module that fails to validate. -h, --help Print this help. - --step-limit + --step-limit= 32-bit unsigned integer specifying maximum number of steps the reducer will take before giving up. + --temp-file-prefix= + Specifies a temporary file prefix that will be used to output + temporary shader files during reduction. A number and .spv + extension will be added. The default is "temp_", which will + cause files like "temp_0001.spv" to be output to the current + directory. --version Display reducer version information. @@ -109,7 +119,7 @@ Supported validator options are as follows. See `spirv-val --help` for details. --scalar-block-layout --skip-block-layout )", - program, program); + program, program, program); } // Message consumer for this tool. Used to emit diagnostics during @@ -123,15 +133,19 @@ void ReduceDiagnostic(spv_message_level_t level, const char* /*source*/, fprintf(stderr, "%s\n", message); } -ReduceStatus ParseFlags(int argc, const char** argv, const char** in_file, - const char** interestingness_test, +ReduceStatus ParseFlags(int argc, const char** argv, + std::string* in_binary_file, + std::string* out_binary_file, + std::vector* interestingness_test, + std::string* temp_file_prefix, spvtools::ReducerOptions* reducer_options, spvtools::ValidatorOptions* validator_options) { uint32_t positional_arg_index = 0; + bool only_positional_arguments_remain = false; for (int argi = 1; argi < argc; ++argi) { const char* cur_arg = argv[argi]; - if ('-' == cur_arg[0]) { + if ('-' == cur_arg[0] && !only_positional_arguments_remain) { if (0 == strcmp(cur_arg, "--version")) { spvtools::Logf(ReduceDiagnostic, SPV_MSG_INFO, nullptr, {}, "%s\n", spvSoftwareVersionDetailsString()); @@ -139,11 +153,13 @@ ReduceStatus ParseFlags(int argc, const char** argv, const char** in_file, } else if (0 == strcmp(cur_arg, "--help") || 0 == strcmp(cur_arg, "-h")) { PrintUsage(argv[0]); return {REDUCE_STOP, 0}; - } else if ('\0' == cur_arg[1]) { - // We do not support reduction from standard input. We could support - // this if there was a compelling use case. - PrintUsage(argv[0]); - return {REDUCE_STOP, 0}; + } else if (0 == strcmp(cur_arg, "-o")) { + if (out_binary_file->empty() && argi + 1 < argc) { + *out_binary_file = std::string(argv[++argi]); + } else { + PrintUsage(argv[0]); + return {REDUCE_STOP, 1}; + } } else if (0 == strncmp(cur_arg, "--step-limit=", sizeof("--step-limit=") - 1)) { const auto split_flag = spvtools::utils::SplitFlagArgs(cur_arg); @@ -153,43 +169,54 @@ ReduceStatus ParseFlags(int argc, const char** argv, const char** in_file, static_cast(strtol(split_flag.second.c_str(), &end, 10)); assert(end != split_flag.second.c_str() && errno == 0); reducer_options->set_step_limit(step_limit); + } else if (0 == strcmp(cur_arg, "--fail-on-validation-error")) { + reducer_options->set_fail_on_validation_error(true); + } else if (0 == strcmp(cur_arg, "--before-hlsl-legalization")) { + validator_options->SetBeforeHlslLegalization(true); + } else if (0 == strcmp(cur_arg, "--relax-logical-pointer")) { + validator_options->SetRelaxLogicalPointer(true); + } else if (0 == strcmp(cur_arg, "--relax-block-layout")) { + validator_options->SetRelaxBlockLayout(true); + } else if (0 == strcmp(cur_arg, "--scalar-block-layout")) { + validator_options->SetScalarBlockLayout(true); + } else if (0 == strcmp(cur_arg, "--skip-block-layout")) { + validator_options->SetSkipBlockLayout(true); + } else if (0 == strcmp(cur_arg, "--relax-struct-store")) { + validator_options->SetRelaxStructStore(true); + } else if (0 == strncmp(cur_arg, "--temp-file-prefix=", + sizeof("--temp-file-prefix=") - 1)) { + const auto split_flag = spvtools::utils::SplitFlagArgs(cur_arg); + *temp_file_prefix = std::string(split_flag.second); + } else if (0 == strcmp(cur_arg, "--")) { + only_positional_arguments_remain = true; + } else { + std::stringstream ss; + ss << "Unrecognized argument: " << cur_arg << std::endl; + spvtools::Error(ReduceDiagnostic, nullptr, {}, ss.str().c_str()); + PrintUsage(argv[0]); + return {REDUCE_STOP, 1}; } } else if (positional_arg_index == 0) { - // Input file name - assert(!*in_file); - *in_file = cur_arg; + // Binary input file name + assert(in_binary_file->empty()); + *in_binary_file = std::string(cur_arg); positional_arg_index++; - } else if (positional_arg_index == 1) { - assert(!*interestingness_test); - *interestingness_test = cur_arg; - positional_arg_index++; - } else if (0 == strcmp(cur_arg, "--fail-on-validation-error")) { - reducer_options->set_fail_on_validation_error(true); - } else if (0 == strcmp(cur_arg, "--before-hlsl-legalization")) { - validator_options->SetBeforeHlslLegalization(true); - } else if (0 == strcmp(cur_arg, "--relax-logical-pointer")) { - validator_options->SetRelaxLogicalPointer(true); - } else if (0 == strcmp(cur_arg, "--relax-block-layout")) { - validator_options->SetRelaxBlockLayout(true); - } else if (0 == strcmp(cur_arg, "--scalar-block-layout")) { - validator_options->SetScalarBlockLayout(true); - } else if (0 == strcmp(cur_arg, "--skip-block-layout")) { - validator_options->SetSkipBlockLayout(true); - } else if (0 == strcmp(cur_arg, "--relax-struct-store")) { - validator_options->SetRelaxStructStore(true); } else { - spvtools::Error(ReduceDiagnostic, nullptr, {}, - "Too many positional arguments specified"); - return {REDUCE_STOP, 1}; + interestingness_test->push_back(std::string(cur_arg)); } } - if (!*in_file) { + if (in_binary_file->empty()) { spvtools::Error(ReduceDiagnostic, nullptr, {}, "No input file specified"); return {REDUCE_STOP, 1}; } - if (!*interestingness_test) { + if (out_binary_file->empty()) { + spvtools::Error(ReduceDiagnostic, nullptr, {}, "-o required"); + return {REDUCE_STOP, 1}; + } + + if (interestingness_test->empty()) { spvtools::Error(ReduceDiagnostic, nullptr, {}, "No interestingness test specified"); return {REDUCE_STOP, 1}; @@ -220,15 +247,18 @@ void DumpShader(spvtools::opt::IRContext* context, const char* filename) { const auto kDefaultEnvironment = SPV_ENV_UNIVERSAL_1_5; int main(int argc, const char** argv) { - const char* in_file = nullptr; - const char* interestingness_test = nullptr; + std::string in_binary_file; + std::string out_binary_file; + std::vector interestingness_test; + std::string temp_file_prefix = "temp_"; spv_target_env target_env = kDefaultEnvironment; spvtools::ReducerOptions reducer_options; spvtools::ValidatorOptions validator_options; - ReduceStatus status = ParseFlags(argc, argv, &in_file, &interestingness_test, - &reducer_options, &validator_options); + ReduceStatus status = ParseFlags( + argc, argv, &in_binary_file, &out_binary_file, &interestingness_test, + &temp_file_prefix, &reducer_options, &validator_options); if (status.action == REDUCE_STOP) { return status.code; @@ -242,15 +272,22 @@ int main(int argc, const char** argv) { spvtools::reduce::Reducer reducer(target_env); + std::stringstream joined; + joined << interestingness_test[0]; + for (size_t i = 1, size = interestingness_test.size(); i < size; ++i) { + joined << " " << interestingness_test[i]; + } + std::string interestingness_command_joined = joined.str(); + reducer.SetInterestingnessFunction( - [interestingness_test](std::vector binary, - uint32_t reductions_applied) -> bool { + [interestingness_command_joined, temp_file_prefix]( + std::vector binary, uint32_t reductions_applied) -> bool { std::stringstream ss; - ss << "temp_" << std::setw(4) << std::setfill('0') << reductions_applied - << ".spv"; + ss << temp_file_prefix << std::setw(4) << std::setfill('0') + << reductions_applied << ".spv"; const auto spv_file = ss.str(); const std::string command = - std::string(interestingness_test) + " " + spv_file; + interestingness_command_joined + " " + spv_file; auto write_file_succeeded = WriteFile(spv_file.c_str(), "wb", &binary[0], binary.size()); (void)(write_file_succeeded); @@ -263,7 +300,7 @@ int main(int argc, const char** argv) { reducer.SetMessageConsumer(spvtools::utils::CLIMessageConsumer); std::vector binary_in; - if (!ReadFile(in_file, "rb", &binary_in)) { + if (!ReadFile(in_binary_file.c_str(), "rb", &binary_in)) { return 1; } @@ -273,7 +310,7 @@ int main(int argc, const char** argv) { if (reduction_status == spvtools::reduce::Reducer::ReductionResultStatus:: kInitialStateNotInteresting || - !WriteFile("_reduced_final.spv", "wb", binary_out.data(), + !WriteFile(out_binary_file.c_str(), "wb", binary_out.data(), binary_out.size())) { return 1; }