[Support] Introduce a new utility for testing child process execution

[zturner]

Summary

I was auditing our process launching API and looking at how well it was tested, and I noticed that none of the functionality related to I/O redirection was tested at all. I also noticed that all testing was done via unit tests and they are all extremely hard to understand and follow.

The approach here is more LLVMish, using a tool to dump some output and then FileCheck it. Since I was specifically trying to test re-direction here, the only support I've added so far is for specifying command line arguments that describe how to redirect stdout and stderr, and I wrote some tests which I believe are significantly easier to understand than the existing unit tests.

There are plenty of other interesting opportunities for exploration with a tool such as this. For example, LLDB (or any kind of tracing utility) could use it as a kind of "strace" equivalent where it logs all of its events and we could FileCheck that.

In a followup patch, I plan to use this mechanism to test that launching a process with a specific environment works. I am going to do this by introducing meta-variable substitutions into the command language, so that you can write something like write stdout $ENV{PATH}

[zturner]

+rnk.

Also, one thing that's a little strange here is that this now marks the one and only Support test that is *not* a unit test. Is that weird? I feel like this is a much better pattern for testing process launching though, so I'd feel pretty bad if I had to re-write all this as unit tests.

[rnk]

Looks good, but @dblaikie has helpful thoughts about testing and he is responsive, so I'd let him take a look.

[davide]

I like the idea. Some comments inline.

llvm/utils/trace/Trace.cpp

19–20

Is io.h really needed?

92–96

Could probably factor this out in a report_error function as the other tools do (or just call report_fatal_error if you can).

148–149

return run() ?

152

SmallVector ?

167–168

As we're basically flushing everytime we write to outs() should we provide a wrapper for this?

[dblaikie]

Hey Zach,

Thanks for looking into/improving test coverage!

My knee-jerk/gut reaction to the idea of adding a tool binary for testing a
fairly low-level API like this is that it doesn't /feel/ quite right, and
I've looked over the patch a bit but not in quite enough detail to provide
really actionable/specific feedback yet.

My broad/high-level question would be: Could the existing unit test
approach be made more/sufficiently readable by implementing helper
functions (essentially the kind of code that's in the new tool you're
introducing - providing that as utility functions with parameters, rather
than a program with command line parameters). It seems like that might be a
simpler alternative, but I'm certainly not 100% sure of it.

• Dave

[zturner]

In D47913#1126958, @dblaikie wrote:

Hey Zach,

Thanks for looking into/improving test coverage!

My knee-jerk/gut reaction to the idea of adding a tool binary for testing a
fairly low-level API like this is that it doesn't /feel/ quite right, and
I've looked over the patch a bit but not in quite enough detail to provide
really actionable/specific feedback yet.

My broad/high-level question would be: Could the existing unit test
approach be made more/sufficiently readable by implementing helper
functions (essentially the kind of code that's in the new tool you're
introducing - providing that as utility functions with parameters, rather
than a program with command line parameters). It seems like that might be a
simpler alternative, but I'm certainly not 100% sure of it.

• Dave

For simple cases, maybe. But I expect to gradually test more and more complicated things and it's going to get quite awkward testing those kinds of things with unit tests.

Right now the way the unit tests test that spawning a process with a specific environment works is that the particular unit test checks for the presence of an environment variable and returns a magic value if it finds it. Then the unit test spawns the top-level gtest executable again, filtered to this specific test. It should detect the environment variable and return the magic value.

That's about on the borderline of how gross I would want a unit test to be.

Imagine you implemented a ptrace loop on top of our sys::Execute functions. Then you might want to test that events that come through are as you would expect for a variety of different test programs, so now the unit tests start having inputs that you have to compile first.

I was hoping this utility could be useful in these types of situations as well.

Think of it sort of like a little miniature interpreted DSL that allow a parent and child process to communicate with each other. Maybe this just further cements your idea that this is too high level for testing low level process launching though :), I was just viewing it as a special case of more complicated scenarios.

It does feel a little weird that this is the only 2 tests that are not unit tests, but idk. If you don't think it's a good fit for a lit style test I can go back to the unit test model, but I do think the current Program unit tests are kind of pushing the limits of what unit tests should be doing.

[dblaikie]

ah, yeah - thanks for pointing out/explaining the existing tests a bit & how they're re-executing the full gtest then waiting to be run themselves, etc.

Worth having /some/ binary rather than rerunning through gtest, I think - and once you have one, this doesn't seem too bad.

Could you port/move the existing unit tests over to this framework so there's not a mix of approaches to the same problem?

llvm/CMakeLists.txt

 
 add_subdirectory(lib)
 
 if( LLVM_INCLUDE_UTILS )
   add_subdirectory(utils/FileCheck)
   add_subdirectory(utils/PerfectShuffle)
   add_subdirectory(utils/count)
   add_subdirectory(utils/not)
+  add_subdirectory(utils/trace)
   add_subdirectory(utils/yaml-bench)
 else()
   if ( LLVM_INCLUDE_TESTS )
     message(FATAL_ERROR "Including tests when not building utils will not work.
     Either set LLVM_INCLUDE_UTILS to On, or set LLVM_INCLDE_TESTS to Off.")
   endif()
 endif()
 

llvm/test/Support/execute.test

+; RUN: rm -f %t.stdout
+; RUN: trace -execfile=%s 2>&1 | FileCheck -check-prefix=NONE_REDIRECTED %s
+; RUN: trace -stdout=%t.stdout -execfile=%s 2>&1 | FileCheck -check-prefix=STDOUT_REDIRECTED %s
+; RUN: FileCheck -input-file=%t.stdout -check-prefix=STDOUT_CONTENTS %s
+; RUN: rm -f %t.stderr
+; RUN: trace -stderr=%t.stderr -execfile=%s 2>&1 | FileCheck -check-prefix=STDERR_REDIRECTED %s
+; RUN: FileCheck -input-file=%t.stderr -check-prefix=STDERR_CONTENTS %s
+; RUN: rm -f %t.stdout %t.stderr
+; RUN: trace -stdout=%t.stdout -stderr=%t.stderr -execfile=%s 2>&1 | FileCheck -check-prefix=BOTH_REDIRECTED %s
+; RUN: FileCheck -input-file=%t.stdout -check-prefix=STDOUT_CONTENTS %s
+; RUN: FileCheck -input-file=%t.stderr -check-prefix=STDERR_CONTENTS %s
+
+
+write stdout This goes to stdout
+write stderr This goes to stderr
+exit 42
+
+; NONE_REDIRECTED:      PARENT(stdout): Running
+; NONE_REDIRECTED-NEXT: CHILD(stdout): Running
+; NONE_REDIRECTED-NEXT: CHILD(stdout): This goes to stdout
+; NONE_REDIRECTED-NEXT: CHILD(stderr): This goes to stderr
+; NONE_REDIRECTED-NEXT: CHILD(stdout): exiting with code 42
+; NONE_REDIRECTED-NEXT: PARENT(stdout): Child exited with code 42
+
+; STDOUT_REDIRECTED:      PARENT(stdout): Running
+; STDOUT_REDIRECTED-NEXT: CHILD(stderr): This goes to stderr
+; STDOUT_REDIRECTED-NEXT: PARENT(stdout): Child exited with code 42
+
+; STDOUT_CONTENTS:      CHILD(stdout): Running
+; STDOUT_CONTENTS-NEXT: CHILD(stdout): This goes to stdout
+; STDOUT_CONTENTS-NEXT: CHILD(stdout): exiting with code 42
+
+; STDERR_REDIRECTED:      PARENT(stdout): Running
+; STDERR_REDIRECTED-NEXT: CHILD(stdout): Running
+; STDERR_REDIRECTED-NEXT: CHILD(stdout): This goes to stdout
+; STDERR_REDIRECTED-NEXT: CHILD(stdout): exiting with code 42
+; STDERR_REDIRECTED-NEXT: PARENT(stdout): Child exited with code 42
+
+; STDERR_CONTENTS: CHILD(stderr): This goes to stderr
+
+; BOTH_REDIRECTED:      PARENT(stdout): Running
+; BOTH_REDIRECTED-NEXT: PARENT(stdout): Child exited with code 42

llvm/test/lit.cfg.py

tools.extend([
     'llvm-config', 'llvm-cov', 'llvm-cxxdump', 'llvm-cvtres', 'llvm-diff', 'llvm-dis',
     'llvm-dwarfdump', 'llvm-extract', 'llvm-isel-fuzzer', 'llvm-opt-fuzzer', 'llvm-lib',
     'llvm-link', 'llvm-lto', 'llvm-lto2', 'llvm-mc', 'llvm-mca',
     'llvm-modextract', 'llvm-nm', 'llvm-objcopy', 'llvm-objdump',
     'llvm-pdbutil', 'llvm-profdata', 'llvm-ranlib', 'llvm-readobj',
     'llvm-rtdyld', 'llvm-size', 'llvm-split', 'llvm-strings', 'llvm-strip', 'llvm-tblgen',
     'llvm-c-test', 'llvm-cxxfilt', 'llvm-xray', 'yaml2obj', 'obj2yaml',
     'yaml-bench', 'verify-uselistorder',
-    'bugpoint', 'llc', 'llvm-symbolizer', 'opt', 'sancov', 'sanstats'])
+    'bugpoint', 'llc', 'llvm-symbolizer', 'opt', 'sancov', 'sanstats',
+    ToolSubst(r'\| \btrace\b', command=FindTool('trace'), verbatim=True, unresolved='fatal'),
+    ])
 
 # The following tools are optional
 tools.extend([
     ToolSubst('llvm-go', unresolved='ignore'),
     ToolSubst('llvm-mt', unresolved='ignore'),
     ToolSubst('Kaleidoscope-Ch3', unresolved='ignore'),
     ToolSubst('Kaleidoscope-Ch4', unresolved='ignore'),
     ToolSubst('Kaleidoscope-Ch5', unresolved='ignore'),

llvm/utils/trace/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  Support
+  )
+
+add_llvm_utility(trace
+  Trace.cpp
+  )

llvm/utils/trace/Trace.cpp

+//===- Trace.cpp - The 'trace' testing tool -------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/LineIterator.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Program.h"
+
+#include <iostream>
+
+#include <io.h>
+
+using namespace llvm;
+
+static cl::opt<std::string> StdinFile("stdin", cl::desc("<stdin file>"),
+                                      cl::Optional);
+
+static cl::opt<std::string> StdoutFile("stdout", cl::desc("<stdout file>"),
+                                       cl::Optional);
+
+static cl::opt<std::string> StderrFile("stderr", cl::desc("<stderr file>"),
+                                       cl::Optional);
+
+static cl::list<std::string>
+    EnvVar("env", cl::desc("KEY:VALUE pair to set in target process"),
+           cl::Optional, cl::ZeroOrMore);
+
+static cl::opt<std::string>
+    Cwd("cwd", cl::desc("The current working directory of the process"),
+        cl::Optional);
+
+static cl::opt<std::string>
+    ExecFile("execfile", cl::desc("The command file to execute"), cl::Required);
+
+static cl::opt<bool> ExecNow("execnow", cl::ReallyHidden);
+
+static void handleWrite(StringRef Line) {
+  Line = Line.trim();
+
+  StringRef Dest;
+  StringRef Text;
+  std::tie(Dest, Text) = Line.split(' ');
+  int FD = -1;
+  if (Dest.equals_lower("stdout")) {
+    outs() << "CHILD(stdout): " << Text << "\n";
+    outs().flush();
+  } else if (Dest.equals_lower("stderr")) {
+    errs() << "CHILD(stderr): " << Text << "\n";
+    errs().flush();
+  } else if (to_integer(Dest, FD, 10)) {
+    raw_fd_ostream OS(FD, false, true);
+    OS << "CHILD(" << FD << "): " << Text << "\n";
+    OS.flush();
+  } else {
+    errs() << formatv(
+        "CHILD(stderr): Invalid argument '{0}' to command 'write'\n", Dest);
+    exit(-1);
+  }
+}
+
+static void handleRead(StringRef Line) {
+  Line = Line.trim();
+
+  StringRef Source;
+  StringRef LengthStr;
+
+  std::tie(Source, LengthStr) = Line.split(' ');
+
+  unsigned Length;
+  int FD = -1;
+  if (!to_integer(LengthStr, Length, 10)) {
+    errs() << formatv(
+        "CHILD(stderr): Invalid argument '{0}' to command 'read'\n", LengthStr);
+    exit(-1);
+  }
+
+  std::vector<char> InputBuffer;
+  InputBuffer.resize(Length);
+
+  if (Source.equals_lower("stdin"))
+    std::cin.get(InputBuffer.data(), Length);
+  else if (to_integer(Source, FD, 10)) {
+    ::read(FD, InputBuffer.data(), Length);
+  } else {
+    errs() << formatv(
+        "CHILD(stderr): Invalid argument '{0}' to command 'read'\n", Source);
+    exit(-1);
+  }
+  outs() << formatv("CHILD(stdout): Read {0} bytes from {1}: '{2}'\n", Length,
+                    Source, StringRef(InputBuffer.data(), InputBuffer.size()));
+}
+
+static void handleExit(StringRef Line) {
+  Line = Line.trim();
+
+  int Code;
+  if (!to_integer(Line, Code, 10)) {
+    errs() << formatv(
+        "CHILD(stderr): Invalid argument '{0}' to command 'exit'\n", Line);
+    exit(-1);
+  }
+
+  outs() << "CHILD(stdout): exiting with code " << Code << "\n";
+  outs().flush();
+  exit(Code);
+}
+
+static int run() {
+  auto FileContents = MemoryBuffer::getFile(ExecFile);
+  if (!FileContents) {
+    errs() << "CHILD(stderr): Unable to open execfile " << ExecFile << "\n";
+    return -1;
+  }
+  std::unique_ptr<MemoryBuffer> Buffer = std::move(*FileContents);
+  for (StringRef Line :
+       make_range(line_iterator(*Buffer, true, ';'), line_iterator())) {
+    Line = Line.trim();
+    if (Line.consume_front("write"))
+      handleWrite(Line);
+    else if (Line.consume_front("read"))
+      handleRead(Line);
+    else if (Line.consume_front("exit"))
+      handleExit(Line);
+    else {
+      errs() << "CHILD(stderr): Error in command: " << Line << "\n";
+      return -1;
+    }
+  }
+  outs() << "CHILD(stdout): EOF encountered, exiting\n";
+  return 0;
+}
+
+int main(int argc, char **argv) {
+  InitLLVM X(argc, argv);
+  cl::ParseCommandLineOptions(argc, argv);
+
+  if (ExecNow) {
+    outs() << "CHILD(stdout): Running\n";
+    outs().flush();
+    int ExitCode = run();
+    return ExitCode;
+  }
+
+  std::array<llvm::Optional<StringRef>, 3> Redirects;
+
+  if (StdinFile.getNumOccurrences() > 0)
+    Redirects[0] = StdinFile;
+  if (StdoutFile.getNumOccurrences() > 0)
+    Redirects[1] = StdoutFile;
+  if (StderrFile.getNumOccurrences() > 0)
+    Redirects[2] = StderrFile;
+
+  std::string Arg1 = formatv("-execfile={0}", ExecFile.getValue());
+  const char *Args[] = {argv[0], "-execnow", Arg1.c_str(), nullptr};
+
+  outs() << "PARENT(stdout): Running\n";
+  outs().flush();
+  int Result = sys::ExecuteAndWait(argv[0], Args, nullptr, Redirects);
+  outs() << "PARENT(stdout): Child exited with code " << Result << "\n";
+  outs().flush();
+  return 0;
+}