clang_generator: Use fully qualified names

Use locally unqualified types to filter ordered type declarations in
`TypeCollector::GetTypeDeclarations()`. This is necessary, as
`clang::TypeName::getFullyQualifiedName()` and
`TypeDecl::getQualifiedNameAsString()` have different ideas which
qualifiers belong to the name. The former works on `QualType`s, while
the latter deals with the declaration directly. This change decays a
`TypeDecl` into its locally unqualified `QualType`.

PiperOrigin-RevId: 490500091
Change-Id: Ie2f4eece4e673f8b06ab6661d7b6611daf34fba9

sandboxed_api/tools/clang_generator/emitter.cc

@@ -25,9 +25,11 @@
 #include "absl/strings/str_split.h"
 #include "absl/strings/string_view.h"
 #include "absl/strings/strip.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/QualTypeNames.h"
 #include "clang/AST/Type.h"
 #include "clang/Format/Format.h"
 #include "sandboxed_api/tools/clang_generator/diagnostics.h"
@@ -200,15 +202,16 @@ std::string PrintRecordTemplateArguments(const clang::CXXRecordDecl* record) {
   if (!template_params) {
     return "";
   }
+  clang::ASTContext& context = record->getASTContext();
   std::vector<std::string> params;
   params.reserve(template_params->size());
   for (const auto& template_param : *template_params) {
     if (const auto* p =
             llvm::dyn_cast<clang::NonTypeTemplateParmDecl>(template_param)) {
-      // TODO(cblichmann): These types should be included by
-      //                   CollectRelatedTypes().
-      params.push_back(
-          p->getType().getDesugaredType(record->getASTContext()).getAsString());
+      // TODO(cblichmann): Should be included by CollectRelatedTypes().
+      params.push_back(clang::TypeName::getFullyQualifiedName(
+          p->getType().getDesugaredType(context), context,
+          context.getPrintingPolicy()));
     } else {  // Also covers template template parameters
       params.push_back("typename");
     }
@@ -294,6 +297,12 @@ absl::StatusOr<std::string> PrintFunctionPrototypeComment(
 }
 
 absl::StatusOr<std::string> EmitFunction(const clang::FunctionDecl* decl) {
+  const clang::QualType return_type = decl->getDeclaredReturnType();
+  if (return_type->isRecordType()) {
+    return MakeStatusWithDiagnostic(
+        decl->getBeginLoc(), absl::StatusCode::kCancelled,
+        "returning record by value, skipping function");
+  }
   std::string out;
 
   SAPI_ASSIGN_OR_RETURN(std::string prototype,
@@ -301,12 +310,6 @@ absl::StatusOr<std::string> EmitFunction(const clang::FunctionDecl* decl) {
   absl::StrAppend(&out, "\n", prototype);
 
   auto function_name = ToStringView(decl->getName());
-  const clang::QualType return_type = decl->getDeclaredReturnType();
-  if (return_type->isRecordType()) {
-    return MakeStatusWithDiagnostic(
-        decl->getBeginLoc(), absl::StatusCode::kCancelled,
-        "returning record by value, skipping function");
-  }
   const bool returns_void = return_type->isVoidType();
 
   const clang::ASTContext& context = decl->getASTContext();
@@ -368,7 +371,6 @@ absl::StatusOr<std::string> EmitHeader(
   std::string out;
   const std::string include_guard = GetIncludeGuard(options.out_file);
   absl::StrAppendFormat(&out, kHeaderProlog, include_guard);
-
   // When embedding the sandboxee, add embed header include
   if (!options.embed_name.empty()) {
     // Not using JoinPath() because even on Windows include paths use plain
@@ -441,8 +443,8 @@ void Emitter::EmitType(clang::TypeDecl* type_decl) {
 
   // Skip types defined in system headers.
   // TODO(cblichmann): Instead of this and the hard-coded entities below, we
-  //                   should map add the correct (system) headers to the
-  //                   generated output.
+  //                   should map types and add the correct (system) headers to
+  //                   the generated output.
   if (type_decl->getASTContext().getSourceManager().isInSystemHeader(
           type_decl->getBeginLoc())) {
     return;

sandboxed_api/tools/clang_generator/emitter_test.cc

@@ -21,7 +21,6 @@
 #include "gtest/gtest.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
-#include "sandboxed_api/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"
@@ -121,7 +120,8 @@ TEST_F(EmitterTest, CollectFunctionPointer) {
 
   EXPECT_THAT(
       UglifyAll(emitter.SpellingsForNS("")),
-      ElementsAre("struct HandlerData { int member; callback_t *cb; }"));
+      ElementsAre("typedef void (callback_t)(void *)",
+                  "struct HandlerData { int member; callback_t *cb; }"));
 }
 
 TEST_F(EmitterTest, TypedefNames) {
@@ -247,6 +247,25 @@ TEST_F(EmitterTest, TypedefStructByValueSkipsFunction) {
   EXPECT_THAT(emitter.GetRenderedFunctions(), IsEmpty());
 }
 
+TEST_F(EmitterTest, CollectTypedefPointerType) {
+  EmitterForTesting emitter;
+  EXPECT_THAT(
+      RunFrontendAction(
+          R"(typedef struct _KernelProfileRecord {
+               int member;
+             }* KernelProfileRecord;
+             extern "C" const KernelProfileRecord*
+             GetOpenCLKernelProfileRecords(const int, long long int*);)",
+          std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
+      IsOk());
+  EXPECT_THAT(emitter.GetRenderedFunctions(), SizeIs(1));
+
+  EXPECT_THAT(
+      UglifyAll(emitter.SpellingsForNS("")),
+      ElementsAre("struct _KernelProfileRecord { int member; }",
+                  "typedef struct _KernelProfileRecord *KernelProfileRecord"));
+}
+
 TEST_F(EmitterTest, TypedefTypeDependencies) {
   EmitterForTesting emitter;
   EXPECT_THAT(
@@ -268,6 +287,7 @@ TEST_F(EmitterTest, TypedefTypeDependencies) {
 
   EXPECT_THAT(UglifyAll(emitter.SpellingsForNS("")),
               ElementsAre("using size_t = long long", "struct _Image",
+                          "typedef struct _Image Image",
                           "typedef size_t (*StreamHandler)(const Image *, "
                           "const void *, const size_t)",
                           "struct _Image {"

sandboxed_api/tools/clang_generator/types.cc

@@ -110,6 +110,24 @@ void TypeCollector::CollectRelatedTypes(clang::QualType qual) {
   }
 }
 
+namespace {
+
+std::string GetQualTypeName(const clang::ASTContext& context,
+                            clang::QualType qual) {
+  // Remove any "const", "volatile", etc. except for those added via typedefs.
+  clang::QualType unqual = qual.getLocalUnqualifiedType();
+
+  // This is to get to the actual name of function pointers.
+  if (unqual->isFunctionPointerType() || IsFunctionReferenceType(unqual) ||
+      unqual->isMemberFunctionPointerType()) {
+    unqual = unqual->getPointeeType();
+  }
+  return clang::TypeName::getFullyQualifiedName(unqual, context,
+                                                context.getPrintingPolicy());
+}
+
+}  // namespace
+
 std::vector<clang::TypeDecl*> TypeCollector::GetTypeDeclarations() {
   if (ordered_decls_.empty()) {
     return {};
@@ -121,28 +139,33 @@ std::vector<clang::TypeDecl*> TypeCollector::GetTypeDeclarations() {
 
   absl::flat_hash_set<std::string> collected_names;
   for (clang::QualType qual : collected_) {
-    collected_names.insert(clang::TypeName::getFullyQualifiedName(
-        qual, context, context.getPrintingPolicy()));
+    const std::string qual_name = GetQualTypeName(context, qual);
+    collected_names.insert(qual_name);
   }
 
   std::vector<clang::TypeDecl*> result;
   for (clang::TypeDecl* type_decl : ordered_decls_) {
+    clang::QualType type_decl_type = context.getTypeDeclType(type_decl);
+
     // Ideally, collected_.contains() on the underlying QualType of the TypeDecl
     // would work here. However, QualTypes obtained from a TypeDecl contain
     // different Type pointers, even when referring to one of the same types
     // from the set and thus will not be found. Instead, work around the issue
     // by always using the fully qualified name of the type.
-    if (!collected_names.contains(type_decl->getQualifiedNameAsString())) {
+    const std::string qual_name = GetQualTypeName(context, type_decl_type);
+    if (!collected_names.contains(qual_name)) {
       continue;
     }
+
+    // Skip anonymous declarations that are typedef-ed. For example skip things
+    // like "typedef enum { A } SomeName". In this case, the enum is unnamed and
+    // the emitter will instead work with the complete typedef, so nothing is
+    // lost.
     if (auto* tag_decl = llvm::dyn_cast<clang::TagDecl>(type_decl);
         tag_decl && tag_decl->getTypedefNameForAnonDecl()) {
-      // Skip anonymous declarations that are typedef-ed. For example skip
-      // things like "typedef enum { A } SomeName". In this case, the enum is
-      // unnamed and the emitter will instead work with the complete typedef, so
-      // nothing is lost.
       continue;
     }
+
     result.push_back(type_decl);
   }
   return result;
@@ -151,10 +174,16 @@ std::vector<clang::TypeDecl*> TypeCollector::GetTypeDeclarations() {
 namespace {
 
 // Removes "const" from a qualified type if it denotes a pointer or reference
-// type.
+// type. Keeps top-level typedef types intact.
 clang::QualType MaybeRemoveConst(const clang::ASTContext& context,
                                  clang::QualType qual) {
-  if (IsPointerOrReference(qual)) {
+  if (
+#if LLVM_VERSION_MAJOR < 13
+      qual->getAs<clang::TypedefType>() == nullptr
+#else
+      !qual->isTypedefNameType()
+#endif
+      && IsPointerOrReference(qual)) {
     clang::QualType pointee_qual = qual->getPointeeType();
     pointee_qual.removeLocalConst();
     qual = context.getPointerType(pointee_qual);
@@ -253,18 +282,21 @@ std::string MapQualType(const clang::ASTContext& context,
     // Remove "const" qualifier from a pointer or reference type's pointee, as
     // e.g. const pointers do not work well with SAPI.
     return absl::StrCat("::sapi::v::Reg<",
-                        MaybeRemoveConst(context, qual).getAsString(), ">");
+                        clang::TypeName::getFullyQualifiedName(
+                            MaybeRemoveConst(context, qual), context,
+                            context.getPrintingPolicy()),
+                        ">");
   }
   // Best-effort mapping to "int", leave a comment.
-  return absl::StrCat("::sapi::v::Int /* aka '", qual.getAsString(), "' */");
+  return absl::StrCat("::sapi::v::Int /* aka '",
+                      clang::TypeName::getFullyQualifiedName(
+                          MaybeRemoveConst(context, qual), context,
+                          context.getPrintingPolicy()),
+                      "' */");
 }
 
 std::string MapQualTypeParameterForCxx(const clang::ASTContext& context,
                                        clang::QualType qual) {
-  if (const auto* typedef_type = qual->getAs<clang::TypedefType>()) {
-    clang::TypedefNameDecl* typedef_decl = typedef_type->getDecl();
-    return typedef_decl->getQualifiedNameAsString();
-  }
   if (const auto* builtin = qual->getAs<clang::BuiltinType>()) {
     if (builtin->getKind() == clang::BuiltinType::Bool) {
       return "bool";  // _Bool -> bool