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

@@ -73,7 +73,7 @@ void PrintUsage(const char* program) {
 
 USAGE: %s [options] <input.spv> -o <output.spv>
 USAGE: %s [options] <input.spv> -o <output.spv> \
-  --shrink=<input.transformations> -- <interestingness_function> <args...>
+  --shrink=<input.transformations> -- <interestingness_test> [args...]
 
 The SPIR-V binary is read from <input.spv>.  If <input.facts> 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 <output.spv>.  Human-readable and
 binary representations of the transformations that were applied are written to
 <output.transformations_json> and <output.transformations>, respectively.
 
-When passing --shrink=<input.transformations> an <interestingness_function>
+When passing --shrink=<input.transformations> an <interestingness_test>
 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<std::string>* interestingness_function,
+                      std::vector<std::string>* 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<std::string> interestingness_function;
+  std::vector<std::string> 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;
       }

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] <input> <interestingness-test>
+USAGE: %s [options] <input.spv> -o <output.spv> -- <interestingness_test> [args...]
 
-The SPIR-V binary is read from <input>.
+The SPIR-V binary is read from <input.spv>. The reduced SPIR-V binary is
+written to <output.spv>.
 
-Whether a binary is interesting is determined by <interestingness-test>, which
-should be the path to a script.
+Whether a binary is interesting is determined by <interestingness_test>, 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<std::string>* 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<uint32_t>(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<std::string> 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<uint32_t> binary,
-                             uint32_t reductions_applied) -> bool {
+      [interestingness_command_joined, temp_file_prefix](
+          std::vector<uint32_t> 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<uint32_t> binary_in;
-  if (!ReadFile<uint32_t>(in_file, "rb", &binary_in)) {
+  if (!ReadFile<uint32_t>(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<uint32_t>("_reduced_final.spv", "wb", binary_out.data(),
+      !WriteFile<uint32_t>(out_binary_file.c_str(), "wb", binary_out.data(),
                            binary_out.size())) {
     return 1;
   }