Restructure the Clang based header generator

- Support multiple input files
- Better testability
- Support for the `--sapi_isystem` argument, same as the Python generator

PiperOrigin-RevId: 333686891
Change-Id: I3e618165c1bd58bb755e1193617fb0737c29ee77

CMakeLists.txt

@@ -44,6 +44,9 @@ include(SapiBuildDefs)
 # Allow the header generator to auto-configure include paths
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 
+# Allow the header generator to auto-configure include paths
+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+
 if (SAPI_FORCE_COLOR_OUTPUT)
   if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")  # GCC
     add_compile_options(-fdiagnostics-color=always)

cmake/SapiBuildDefs.cmake

@@ -132,44 +132,44 @@ function(add_sapi_library)
     set(_sapi_embed_dir "${CMAKE_CURRENT_BINARY_DIR}")
     set(_sapi_embed_name "${_sapi_NAME}")
   endif()
+
+  set(_sapi_isystem "${_sapi_NAME}.isystem}")
+  list(APPEND _sapi_generator_args
+    "--sapi_name=${_sapi_LIBRARY_NAME}"
+    "--sapi_out=${_sapi_gen_header}"
+    "--sapi_embed_dir=${_sapi_embed_dir}"
+    "--sapi_embed_name=${_sapi_embed_name}"
+    "--sapi_functions=${_sapi_funcs}"
+    "--sapi_ns=${_sapi_NAMESPACE}"
+    "--sapi_isystem=${_sapi_isystem}"
+  )
+  list(JOIN _sapi_full_inputs "," _sapi_full_inputs)
   if(SAPI_ENABLE_GENERATOR)
-    add_custom_command(
-      OUTPUT "${_sapi_gen_header}"
-      COMMAND sapi_generator_tool
-              "--sapi_name=${_sapi_LIBRARY_NAME}"
-              "--sapi_out=${_sapi_gen_header}"
-              "--sapi_embed_dir=${_sapi_embed_dir}"
-              "--sapi_embed_name=${_sapi_embed_name}"
-              "--sapi_functions=${_sapi_funcs}"
-              "--sapi_ns=${_sapi_NAMESPACE}"
-              ${_sapi_full_inputs}
-      COMMENT "Generating interface"
-      DEPENDS ${_sapi_INPUTS}
-      VERBATIM
+    list(APPEND _sapi_generator_command
+      sapi_generator_tool
+      -p "${CMAKE_CURRENT_BINARY_DIR}"
+      ${_sapi_generator_args}
+      ${_sapi_full_inputs}
     )
   else()
-    set(_sapi_isystem "${_sapi_NAME}.isystem")
-    list(JOIN _sapi_full_inputs "," _sapi_full_inputs)
-    add_custom_command(
-      OUTPUT "${_sapi_gen_header}" "${_sapi_isystem}"
-      COMMAND sh -c
-              "${CMAKE_CXX_COMPILER} -E -x c++ -v /dev/null 2>&1 | \
-               awk '/> search starts here:/{f=1;next}/^End of search/{f=0}f{print $1}' \
-               > \"${_sapi_isystem}\""
-      COMMAND "${SAPI_PYTHON3_EXECUTABLE}" -B
-              "${SAPI_SOURCE_DIR}/sandboxed_api/tools/generator2/sapi_generator.py"
-              "--sapi_name=${_sapi_LIBRARY_NAME}"
-              "--sapi_out=${_sapi_gen_header}"
-              "--sapi_embed_dir=${_sapi_embed_dir}"
-              "--sapi_embed_name=${_sapi_embed_name}"
-              "--sapi_functions=${_sapi_funcs}"
-              "--sapi_ns=${_sapi_NAMESPACE}"
-              "--sapi_isystem=${_sapi_isystem}"
-              "--sapi_in=${_sapi_full_inputs}"
-      COMMENT "Generating interface"
-      VERBATIM
+    list(APPEND _sapi_generator_command
+      "${SAPI_PYTHON3_EXECUTABLE}" -B
+      "${SAPI_SOURCE_DIR}/sandboxed_api/tools/generator2/sapi_generator.py"
+      ${_sapi_generator_args}
+      "--sapi_in=${_sapi_full_inputs}"
     )
   endif()
+  add_custom_command(
+    OUTPUT "${_sapi_gen_header}" "${_sapi_isystem}"
+    COMMAND sh -c
+            "${CMAKE_CXX_COMPILER} -E -x c++ -v /dev/null 2>&1 | \
+             awk '/> search starts here:/{f=1;next}/^End of search/{f=0}f{print $1}' \
+             > \"${_sapi_isystem}\""
+    COMMAND ${_sapi_generator_command}
+    COMMENT "Generating interface"
+    DEPENDS ${_sapi_INPUTS}
+    VERBATIM
+  )
 
   # Library with the interface
   if(NOT _sapi_SOURCES)

sandboxed_api/tools/clang_generator/CMakeLists.txt

@@ -45,6 +45,7 @@ target_link_libraries(sapi_generator PUBLIC
   absl::memory
   absl::random_random
   absl::status
+  absl::statusor
   absl::strings
   clangFormat
   clangFrontendTool
@@ -59,12 +60,15 @@ add_executable(sapi_generator_tool
 )
 target_link_libraries(sapi_generator_tool PRIVATE
   sapi::base
+  sandbox2::file_helpers
   sandbox2::fileops
   sapi::generator
 )
 
 if(SAPI_ENABLE_TESTS)
   add_executable(sapi_generator_test
+    frontend_action_test_util.cc
+    frontend_action_test_util.h
     emitter_test.cc
   )
   target_link_libraries(sapi_generator_test PRIVATE

sandboxed_api/tools/clang_generator/emitter.cc

@@ -23,9 +23,13 @@
 #include "absl/strings/str_format.h"
 #include "absl/strings/str_join.h"
 #include "absl/strings/str_replace.h"
+#include "absl/strings/string_view.h"
 #include "absl/strings/strip.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/Type.h"
+#include "clang/Format/Format.h"
 #include "sandboxed_api/tools/clang_generator/diagnostics.h"
+#include "sandboxed_api/tools/clang_generator/generator.h"
 #include "sandboxed_api/util/status_macros.h"
 
 namespace sapi {
@@ -106,6 +110,30 @@ constexpr absl::string_view kClassFooterTemplate = R"(
 };
 )";
 
+namespace internal {
+
+absl::StatusOr<std::string> ReformatGoogleStyle(const std::string& filename,
+                                                const std::string& code) {
+  // Configure code style based on Google style, but enforce pointer alignment
+  clang::format::FormatStyle style =
+      clang::format::getGoogleStyle(clang::format::FormatStyle::LK_Cpp);
+  style.DerivePointerAlignment = false;
+  style.PointerAlignment = clang::format::FormatStyle::PAS_Left;
+
+  clang::tooling::Replacements replacements = clang::format::reformat(
+      style, code, llvm::makeArrayRef(clang::tooling::Range(0, code.size())),
+      filename);
+
+  llvm::Expected<std::string> formatted_header =
+      clang::tooling::applyAllReplacements(code, replacements);
+  if (!formatted_header) {
+    return absl::InternalError(llvm::toString(formatted_header.takeError()));
+  }
+  return *formatted_header;
+}
+
+}  // namespace internal
+
 std::string GetIncludeGuard(absl::string_view filename) {
   if (filename.empty()) {
     static auto* bit_gen = new absl::BitGen();
@@ -250,7 +278,8 @@ absl::StatusOr<std::string> EmitFunction(const clang::FunctionDecl* decl) {
 }
 
 absl::StatusOr<std::string> EmitHeader(
-    std::vector<clang::FunctionDecl*> functions, const QualTypeSet& types,
+    const std::vector<std::string>& functions,
+    const Emitter::RenderedTypesMap& rendered_types,
     const GeneratorOptions& options) {
   std::string out;
   const std::string include_guard = GetIncludeGuard(options.out_file);
@@ -276,45 +305,19 @@ absl::StatusOr<std::string> EmitHeader(
   }
 
   // Emit type dependencies
-  // TODO(cblichmann): Coalesce namespaces
-  std::string out_types = "// Types this API depends on\n";
-  bool added_types = false;
-  for (const clang::QualType& qual : types) {
-    clang::TypeDecl* decl = nullptr;
-    if (const auto* typedef_type = qual->getAs<clang::TypedefType>()) {
-      decl = typedef_type->getDecl();
-    } else if (const auto* enum_type = qual->getAs<clang::EnumType>()) {
-      decl = enum_type->getDecl();
-    } else {
-      decl = qual->getAsRecordDecl();
-    }
-    if (!decl) {
-      continue;
-    }
-
-    const std::vector<std::string> ns_path = GetNamespacePath(decl);
-    std::string nested_ns_name;
-    if (!ns_path.empty()) {
-      if (const auto& ns_root = ns_path.front();
-          ns_root == "std" || ns_root == "sapi" || ns_root == "__gnu_cxx") {
-        // Filter out any and all declarations from the C++ standard library,
-        // from SAPI itself and from other well-known namespaces. This avoids
-        // re-declaring things like standard integer types, for example.
-        continue;
+  if (!rendered_types.empty()) {
+    absl::StrAppend(&out, "// Types this API depends on\n");
+    for (const auto& [ns_name, types] : rendered_types) {
+      if (!ns_name.empty()) {
+        absl::StrAppend(&out, "namespace ", ns_name, " {\n");
+      }
+      for (const auto& type : types) {
+        absl::StrAppend(&out, type, ";\n");
+      }
+      if (!ns_name.empty()) {
+        absl::StrAppend(&out, "}  // namespace ", ns_name, "\n\n");
       }
-      nested_ns_name = absl::StrCat(ns_path[0].empty() ? "" : " ",
-                                    absl::StrJoin(ns_path, "::"));
-      absl::StrAppend(&out_types, "namespace", nested_ns_name, " {\n");
     }
-    absl::StrAppend(&out_types, PrintAstDecl(decl), ";");
-    if (!ns_path.empty()) {
-      absl::StrAppend(&out_types, "\n}  // namespace", nested_ns_name);
-    }
-    absl::StrAppend(&out_types, "\n");
-    added_types = true;
-  }
-  if (added_types) {
-    absl::StrAppend(&out, out_types);
   }
 
   // Optionally emit a default sandbox that instantiates an embedded sandboxee
@@ -329,11 +332,7 @@ absl::StatusOr<std::string> EmitHeader(
   // TODO(cblichmann): Make the "Api" suffix configurable or at least optional.
   absl::StrAppendFormat(&out, kClassHeaderTemplate,
                         absl::StrCat(options.name, "Api"));
-  std::string out_func;
-  for (const clang::FunctionDecl* decl : functions) {
-    SAPI_ASSIGN_OR_RETURN(out_func, EmitFunction(decl));
-    absl::StrAppend(&out, out_func);
-  }
+  absl::StrAppend(&out, absl::StrJoin(functions, "\n"));
   absl::StrAppend(&out, kClassFooterTemplate);
 
   // Close out the header: close namespace (if needed) and end include guard
@@ -344,4 +343,45 @@ absl::StatusOr<std::string> EmitHeader(
   return out;
 }
 
+void Emitter::CollectType(clang::QualType qual) {
+  clang::TypeDecl* decl = nullptr;
+  if (const auto* typedef_type = qual->getAs<clang::TypedefType>()) {
+    decl = typedef_type->getDecl();
+  } else if (const auto* enum_type = qual->getAs<clang::EnumType>()) {
+    decl = enum_type->getDecl();
+  } else {
+    decl = qual->getAsRecordDecl();
+  }
+  if (!decl) {
+    return;
+  }
+
+  const std::vector<std::string> ns_path = GetNamespacePath(decl);
+  std::string ns_name;
+  if (!ns_path.empty()) {
+    if (const auto& ns_root = ns_path.front();
+        ns_root == "std" || ns_root == "sapi" || ns_root == "__gnu_cxx") {
+      // Filter out any and all declarations from the C++ standard library,
+      // from SAPI itself and from other well-known namespaces. This avoids
+      // re-declaring things like standard integer types, for example.
+      return;
+    }
+    ns_name = absl::StrCat(ns_path[0].empty() ? "" : " ",
+                           absl::StrJoin(ns_path, "::"));
+  }
+
+  rendered_types_[ns_name].push_back(PrintAstDecl(decl));
+}
+
+void Emitter::CollectFunction(clang::FunctionDecl* decl) {
+  functions_.push_back(*EmitFunction(decl));  // Cannot currently fail
+}
+
+absl::StatusOr<std::string> Emitter::EmitHeader(
+    const GeneratorOptions& options) {
+  SAPI_ASSIGN_OR_RETURN(const std::string header,
+                   ::sapi::EmitHeader(functions_, rendered_types_, options));
+  return internal::ReformatGoogleStyle(options.out_file, header);
+}
+
 }  // namespace sapi

sandboxed_api/tools/clang_generator/emitter.h

@@ -16,16 +16,44 @@
 #define SANDBOXED_API_TOOLS_CLANG_GENERATOR_EMITTER_H_
 
 #include <string>
+#include <vector>
 
+#include "absl/container/flat_hash_map.h"
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Type.h"
-#include "sandboxed_api/tools/clang_generator/generator.h"
 #include "sandboxed_api/tools/clang_generator/types.h"
 
 namespace sapi {
+namespace internal {
+
+absl::StatusOr<std::string> ReformatGoogleStyle(const std::string& filename,
+                                                const std::string& code);
+
+}  // namespace internal
+
+class GeneratorOptions;
+
+class Emitter {
+ public:
+  using RenderedTypesMap =
+      absl::flat_hash_map<std::string, std::vector<std::string>>;
+
+  void CollectType(clang::QualType qual);
+  void CollectFunction(clang::FunctionDecl* decl);
+
+  // Outputs a formatted header for a list of functions and their related types.
+  absl::StatusOr<std::string> EmitHeader(const GeneratorOptions& options);
+
+ protected:
+  // Maps namespace to a list of spellings for types
+  RenderedTypesMap rendered_types_;
+
+  // Functions for sandboxed API, including their bodies
+  std::vector<std::string> functions_;
+};
 
 // Constructs an include guard name for the given filename. The name is of the
 // same form as the include guards in this project.
@@ -35,11 +63,6 @@ namespace sapi {
 //   SANDBOXED_API_EXAMPLES_ZLIB_ZLIB_SAPI_SAPI_H_
 std::string GetIncludeGuard(absl::string_view filename);
 
-// Outputs a formatted header for a list of functions and their related types.
-absl::StatusOr<std::string> EmitHeader(
-    std::vector<clang::FunctionDecl*> functions, const QualTypeSet& types,
-    const GeneratorOptions& options);
-
 }  // namespace sapi
 
 #endif  // SANDBOXED_API_TOOLS_CLANG_GENERATOR_EMITTER_H_

sandboxed_api/tools/clang_generator/emitter_test.cc

@@ -14,18 +14,48 @@
 
 #include "sandboxed_api/tools/clang_generator/emitter.h"
 
+#include <initializer_list>
+
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include "absl/memory/memory.h"
 #include "absl/strings/string_view.h"
+#include "sandboxed_api/sandbox2/testing.h"
+#include "sandboxed_api/tools/clang_generator/frontend_action_test_util.h"
+#include "sandboxed_api/tools/clang_generator/generator.h"
 #include "sandboxed_api/util/status_matchers.h"
 
 namespace sapi {
 namespace {
 
 using ::testing::MatchesRegex;
+using ::testing::SizeIs;
 using ::testing::StrEq;
 using ::testing::StrNe;
 
+class EmitterForTesting : public Emitter {
+ public:
+  using Emitter::functions_;
+};
+
+class EmitterTest : public FrontendActionTest {};
+
+TEST_F(EmitterTest, BasicFunctionality) {
+  GeneratorOptions options;
+  options.out_file = "input.h";
+  options.set_function_names<std::initializer_list<std::string>>(
+      {"ExposedFunction"});
+
+  EmitterForTesting emitter;
+  RunFrontendAction(R"(extern "C" void ExposedFunction() {})",
+                    absl::make_unique<GeneratorAction>(emitter, options));
+
+  EXPECT_THAT(emitter.functions_, SizeIs(1));
+
+  absl::StatusOr<std::string> header = emitter.EmitHeader(options);
+  EXPECT_THAT(header, IsOk());
+}
+
 TEST(IncludeGuard, CreatesRandomizedGuardForEmptyFilename) {
   // Copybara will transform the string. This is intentional.
   constexpr absl::string_view kGeneratedHeaderPrefix =

sandboxed_api/tools/clang_generator/frontend_action_test_util.cc

@@ -0,0 +1,70 @@
+// Copyright 2020 google llc
+//
+// licensed under the apache license, version 2.0 (the "license");
+// you may not use this file except in compliance with the license.
+// you may obtain a copy of the license at
+//
+//     http://www.apache.org/licenses/license-2.0
+//
+// unless required by applicable law or agreed to in writing, software
+// distributed under the license is distributed on an "as is" basis,
+// without warranties or conditions of any kind, either express or implied.
+// see the license for the specific language governing permissions and
+// limitations under the license.
+
+#include "sandboxed_api/tools/clang_generator/frontend_action_test_util.h"
+
+#include <vector>
+
+#include "absl/status/status.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
+
+namespace sapi {
+namespace internal {
+
+absl::Status RunClangTool(
+    const std::vector<std::string> command_line,
+    const absl::flat_hash_map<std::string, std::string> file_contents,
+    std::unique_ptr<clang::FrontendAction> action) {
+  // Setup an in-memory virtual filesystem
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> fs(
+      new llvm::vfs::InMemoryFileSystem());
+  llvm::IntrusiveRefCntPtr<clang::FileManager> files =
+      new clang::FileManager(clang::FileSystemOptions(), fs);
+
+  for (const auto& [filename, content] : file_contents) {
+    if (!fs->addFile(filename, /*ModificationTime=*/0,
+                     llvm::MemoryBuffer::getMemBuffer(content))) {
+      return absl::UnknownError(
+          absl::StrCat("Couldn't add file to in-memory VFS: ", filename));
+    }
+  }
+
+#if LLVM_VERSION_MAJOR >= 10
+  clang::tooling::ToolInvocation invocation(command_line, std::move(action),
+                                            files.get());
+#else
+  clang::tooling::ToolInvocation invocation(command_line, action.get(),
+                                            files.get());
+#endif
+  if (!invocation.run()) {
+    return absl::UnknownError("Tool invocation failed");
+  }
+  return absl::OkStatus();
+}
+
+}  // namespace internal
+
+std::vector<std::string> FrontendActionTest::GetCommandLineFlagsForTesting(
+    absl::string_view input_file) {
+  return {"tool", "-fsyntax-only", "--std=c++17",
+          "-I.",  "-Wno-error",    std::string(input_file)};
+}
+
+}  // namespace sapi

sandboxed_api/tools/clang_generator/frontend_action_test_util.h

@@ -0,0 +1,83 @@
+// Copyright 2020 google llc
+//
+// licensed under the apache license, version 2.0 (the "license");
+// you may not use this file except in compliance with the license.
+// you may obtain a copy of the license at
+//
+//     http://www.apache.org/licenses/license-2.0
+//
+// unless required by applicable law or agreed to in writing, software
+// distributed under the license is distributed on an "as is" basis,
+// without warranties or conditions of any kind, either express or implied.
+// see the license for the specific language governing permissions and
+// limitations under the license.
+
+#ifndef SANDBOXED_API_TOOLS_CLANG_GENERATOR_FRONTEND_ACTION_TEST_UTIL_H_
+#define SANDBOXED_API_TOOLS_CLANG_GENERATOR_FRONTEND_ACTION_TEST_UTIL_H_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "absl/container/flat_hash_map.h"
+#include "absl/memory/memory.h"
+#include "absl/status/status.h"
+#include "absl/strings/str_cat.h"
+#include "absl/strings/string_view.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "sandboxed_api/util/status_matchers.h"
+
+namespace sapi {
+namespace internal {
+
+absl::Status RunClangTool(
+    const std::vector<std::string> command_line,
+    const absl::flat_hash_map<std::string, std::string> file_contents,
+    std::unique_ptr<clang::FrontendAction> action);
+
+}  // namespace internal
+
+class FrontendActionTest : public ::testing::Test {
+ protected:
+  // Adds code to a virtual filesystem with the given filename.
+  void AddCode(const std::string& filename, absl::string_view code) {
+    absl::StrAppend(&file_contents_[filename], code);
+  }
+
+  // Changes the name of the virtual input file. Useful for special cases where
+  // the filenames of compiled sources matter.
+  void set_input_file(absl::string_view value) {
+    input_file_ = std::string(value);
+  }
+
+  virtual std::vector<std::string> GetCommandLineFlagsForTesting(
+      absl::string_view input_file);
+
+  // Runs the specified frontend action on in-memory source code.
+  void RunFrontendAction(absl::string_view code,
+                         std::unique_ptr<clang::FrontendAction> action) {
+    std::vector<std::string> command_line =
+        GetCommandLineFlagsForTesting(input_file_);
+    AddCode(input_file_, code);
+    ASSERT_THAT(
+        internal::RunClangTool(command_line, file_contents_, std::move(action)),
+        IsOk());
+  }
+
+  // Runs the specified frontend action. Provided for compatibility with LLVM <
+  // 10. Takes ownership.
+  void RunFrontendAction(absl::string_view code,
+                         clang::FrontendAction* action) {
+    RunFrontendAction(code, absl::WrapUnique(action));
+  }
+
+ private:
+  std::string input_file_ = "input.cc";
+  absl::flat_hash_map<std::string, std::string> file_contents_;
+};
+
+}  // namespace sapi
+
+#endif  // SANDBOXED_API_TOOLS_CLANG_GENERATOR_FRONTEND_ACTION_TEST_UTIL_H_

sandboxed_api/tools/clang_generator/generator.cc

@@ -19,6 +19,7 @@
 
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
+#include "clang/AST/Type.h"
 #include "clang/Format/Format.h"
 #include "sandboxed_api/sandbox2/util/fileops.h"
 #include "sandboxed_api/tools/clang_generator/diagnostics.h"
@@ -39,88 +40,46 @@ std::string ReplaceFileExtension(absl::string_view path,
   return absl::StrCat(path.substr(0, pos), new_extension);
 }
 
-std::string GetOutputFilename(absl::string_view source_file) {
-  return ReplaceFileExtension(source_file, ".sapi.h");
-}
-
 inline absl::string_view ToStringView(llvm::StringRef ref) {
   return absl::string_view(ref.data(), ref.size());
 }
 
 }  // namespace
 
+std::string GetOutputFilename(absl::string_view source_file) {
+  return ReplaceFileExtension(source_file, ".sapi.h");
+}
+
 bool GeneratorASTVisitor::VisitFunctionDecl(clang::FunctionDecl* decl) {
   if (!decl->isCXXClassMember() &&  // Skip classes
       decl->isExternC() &&          // Skip non external functions
       !decl->isTemplated() &&       // Skip function templates
       // Process either all function or just the requested ones
-      (options_->function_names.empty() ||
-       options_->function_names.count(ToStringView(decl->getName())) > 0)) {
+      (options_.function_names.empty() ||
+       options_.function_names.count(ToStringView(decl->getName())) > 0)) {
     functions_.push_back(decl);
-    GatherRelatedTypes(decl->getDeclaredReturnType(), &types_);
+
+    CollectRelatedTypes(decl->getDeclaredReturnType(), &types_);
     for (const clang::ParmVarDecl* param : decl->parameters()) {
-      GatherRelatedTypes(param->getType(), &types_);
+      CollectRelatedTypes(param->getType(), &types_);
     }
   }
   return true;
 }
 
-namespace internal {
-
-absl::StatusOr<std::string> ReformatGoogleStyle(const std::string& filename,
-                                                const std::string& code) {
-  // Configure code style based on Google style, but enforce pointer alignment
-  clang::format::FormatStyle style =
-      clang::format::getGoogleStyle(clang::format::FormatStyle::LK_Cpp);
-  style.DerivePointerAlignment = false;
-  style.PointerAlignment = clang::format::FormatStyle::PAS_Left;
-
-  clang::tooling::Replacements replacements = clang::format::reformat(
-      style, code, llvm::makeArrayRef(clang::tooling::Range(0, code.size())),
-      filename);
-
-  llvm::Expected<std::string> formatted_header =
-      clang::tooling::applyAllReplacements(code, replacements);
-  if (!formatted_header) {
-    return absl::InternalError(llvm::toString(formatted_header.takeError()));
-  }
-  return *formatted_header;
-}
-
-}  // namespace internal
-
-absl::Status GeneratorASTConsumer::GenerateAndSaveHeader() {
-  const std::string out_file =
-      options_->out_file.empty() ? GetOutputFilename(in_file_)
-                                 : sandbox2::file_util::fileops::MakeAbsolute(
-                                       options_->out_file, options_->work_dir);
-
-  SAPI_ASSIGN_OR_RETURN(const std::string header,
-                   EmitHeader(visitor_.functions_, visitor_.types_, *options_));
-  SAPI_ASSIGN_OR_RETURN(const std::string formatted_header,
-                   internal::ReformatGoogleStyle(in_file_, header));
-
-  std::ofstream os(out_file, std::ios::out | std::ios::trunc);
-  os << formatted_header;
-  if (!os) {
-    return absl::UnknownError("I/O error");
-  }
-  return absl::OkStatus();
-}
-
 void GeneratorASTConsumer::HandleTranslationUnit(clang::ASTContext& context) {
-  absl::Status status;
+  std::cout << "Processing " << in_file_ << "\n";
   if (!visitor_.TraverseDecl(context.getTranslationUnitDecl())) {
-    status = absl::InternalError("AST traversal exited early");
-  } else {
-    status = GenerateAndSaveHeader();
+    ReportFatalError(context.getDiagnostics(),
+                     context.getTranslationUnitDecl()->getBeginLoc(),
+                     "AST traversal exited early");
   }
 
-  if (!status.ok()) {
-    ReportFatalError(context.getDiagnostics(),
-                     GetDiagnosticLocationFromStatus(status).value_or(
-                         context.getTranslationUnitDecl()->getBeginLoc()),
-                     status.message());
+  for (clang::QualType qual : visitor_.types_) {
+    emitter_.CollectType(qual);
+  }
+  for (clang::FunctionDecl* func : visitor_.functions_) {
+    emitter_.CollectFunction(func);
   }
 }
 

sandboxed_api/tools/clang_generator/generator.h

@@ -26,6 +26,7 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Tooling/Tooling.h"
+#include "sandboxed_api/tools/clang_generator/emitter.h"
 #include "sandboxed_api/tools/clang_generator/types.h"
 
 namespace sapi {
@@ -46,85 +47,86 @@ struct GeneratorOptions {
   std::string work_dir;
   std::string name;            // Name of the Sandboxed API
   std::string namespace_name;  // Namespace to wrap the SAPI in
-  std::string out_file;        // Output path of the generated header
-  std::string embed_dir;       // Directory with embedded includes
-  std::string embed_name;      // Identifier of the embed object
+  // Output path of the generated header. Used to build the header include
+  // guard.
+  std::string out_file;
+  std::string embed_dir;   // Directory with embedded includes
+  std::string embed_name;  // Identifier of the embed object
 };
 
 class GeneratorASTVisitor
     : public clang::RecursiveASTVisitor<GeneratorASTVisitor> {
  public:
+  explicit GeneratorASTVisitor(const GeneratorOptions& options)
+      : options_(options) {}
+
   bool VisitFunctionDecl(clang::FunctionDecl* decl);
 
  private:
   friend class GeneratorASTConsumer;
-  const GeneratorOptions* options_ = nullptr;
 
   std::vector<clang::FunctionDecl*> functions_;
   QualTypeSet types_;
+
+  const GeneratorOptions& options_;
 };
 
-namespace internal {
-
-absl::StatusOr<std::string> ReformatGoogleStyle(const std::string& filename,
-                                                const std::string& code);
-
-}  // namespace internal
-
 class GeneratorASTConsumer : public clang::ASTConsumer {
  public:
-  GeneratorASTConsumer(std::string in_file, const GeneratorOptions* options)
-      : in_file_(std::move(in_file)), options_(options) {
-    visitor_.options_ = options_;
-  }
+  GeneratorASTConsumer(std::string in_file, Emitter& emitter,
+                       const GeneratorOptions& options)
+      : in_file_(std::move(in_file)), visitor_(options), emitter_(emitter) {}
 
  private:
   void HandleTranslationUnit(clang::ASTContext& context) override;
 
-  absl::Status GenerateAndSaveHeader();
-
   std::string in_file_;
-  const GeneratorOptions* options_;
-
   GeneratorASTVisitor visitor_;
+
+  Emitter& emitter_;
 };
 
 class GeneratorAction : public clang::ASTFrontendAction {
  public:
-  explicit GeneratorAction(const GeneratorOptions* options)
-      : options_(options) {}
+  GeneratorAction(Emitter& emitter, const GeneratorOptions& options)
+      : emitter_(emitter), options_(options) {}
 
  private:
   std::unique_ptr<clang::ASTConsumer> CreateASTConsumer(
       clang::CompilerInstance&, llvm::StringRef in_file) override {
     return absl::make_unique<GeneratorASTConsumer>(std::string(in_file),
-                                                   options_);
+                                                   emitter_, options_);
   }
 
   bool hasCodeCompletionSupport() const override { return false; }
 
-  const GeneratorOptions* options_;
+  Emitter& emitter_;
+  const GeneratorOptions& options_;
 };
 
 class GeneratorFactory : public clang::tooling::FrontendActionFactory {
  public:
-  explicit GeneratorFactory(GeneratorOptions options = {})
-      : options_(std::move(options)) {}
+  // Does not take ownership
+  GeneratorFactory(Emitter& emitter, const GeneratorOptions& options)
+      : emitter_(emitter), options_(options) {}
 
  private:
 #if LLVM_VERSION_MAJOR >= 10
   std::unique_ptr<clang::FrontendAction> create() override {
-    return absl::make_unique<GeneratorAction>(&options_);
+    return absl::make_unique<GeneratorAction>(emitter_, options_);
   }
 #else
   clang::FrontendAction* create() override {
-    return new GeneratorAction(&options_);
+    return new GeneratorAction(emitter_, options_);
   }
 #endif
 
-  GeneratorOptions options_;
+  Emitter& emitter_;
+  const GeneratorOptions& options_;
 };
 
+std::string GetOutputFilename(absl::string_view source_file);
+
 }  // namespace sapi
 
 #endif  // SANDBOXED_API_TOOLS_CLANG_GENERATOR_GENERATOR_H_

sandboxed_api/tools/clang_generator/generator_tool.cc

@@ -12,16 +12,23 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <cstdlib>
 #include <string>
 #include <vector>
 
 #include "absl/memory/memory.h"
+#include "absl/status/status.h"
+#include "absl/strings/str_format.h"
+#include "absl/strings/str_split.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Frontend/FrontendActions.h"
+#include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "llvm/Support/CommandLine.h"
+#include "sandboxed_api/sandbox2/util/file_helpers.h"
 #include "sandboxed_api/sandbox2/util/fileops.h"
 #include "sandboxed_api/tools/clang_generator/generator.h"
+#include "sandboxed_api/util/status_macros.h"
 
 namespace sapi {
 namespace {
@@ -49,11 +56,11 @@ static auto* g_sapi_functions = new llvm::cl::list<std::string>(
     llvm::cl::cat(*g_tool_category));
 static auto* g_sapi_in = new llvm::cl::list<std::string>(
     "sapi_in", llvm::cl::CommaSeparated,
-    llvm::cl::desc("List of input files to analyze. Deprecated, use positional "
-                   "arguments instead."),
+    llvm::cl::desc("List of input files to analyze."),
     llvm::cl::cat(*g_tool_category));
 static auto* g_sapi_isystem = new llvm::cl::opt<std::string>(
-    "sapi_isystem", llvm::cl::desc("Extra system include paths"),
+    "sapi_isystem",
+    llvm::cl::desc("Parameter file with extra system include paths"),
     llvm::cl::cat(*g_tool_category));
 static auto* g_sapi_limit_scan_depth = new llvm::cl::opt<bool>(
     "sapi_limit_scan_depth",
@@ -69,28 +76,28 @@ static auto* g_sapi_ns = new llvm::cl::opt<std::string>(
 static auto* g_sapi_out = new llvm::cl::opt<std::string>(
     "sapi_out",
     llvm::cl::desc(
-        "Ouput path of the generated header. If empty, simply appends .sapi.h "
+        "Output path of the generated header. If empty, simply appends .sapi.h "
         "to the basename of the first source file specified."),
     llvm::cl::cat(*g_tool_category));
 
 }  // namespace
 
-GeneratorOptions GeneratorOptionsFromFlags() {
+GeneratorOptions GeneratorOptionsFromFlags(
+    const std::vector<std::string>& sources) {
   GeneratorOptions options;
   options.function_names.insert(g_sapi_functions->begin(),
                                 g_sapi_functions->end());
   options.work_dir = sandbox2::file_util::fileops::GetCWD();
   options.name = *g_sapi_name;
   options.namespace_name = *g_sapi_ns;
-  options.out_file = *g_sapi_out;
+  options.out_file =
+      !g_sapi_out->empty() ? *g_sapi_out : GetOutputFilename(sources.front());
   options.embed_dir = *g_sapi_embed_dir;
   options.embed_name = *g_sapi_embed_name;
   return options;
 }
 
-}  // namespace sapi
-
-int main(int argc, const char** argv) {
+absl::Status GeneratorMain(int argc, const char** argv) {
   clang::tooling::CommonOptionsParser opt_parser(
       argc, argv, *sapi::g_tool_category, llvm::cl::ZeroOrMore,
       "Generates a Sandboxed API header for C/C++ translation units.");
@@ -98,9 +105,45 @@ int main(int argc, const char** argv) {
   for (const auto& sapi_in : *sapi::g_sapi_in) {
     sources.push_back(sapi_in);
   }
+  if (sources.empty()) {
+    return absl::InvalidArgumentError("error: no input files");
+  }
+
+  auto options = sapi::GeneratorOptionsFromFlags(sources);
+  sapi::Emitter emitter;
 
   clang::tooling::ClangTool tool(opt_parser.getCompilations(), sources);
-  return tool.run(absl::make_unique<sapi::GeneratorFactory>(
-                      sapi::GeneratorOptionsFromFlags())
-                      .get());
+  if (!sapi::g_sapi_isystem->empty()) {
+    std::string isystem_lines;
+    SAPI_RETURN_IF_ERROR(sandbox2::file::GetContents(
+        *sapi::g_sapi_isystem, &isystem_lines, sandbox2::file::Defaults()));
+    std::vector<std::string> isystem =
+        absl::StrSplit(isystem_lines, '\n', absl::SkipWhitespace());
+    for (std::string& line : isystem) {
+      line.insert(0, "-isystem");
+    }
+    tool.appendArgumentsAdjuster(clang::tooling::getInsertArgumentAdjuster(
+        isystem, clang::tooling::ArgumentInsertPosition::BEGIN));
+  }
+  if (int result = tool.run(
+          absl::make_unique<sapi::GeneratorFactory>(emitter, options).get());
+      result != 0) {
+    return absl::UnknownError("header generation failed");
+  }
+
+  SAPI_ASSIGN_OR_RETURN(std::string header, emitter.EmitHeader(options));
+
+  SAPI_RETURN_IF_ERROR(sandbox2::file::SetContents(options.out_file, header,
+                                              sandbox2::file::Defaults()));
+  return absl::OkStatus();
+}
+
+}  // namespace sapi
+
+int main(int argc, const char** argv) {
+  if (absl::Status status = sapi::GeneratorMain(argc, argv); !status.ok()) {
+    absl::FPrintF(stderr, "error: %s\n", status.message());
+    return EXIT_FAILURE;
+  }
+  return EXIT_SUCCESS;
 }

sandboxed_api/tools/clang_generator/types.cc

@@ -30,9 +30,9 @@ bool IsFunctionReferenceType(clang::QualType qual) {
 
 }  // namespace
 
-void GatherRelatedTypes(clang::QualType qual, QualTypeSet* types) {
+void CollectRelatedTypes(clang::QualType qual, QualTypeSet* types) {
   if (const auto* typedef_type = qual->getAs<clang::TypedefType>()) {
-    GatherRelatedTypes(typedef_type->getDecl()->getUnderlyingType(), types);
+    CollectRelatedTypes(typedef_type->getDecl()->getUnderlyingType(), types);
     types->insert(qual);
     return;
   }
@@ -43,9 +43,9 @@ void GatherRelatedTypes(clang::QualType qual, QualTypeSet* types) {
                                         ->getAs<clang::FunctionProtoType>()) {
       // Note: Do not add the function type itself, as this will always be a
       //       pointer argument. We only need to collect all its related types.
-      GatherRelatedTypes(function_type->getReturnType(), types);
+      CollectRelatedTypes(function_type->getReturnType(), types);
       for (const clang::QualType& param : function_type->getParamTypes()) {
-        GatherRelatedTypes(param, types);
+        CollectRelatedTypes(param, types);
       }
       return;
     }
@@ -56,13 +56,13 @@ void GatherRelatedTypes(clang::QualType qual, QualTypeSet* types) {
     while (IsPointerOrReference(pointee)) {
       pointee = pointee->getPointeeType();
     }
-    GatherRelatedTypes(pointee, types);
+    CollectRelatedTypes(pointee, types);
     return;
   }
 
   // C array with specified constant size (i.e. int a[42])?
   if (const clang::ArrayType* array_type = qual->getAsArrayTypeUnsafe()) {
-    GatherRelatedTypes(array_type->getElementType(), types);
+    CollectRelatedTypes(array_type->getElementType(), types);
     return;
   }
 
@@ -71,7 +71,7 @@ void GatherRelatedTypes(clang::QualType qual, QualTypeSet* types) {
       // Collect the underlying integer type of enum classes as well, as it may
       // be a typedef.
       if (const clang::EnumDecl* decl = enum_type->getDecl(); decl->isFixed()) {
-        GatherRelatedTypes(decl->getIntegerType(), types);
+        CollectRelatedTypes(decl->getIntegerType(), types);
       }
     }
     types->insert(qual);
@@ -81,7 +81,7 @@ void GatherRelatedTypes(clang::QualType qual, QualTypeSet* types) {
   if (const auto* record_type = qual->getAs<clang::RecordType>()) {
     const clang::RecordDecl* decl = record_type->getDecl();
     for (const clang::FieldDecl* field : decl->fields()) {
-      GatherRelatedTypes(field->getType(), types);
+      CollectRelatedTypes(field->getType(), types);
     }
     types->insert(qual);
     return;

sandboxed_api/tools/clang_generator/types.h

@@ -54,7 +54,7 @@ inline bool IsPointerOrReference(clang::QualType qual) {
 //   int
 //   SubStruct
 //   bool
-void GatherRelatedTypes(clang::QualType qual, QualTypeSet* types);
+void CollectRelatedTypes(clang::QualType qual, QualTypeSet* types);
 
 // Maps a qualified type to a fully qualified SAPI-compatible type name. This
 // is used for the generated code that invokes the actual function call RPC.