emit fixes for improperly annotated symbols

Sources/idt/idt.cc

@@ -152,6 +152,7 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
   clang::ASTContext &context_;
   clang::SourceManager &source_manager_;
   std::optional<unsigned> id_unexported_;
+  std::optional<unsigned> id_improper_;
   std::optional<unsigned> id_exported_;
   PPCallbacks::FileIncludes &file_includes_;
 
@@ -228,6 +229,18 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
     return diagnostics_engine.Report(location, *id_unexported_);
   }
 
+  clang::DiagnosticBuilder
+  improperly_exported_interface(clang::SourceLocation location) {
+    clang::DiagnosticsEngine &diagnostics_engine = context_.getDiagnostics();
+
+    if (!id_improper_)
+      id_improper_ = diagnostics_engine.getCustomDiagID(
+          clang::DiagnosticsEngine::Remark,
+          "improperly exported symbol %0: %1");
+
+    return diagnostics_engine.Report(location, *id_improper_);
+  }
+
   clang::DiagnosticBuilder
   exported_private_interface(clang::SourceLocation location) {
     clang::DiagnosticsEngine &diagnostics_engine = context_.getDiagnostics();
@@ -280,9 +293,11 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
     if (const auto *VA = D->template getAttr<clang::VisibilityAttr>())
       return VA->getVisibility() == clang::VisibilityAttr::VisibilityType::Default;
 
-    if (llvm::isa<clang::RecordDecl>(D))
-      return false;
+    return false;
+  }
 
+  template <typename Decl_>
+  bool is_containing_record_exported(const Decl_ *D) const {
     // For non-record declarations, the DeclContext is the containing record.
     for (const clang::DeclContext *DC = D->getDeclContext(); DC; DC = DC->getParent())
       if (const auto *RD = llvm::dyn_cast<clang::RecordDecl>(DC))
@@ -291,13 +306,39 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
     return false;
   }
 
+  // Emit a FixIt if a symbol is annotated with a default visibility or DLL
+  // export/import annotation. The FixIt will remove the annotation
+  template <typename Decl_>
+  void check_symbol_not_exported(const Decl_ *D, const std::string &message) {
+    clang::SourceLocation SLoc;
+    if (const auto *VA = D->template getAttr<clang::DLLExportAttr>())
+      if (!VA->isInherited())
+        SLoc = VA->getLocation();
+
+    if (const auto *VA = D->template getAttr<clang::DLLImportAttr>())
+      if (!VA->isInherited())
+        SLoc = VA->getLocation();
+
+    if (const auto *VA = D->template getAttr<clang::VisibilityAttr>())
+      if (!VA->isInherited() &&
+          VA->getVisibility() == clang::VisibilityAttr::VisibilityType::Default)
+        SLoc = VA->getLocation();
+
+    if (SLoc.isInvalid())
+      return;
+
+    if (SLoc.isMacroID())
+      SLoc = source_manager_.getExpansionLoc(SLoc);
+
+    clang::CharSourceRange range =
+        clang::CharSourceRange::getTokenRange(SLoc, SLoc);
+    improperly_exported_interface(SLoc)
+        << D << message << clang::FixItHint::CreateRemoval(range);
+  }
+
   // Determine if a function needs exporting and add the export annotation as
   // required.
   void export_function_if_needed(const clang::FunctionDecl *FD) {
-    // Check if the symbol is already exported.
-    if (is_symbol_exported(FD))
-      return;
-
     // Ignore declarations from the system.
     if (is_in_system_header(FD))
       return;
@@ -306,41 +347,63 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
     if (!is_in_header(FD))
       return;
 
-    // We are only interested in non-dependent types.
-    if (FD->isDependentContext())
+    // Ignore friend declarations.
+    if (FD->getFriendObjectKind() != clang::Decl::FOK_None)
       return;
 
-    // If the function has a body, it can be materialized by the user.
-    if (FD->hasBody())
+    // Ignore known forward declarations (builtins)
+    if (contains(kIgnoredBuiltins, FD->getNameAsString()))
       return;
 
-    // Skip methods in template declarations.
-    if (FD->getTemplateInstantiationPattern())
+    // TODO(compnerd) replace with std::set::contains in C++20
+    if (contains(get_ignored_symbols(), FD->getNameAsString()))
       return;
 
-    // Ignore friend declarations.
-    if (FD->getFriendObjectKind() != clang::Decl::FOK_None)
+    // Skip functions contained in classes that are already exported.
+    if (is_containing_record_exported(FD)) {
+      // Exporting a symbol contained in an already exported class/struct will
+      // fail compilation on Windows.
+      check_symbol_not_exported(FD, "containing class is exported");
+      return;
+    }
+
+    // Skip any function defined inline, it can be materialized by the user.
+    if (FD->isThisDeclarationADefinition()) {
+      check_symbol_not_exported(FD, "function is defined inline");
       return;
+    }
+
+    // Pure virtual methods cannot be exported.
+    if (const auto *MD = llvm::dyn_cast<clang::CXXMethodDecl>(FD))
+      if (MD->isPureVirtual()) {
+        check_symbol_not_exported(FD, "pure virtual method");
+        return;
+      }
 
     // Ignore deleted and defaulted functions (e.g. operators).
     if (FD->isDeleted() || FD->isDefaulted())
       return;
 
+    // We are only interested in non-dependent types.
+    if (FD->isDependentContext())
+      return;
+
+    // Skip methods in template declarations.
+    if (FD->getTemplateInstantiationPattern())
+      return;
+
     // Skip template class template argument deductions.
     if (llvm::isa<clang::CXXDeductionGuideDecl>(FD))
       return;
 
-    // Pure virtual methods cannot be exported.
-    if (const auto *MD = llvm::dyn_cast<clang::CXXMethodDecl>(FD))
-      if (MD->isPureVirtual())
-        return;
-
-    // Ignore known forward declarations (builtins)
-    if (contains(kIgnoredBuiltins, FD->getNameAsString()))
+    // If the function has a body, it can be materialized by the user. This
+    // check is distinct from isThisDeclarationADefinition() because it will
+    // return true if the function has a body anywhere in the translation unit.
+    if (FD->hasBody())
       return;
 
-    // TODO(compnerd) replace with std::set::contains in C++20
-    if (contains(get_ignored_symbols(), FD->getNameAsString()))
+    // Check if the symbol is already exported.
+    if (is_symbol_exported(FD))
       return;
 
     // Use the inner start location so that the annotation comes after
@@ -360,10 +423,6 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
   // Determine if a variable needs exporting and add the export annotation as
   // required. This only applies to extern globals and static member fields.
   void export_variable_if_needed(const clang::VarDecl *VD) {
-    // Check if the symbol is already exported.
-    if (is_symbol_exported(VD))
-      return;
-
     // Ignore declarations from the system.
     if (is_in_system_header(VD))
       return;
@@ -376,18 +435,28 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
     if (VD->getParentFunctionOrMethod())
       return;
 
-    // Skip static fields that have initializers.
-    if (VD->hasInit())
+    // TODO(compnerd) replace with std::set::contains in C++20
+    if (contains(get_ignored_symbols(), VD->getNameAsString()))
       return;
 
+    // Skip variables that have initializers.
+    if (VD->hasInit()) {
+      check_symbol_not_exported(VD, "variable initialized at declaration");
+      return;
+    }
+
     // Skip all other local and global variables unless they are extern.
     if (!(VD->isStaticDataMember() ||
-          VD->getStorageClass() == clang::StorageClass::SC_Extern))
+          VD->getStorageClass() == clang::StorageClass::SC_Extern)) {
+      check_symbol_not_exported(VD, "variable not static or extern");
       return;
+    }
 
-    // Skip fields in template declarations.
-    if (VD->getTemplateInstantiationPattern() != nullptr)
+    // Skip variables contained in classes that are already exported.
+    if (is_containing_record_exported(VD)) {
+      check_symbol_not_exported(VD, "containing class is exported");
       return;
+    }
 
     // Skip static variables declared in template class unless the template is
     // fully specialized.
@@ -400,8 +469,12 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
           return;
     }
 
-    // TODO(compnerd) replace with std::set::contains in C++20
-    if (contains(get_ignored_symbols(), VD->getNameAsString()))
+    // Skip fields in template declarations.
+    if (VD->getTemplateInstantiationPattern() != nullptr)
+      return;
+
+    // Check if the symbol is already exported.
+    if (is_symbol_exported(VD))
       return;
 
     clang::SourceLocation SLoc = VD->getBeginLoc();

Tests/Negative.hh

@@ -0,0 +1,92 @@
+// NOTE: We expect idt to return a non-zero exit code on Windows because the
+// compiler will actually produce errors when __declspec(dllexport) appears in
+// unsupported locations (not the case for visibility attributes). So we have
+// to run "not idt" when running on Windows so the test doesn't fail due to
+// the non-zero exit code from idt.
+
+// RUN: %if system-windows %{ \
+// RUN:   not %idt --export-macro IDT_TEST_ABI %s 2>&1 | %FileCheck %s \
+// RUN: %} %else %{  \
+// RUN:   %idt --export-macro IDT_TEST_ABI %s 2>&1 | %FileCheck %s \
+// RUN: %}
+
+#if defined(_WIN32)
+#define IDS_TEST_ABI __declspec(dllexport)
+#else
+#define IDS_TEST_ABI __attribute__((visibility("default")))
+#endif
+
+// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'staticFunction'
+IDS_TEST_ABI static void staticFunction() {}
+
+// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'inlineFunction'
+IDS_TEST_ABI inline void inlineFunction() {}
+
+// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'function'
+IDS_TEST_ABI void function() {}
+
+// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'constVariable'
+IDS_TEST_ABI const int constVariable = 0;
+
+// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'staticConstVariable'
+IDS_TEST_ABI static const int staticConstVariable = 0;
+
+// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'constexprVariable'
+IDS_TEST_ABI constexpr int constexprVariable = 0;
+
+// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'staticConstexprVariable'
+IDS_TEST_ABI static constexpr int staticConstexprVariable = 0;
+
+// CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
+struct Struct {
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'Struct'
+  IDS_TEST_ABI Struct() = default;
+
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'Struct'
+  IDS_TEST_ABI Struct(const Struct&) = delete;
+
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'operator='
+  IDS_TEST_ABI void operator=(const Struct&) = delete;
+
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticMethod'
+  IDS_TEST_ABI static void staticMethod() {}
+
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'method'
+  IDS_TEST_ABI void method() {}
+
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'pureVirtualMethod'
+  IDS_TEST_ABI virtual void pureVirtualMethod() = 0;
+
+  // CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
+  IDS_TEST_ABI inline void inlineMethod() const;
+
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticConstField'
+  IDS_TEST_ABI static const int staticConstField = 0;
+
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticConstexprField'
+  IDS_TEST_ABI static constexpr int staticConstexprField = 0;
+};
+
+// CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
+void Struct::inlineMethod() const {}
+
+// CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
+struct IDS_TEST_ABI ExportedStruct {
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticMethod'
+  IDS_TEST_ABI static void staticMethod();
+
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'method'
+  IDS_TEST_ABI void method();
+
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticConstField'
+  IDS_TEST_ABI static const int staticConstField;
+
+  // CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticField'
+  IDS_TEST_ABI static int staticField;
+
+  // CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
+  IDS_TEST_ABI friend void friendMethod();
+};
+
+// CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
+IDS_TEST_ABI void friendMethod();

Tests/lit.cfg

@@ -47,3 +47,8 @@ config.test_exec_root = os.path.join(ids_obj_root, 'Tests')
 
 config.substitutions.append(('%FileCheck', config.filecheck_path))
 config.substitutions.append(('%idt', lit_config.params['idt']))
+
+config.supports_substitutions = True
+
+if platform.system() == 'Windows':
+    config.available_features.add('system-windows')