clang_generator: Fail header generation with struct-by-value

The libclang based header generator disallows functions that pass structs (or
more generally "record types") by value. While this can be implemented, the
such functions as emitted by the clang_generator never worked.

We should revisit this when we implement support for passing 128-bit integer
types directly, as those will work the same as small structs.

PiperOrigin-RevId: 485522603
Change-Id: Iae8284720da52496d7a48fe3ca3c3c8605e6d19d

sandboxed_api/tools/clang_generator/emitter.cc

@@ -17,7 +17,6 @@
 #include "absl/random/random.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/ascii.h"
-#include "absl/strings/escaping.h"
 #include "absl/strings/match.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/str_format.h"
@@ -28,11 +27,11 @@
 #include "absl/strings/strip.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
-#include "clang/AST/PrettyPrinter.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/tools/clang_generator/types.h"
 #include "sandboxed_api/util/status_macros.h"
 
 namespace sapi {
@@ -298,6 +297,10 @@ absl::StatusOr<std::string> EmitFunction(const clang::FunctionDecl* decl) {
 
   auto function_name = ToStringView(decl->getName());
   const clang::QualType return_type = decl->getDeclaredReturnType();
+  if (return_type->isRecordType()) {
+    return MakeStatusWithDiagnostic(decl->getBeginLoc(),
+                                    "returning record by value");
+  }
   const bool returns_void = return_type->isVoidType();
 
   const clang::ASTContext& context = decl->getASTContext();
@@ -315,6 +318,12 @@ absl::StatusOr<std::string> EmitFunction(const clang::FunctionDecl* decl) {
   std::string print_separator;
   for (int i = 0; i < decl->getNumParams(); ++i) {
     const clang::ParmVarDecl* param = decl->getParamDecl(i);
+    if (param->getType()->isRecordType()) {
+      return MakeStatusWithDiagnostic(
+          param->getBeginLoc(),
+          absl::StrCat("passing record parameter '",
+                       ToStringView(param->getName()), "' by value"));
+    }
 
     ParameterInfo& param_info = params.emplace_back();
     param_info.qual = param->getType();
@@ -411,7 +420,7 @@ absl::StatusOr<std::string> EmitHeader(
   return out;
 }
 
-void Emitter::CollectType(clang::QualType qual) {
+void Emitter::EmitType(clang::QualType qual) {
   clang::TypeDecl* decl = nullptr;
   if (const auto* typedef_type = qual->getAs<clang::TypedefType>()) {
     decl = typedef_type->getDecl();
@@ -449,8 +458,16 @@ void Emitter::CollectType(clang::QualType qual) {
   rendered_types_[ns_name].push_back(GetSpelling(decl));
 }
 
-void Emitter::CollectFunction(clang::FunctionDecl* decl) {
+void Emitter::AddTypesFiltered(const QualTypeSet& types) {
-  functions_.push_back(*EmitFunction(decl));  // Cannot currently fail
+  for (clang::QualType qual : types) {
+    EmitType(qual);
+  }
+}
+
+absl::Status Emitter::AddFunction(clang::FunctionDecl* decl) {
+  SAPI_ASSIGN_OR_RETURN(std::string function, EmitFunction(decl));
+  functions_.push_back(function);
+  return absl::OkStatus();
 }
 
 absl::StatusOr<std::string> Emitter::EmitHeader(

sandboxed_api/tools/clang_generator/emitter.h

@@ -37,17 +37,29 @@ absl::StatusOr<std::string> ReformatGoogleStyle(const std::string& filename,
 
 class GeneratorOptions;
 
+// Responsible for emitting the actual textual representation of the generated
+// Sandboxed API header.
 class Emitter {
  public:
   using RenderedTypesMap =
       absl::btree_map<std::string, std::vector<std::string>>;
 
-  void CollectType(clang::QualType qual);
+  // Adds the set of previously collected types to the emitter, recording the
-  void CollectFunction(clang::FunctionDecl* decl);
+  // spelling of each one. Types that are not supported by the current
+  // generator settings or that are unwanted/unnecessary are skipped. Filtered
+  // types include C++ constructs or well-known standard library elements. The
+  // latter can be replaced by including the correct headers in the emitted
+  // header.
+  void AddTypesFiltered(const QualTypeSet& types);
+
+  absl::Status AddFunction(clang::FunctionDecl* decl);
 
   // Outputs a formatted header for a list of functions and their related types.
   absl::StatusOr<std::string> EmitHeader(const GeneratorOptions& options);
 
+ private:
+  void EmitType(clang::QualType qual);
+
  protected:
   // Maps namespace to a list of spellings for types
   RenderedTypesMap rendered_types_;
@@ -57,8 +69,8 @@ class Emitter {
 };
 
 // Constructs an include guard name for the given filename. The name is of the
-// same form as the include guards in this project.
+// same form as the include guards in this project and conforms to the Google
-// For example,
+// C++ style. For example,
 //   sandboxed_api/examples/zlib/zlib-sapi.sapi.h
 // will be mapped to
 //   SANDBOXED_API_EXAMPLES_ZLIB_ZLIB_SAPI_SAPI_H_

sandboxed_api/tools/clang_generator/emitter_test.cc

@@ -75,8 +75,7 @@ TEST_F(EmitterTest, RelatedTypes) {
                size_t width;
                size_t height;
              };
-             struct ByValue { int value; };
+             extern "C" void Colorize(Channel* chan);
-             extern "C" void Colorize(Channel* chan, ByValue v);
 
              typedef struct { int member; } MyStruct;
              extern "C" void Structize(MyStruct* s);)",
@@ -92,7 +91,6 @@ TEST_F(EmitterTest, RelatedTypes) {
                           " Color color;"
                           " size_t width;"
                           " size_t height; }",
-                          "struct ByValue { int value; }",
                           "typedef struct { int member; } MyStruct"));
 }
 
@@ -167,6 +165,36 @@ TEST_F(EmitterTest, RemoveQualifiers) {
               ElementsAre("struct A { int data; }"));
 }
 
+TEST_F(EmitterTest, StructByValueFails) {
+  EmitterForTesting emitter;
+  EXPECT_THAT(
+      RunFrontendAction(
+          R"(struct A { int data; };
+             extern "C" int Structize(A a);)",
+          std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
+      Not(IsOk()));
+}
+
+TEST_F(EmitterTest, ReturnStructByValueFails) {
+  EmitterForTesting emitter;
+  EXPECT_THAT(
+      RunFrontendAction(
+          R"(struct A { int data; };
+             extern "C" A Structize();)",
+          std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
+      Not(IsOk()));
+}
+
+TEST_F(EmitterTest, TypedefStructByValueFails) {
+  EmitterForTesting emitter;
+  EXPECT_THAT(
+      RunFrontendAction(
+          R"(typedef struct { int data; } A;
+             extern "C" int Structize(A a);)",
+          std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
+      Not(IsOk()));
+}
+
 TEST(IncludeGuard, CreatesRandomizedGuardForEmptyFilename) {
   // Copybara will transform the string. This is intentional.
   constexpr absl::string_view kGeneratedHeaderPrefix =

sandboxed_api/tools/clang_generator/generator.cc

@@ -18,14 +18,10 @@
 #include <iostream>
 
 #include "absl/status/status.h"
-#include "absl/status/statusor.h"
 #include "clang/AST/Type.h"
-#include "clang/Format/Format.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "sandboxed_api/tools/clang_generator/diagnostics.h"
 #include "sandboxed_api/tools/clang_generator/emitter.h"
-#include "sandboxed_api/util/fileops.h"
-#include "sandboxed_api/util/status_macros.h"
 
 namespace sapi {
 namespace {
@@ -84,11 +80,17 @@ void GeneratorASTConsumer::HandleTranslationUnit(clang::ASTContext& context) {
                      "AST traversal exited early");
   }
 
-  for (clang::QualType qual : visitor_.collector_.collected()) {
+  emitter_.AddTypesFiltered(visitor_.collector_.collected());
-    emitter_.CollectType(qual);
+
-  }
   for (clang::FunctionDecl* func : visitor_.functions_) {
-    emitter_.CollectFunction(func);
+    absl::Status status = emitter_.AddFunction(func);
+    if (!status.ok()) {
+      ReportFatalError(
+          context.getDiagnostics(),
+          GetDiagnosticLocationFromStatus(status).value_or(func->getBeginLoc()),
+          status.message());
+      break;
+    }
   }
 }
 

sandboxed_api/tools/clang_generator/types.cc

@@ -240,6 +240,8 @@ std::string MapQualTypeReturn(const clang::ASTContext& context,
     return "::absl::Status";
   }
   // Remove const qualifier like in MapQualType().
+  // TODO(cblichmann): We should return pointers differently, as they point to
+  //                   the sandboxee's address space.
   return absl::StrCat(
       "::absl::StatusOr<",
       MapQualTypeParameterForCxx(context, MaybeRemoveConst(context, qual)),

sandboxed_api/tools/clang_generator/types.h

@@ -29,7 +29,7 @@ using QualTypeSet =
     llvm::SetVector<clang::QualType, std::vector<clang::QualType>,
                     llvm::SmallPtrSet<clang::QualType, 8>>;
 
-// Returns whether a type is "simple". Simple types are arithemtic types,
+// Returns whether a type is "simple". Simple types are arithmetic types,
 // i.e. signed and unsigned integer, character and bool types, as well as
 // "void".
 inline bool IsSimple(clang::QualType qual) {
@@ -66,7 +66,7 @@ class TypeCollector {
 };
 
 // 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.
+// is used for the generated code that invokes the actual function call IPC.
 // If no mapping can be found, "int" is assumed.
 std::string MapQualType(const clang::ASTContext& context, clang::QualType qual);
 
@@ -77,8 +77,8 @@ std::string MapQualTypeParameter(const clang::ASTContext& context,
 
 // Maps a qualified type used as a function return type to a type name
 // compatible with the generated Sandboxed API. Uses MapQualTypeParameter() and
-// wraps the type in an absl::StatusOr<> if qual is non-void. Otherwise returns
+// wraps the type in an "absl::StatusOr<>" if qual is non-void. Otherwise
-// absl::Status.
+// returns "absl::Status".
 std::string MapQualTypeReturn(const clang::ASTContext& context,
                               clang::QualType qual);