diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index c902635..a394fd0 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -9,12 +9,12 @@ #include "clang/Rewrite/Frontend/FixItRewriter.h" #include "clang/Tooling/CommonOptionsParser.h" #include "clang/Tooling/Tooling.h" +#include "llvm/ADT/SmallPtrSet.h" #include #include #include #include -#include #include #include #include @@ -126,6 +126,28 @@ struct PPCallbacks : clang::PPCallbacks { FileIncludes &file_includes_; }; +// Track a set of clang::Decl declarations by unique ID. +class DeclSet { + llvm::SmallPtrSet decls_; + + // Use pointer identity of the canonical declaration object as a unique ID. + template + std::uintptr_t decl_id(const Decl_ *D) const { + return reinterpret_cast(D->getCanonicalDecl()); + } + +public: + template + inline void insert(const Decl_ *D) { + decls_.insert(decl_id(D)); + } + + template + inline bool contains(const Decl_ *D) const { + return decls_.find(decl_id(D)) != decls_.end(); + } +}; + class visitor : public clang::RecursiveASTVisitor { clang::ASTContext &context_; clang::SourceManager &source_manager_; @@ -133,9 +155,9 @@ class visitor : public clang::RecursiveASTVisitor { std::optional id_exported_; PPCallbacks::FileIncludes &file_includes_; - // This member is set to true when the containing record being traversed (if - // any) is already annotated for export. - bool in_exported_record_ = false; + // Accumulates the set of declarations that have been marked for export by + // this visitor. + DeclSet exported_decls_; void add_missing_include(clang::SourceLocation location) { if (include_header.empty()) @@ -183,9 +205,19 @@ class visitor : public clang::RecursiveASTVisitor { } clang::DiagnosticBuilder - unexported_public_interface(clang::SourceLocation location) { + unexported_public_interface(const clang::Decl *D) { + return unexported_public_interface(D, get_location(D)); + } + + clang::DiagnosticBuilder + unexported_public_interface(const clang::Decl *D, clang::SourceLocation location) { add_missing_include(location); + // Track every unexported declaration encountered. This information is used + // by is_symbol_exported to ensure a symbol does not get reported multiple + // times. + exported_decls_.insert(D); + clang::DiagnosticsEngine &diagnostics_engine = context_.getDiagnostics(); if (!id_unexported_) @@ -226,8 +258,17 @@ class visitor : public clang::RecursiveASTVisitor { return false; } + template + inline bool is_in_system_header(const Decl_ *D) const { + return source_manager_.isInSystemHeader(get_location(D)); + } + template bool is_symbol_exported(const Decl_ *D) const { + // Check the set of symbols we've already marked for export. + if (exported_decls_.contains(D)) + return true; + // Check if the symbol is annotated with __declspec(dllimport) or // __declspec(dllexport). if (D->template hasAttr() || @@ -238,21 +279,145 @@ class visitor : public clang::RecursiveASTVisitor { // or the equivalent __attribute__((visibility("default"))) if (const auto *VA = D->template getAttr()) return VA->getVisibility() == clang::VisibilityAttr::VisibilityType::Default; + + if (llvm::isa(D)) + return false; + + // 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(DC)) + return is_symbol_exported(RD); + return false; } - // Determine if a tagged type needs exporting at the record level. - // Returns true if the record has been, or was already, annotated. Returns - // false otherwise. - bool export_record_if_needed(clang::CXXRecordDecl *RD) { + // 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; + + // Skip declarations not in header files. + if (!is_in_header(FD)) + return; + + // We are only interested in non-dependent types. + if (FD->isDependentContext()) + return; + + // If the function has a body, it can be materialized by the user. + if (FD->hasBody()) + return; + + // Skip methods in template declarations. + if (FD->getTemplateInstantiationPattern()) + return; + + // Ignore friend declarations. + if (FD->getFriendObjectKind() != clang::Decl::FOK_None) + return; + + // Ignore deleted and defaulted functions (e.g. operators). + if (FD->isDeleted() || FD->isDefaulted()) + return; + + // Skip template class template argument deductions. + if (llvm::isa(FD)) + return; + + // Pure virtual methods cannot be exported. + if (const auto *MD = llvm::dyn_cast(FD)) + if (MD->isPureVirtual()) + return; + + // Ignore known forward declarations (builtins) + if (contains(kIgnoredBuiltins, FD->getNameAsString())) + return; + + // TODO(compnerd) replace with std::set::contains in C++20 + if (contains(get_ignored_symbols(), FD->getNameAsString())) + return; + + clang::SourceLocation SLoc = + FD->getTemplatedKind() == clang::FunctionDecl::TK_NonTemplate + ? FD->getBeginLoc() + : FD->getInnerLocStart(); + unexported_public_interface(FD) + << FD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); + } + + // 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; + + // Skip all variable declarations not in header files. + if (!is_in_header(VD)) + return; + + // Skip local variables. We are only interested in static fields. + if (VD->getParentFunctionOrMethod()) + return; + + // Skip static fields that have initializers. + if (VD->hasInit()) + return; + + // Skip all other local and global variables unless they are extern. + if (!(VD->isStaticDataMember() || + VD->getStorageClass() == clang::StorageClass::SC_Extern)) + return; + + // Skip fields in template declarations. + if (VD->getTemplateInstantiationPattern() != nullptr) + return; + + // Skip static variables declared in template class unless the template is + // fully specialized. + if (auto *RD = llvm::dyn_cast(VD->getDeclContext())) { + if (RD->getDescribedClassTemplate()) + return; + + if (auto *CTSD = llvm::dyn_cast(RD)) + if (llvm::isa(CTSD)) + return; + } + + // TODO(compnerd) replace with std::set::contains in C++20 + if (contains(get_ignored_symbols(), VD->getNameAsString())) + return; + + clang::SourceLocation SLoc = VD->getBeginLoc(); + unexported_public_interface(VD) + << VD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); + } + + // Determine if a tagged type needs exporting at the record level and add the + // export annotation as required. + void export_record_if_needed(clang::CXXRecordDecl *RD) { // Check if the class is already exported. if (is_symbol_exported(RD)) - return true; + return; + + // Ignore declarations from the system. + if (is_in_system_header(RD)) + return; // Skip exporting template classes. For fully-specialized template classes, // isTemplated() returns false so they will be annotated if needed. if (RD->isTemplated()) - return false; + return; // If a class declaration contains an out-of-line virtual method, annotate // the class instead of its individual members. This ensures its vtable is @@ -266,7 +431,7 @@ class visitor : public clang::RecursiveASTVisitor { break; if (!should_export_record) - return false; + return; // Insert the annotation immediately before the tag name, which is the // position returned by getLocation. @@ -274,10 +439,8 @@ class visitor : public clang::RecursiveASTVisitor { clang::SourceLocation SLoc = RD->getLocation(); const clang::SourceLocation location = context_.getFullLoc(SLoc).getExpansionLoc(); - unexported_public_interface(location) + unexported_public_interface(RD, location) << RD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); - - return true; } public: @@ -286,17 +449,12 @@ class visitor : public clang::RecursiveASTVisitor { file_includes_(file_includes) {} bool TraverseCXXRecordDecl(clang::CXXRecordDecl *RD) { - // Save/restore the current value of in_exported_record_ to support nested - // record definitions. - const bool old_in_exported_record = in_exported_record_; - in_exported_record_ = export_record_if_needed(RD); + export_record_if_needed(RD); // Traverse the class by invoking the parent's version of this method. This // call is required even if the record is exported because it may contain // nested records. - const bool result = RecursiveASTVisitor::TraverseCXXRecordDecl(RD); - in_exported_record_ = old_in_exported_record; - return result; + return RecursiveASTVisitor::TraverseCXXRecordDecl(RD); } // RecursiveASTVisitor::TraverseCXXRecordDecl does not get called for fully @@ -323,131 +481,85 @@ class visitor : public clang::RecursiveASTVisitor { } } + // VisitFunctionDecl will visit all function declarations. This includes top- + // level functions as well as class member and static functions. bool VisitFunctionDecl(clang::FunctionDecl *FD) { - clang::FullSourceLoc location = get_location(FD); - - // Ignore declarations from the system. - if (source_manager_.isInSystemHeader(location)) - return true; - - // Skip declarations not in header files. - if (!is_in_header(FD)) - return true; - - // We are only interested in non-dependent types. - if (FD->isDependentContext()) - return true; - - // If the function has a body, it can be materialized by the user. - if (FD->hasBody()) - return true; + // Ignore private member function declarations. Any that require export will + // be identified by VisitCallExpr. + if (const auto *MD = llvm::dyn_cast(FD)) + if (MD->getAccess() == clang::AccessSpecifier::AS_private) + return true; - // Ignore friend declarations. - if (FD->getFriendObjectKind() != clang::Decl::FOK_None) - return true; + export_function_if_needed(FD); + return true; + } - // Ignore deleted and defaulted functions (e.g. operators). - if (FD->isDeleted() || FD->isDefaulted()) + // Visit every function call in the compilation unit to determine if there are + // any inline calls to private member functions. In this uncommon case, the + // private method must be annotated for export. + bool VisitCallExpr(clang::CallExpr *CE) { + const clang::FunctionDecl *FD = CE->getDirectCallee(); + if (!FD) return true; - // Skip template class template argument deductions. - if (llvm::isa(FD)) + const clang::CXXMethodDecl *MD = llvm::dyn_cast(FD); + if (!MD) return true; - if (const auto *MD = llvm::dyn_cast(FD)) { - // Ignore private members (except for a negative check). - if (MD->getAccess() == clang::AccessSpecifier::AS_private) { - if (is_symbol_exported(MD)) - // TODO(compnerd) this should emit a fix-it to remove the attribute - exported_private_interface(location) << MD; - return true; - } + // Only consider private methods here. Non-private methods will be + // considered for export by VisitFunctionDecl. + if (MD->getAccess() == clang::AccessSpecifier::AS_private) + export_function_if_needed(MD); - // Pure virtual methods cannot be exported. - if (MD->isPureVirtual()) - return true; - } - - // If the function has a dll-interface, it is properly annotated. - if (is_symbol_exported(FD)) - return true; - - // If the containing record is exported, do not annotate individual members. - // TODO: if the symbol is already exported, emit a fix-it to remove it. - if (in_exported_record_) - return true; + return true; + } - // Ignore known forward declarations (builtins) - if (contains(kIgnoredBuiltins, FD->getNameAsString())) + // Visit every constructor call in the compilation unit to determine if there + // are any inline calls to private constructors. In this uncommon case, the + // private constructor must be annotated for export. Constructor calls are not + // visited by VisitCallExpr. + bool VisitCXXConstructExpr(clang::CXXConstructExpr *CE) { + const clang::CXXConstructorDecl *CD = CE->getConstructor(); + if (!CD) return true; - // TODO(compnerd) replace with std::set::contains in C++20 - if (contains(get_ignored_symbols(), FD->getNameAsString())) - return true; + // Only consider private constructors here. Non-private constructors will be + // considered for export by VisitFunctionDecl. + if (CD->getAccess() == clang::AccessSpecifier::AS_private) + export_function_if_needed(CD); - clang::SourceLocation insertion_point = - FD->getTemplatedKind() == clang::FunctionDecl::TK_NonTemplate - ? FD->getBeginLoc() - : FD->getInnerLocStart(); - unexported_public_interface(location) - << FD - << clang::FixItHint::CreateInsertion(insertion_point, - export_macro + " "); return true; } // VisitVarDecl will visit all variable declarations as well as static fields // in classes and structs. Non-static fields are not visited by this method. bool VisitVarDecl(clang::VarDecl *VD) { - if (is_symbol_exported(VD)) - return true; - - // If the containing record is exported, do not annotate individual members. - // TODO: if the symbol is already exported, emit a fix-it to remove it. - if (in_exported_record_) - return true; - - if (VD->hasInit()) - return true; - - // Skip local variables. - if (VD->getParentFunctionOrMethod()) - return true; - - // Skip all variable declarations not in header files. - if (!is_in_header(VD)) - return true; - - // Skip private static members. + // Ignore private static field declarations. Any that require export will be + // identified by VisitDeclRefExpr. if (VD->getAccess() == clang::AccessSpecifier::AS_private) return true; - // Skip all other local and global variables unless they are extern. - if (!VD->isStaticDataMember() && - VD->getStorageClass() != clang::StorageClass::SC_Extern) - return true; - - // Skip static variables declared in template class unless the template is - // fully specialized. - if (auto *RD = llvm::dyn_cast(VD->getDeclContext())) { - if (RD->getDescribedClassTemplate()) - return true; + export_variable_if_needed(VD); + return true; + } - if (auto *CTSD = llvm::dyn_cast(RD)) - if (llvm::isa(CTSD)) - return true; - } + // Visit every variable reference in the compilation unit to determine if + // there are any inline references to private, static member fields. In this + // uncommon case, the private field must be annotated for export. + bool VisitDeclRefExpr(clang::DeclRefExpr *DRE) { + // Only consider expresions referencing variable declarations. This includes + // static fields and local variables but not member variables, which are + // type FieldDecl. + auto *VD = llvm::dyn_cast(DRE->getDecl()); + if (!VD) + return true; - // TODO(compnerd) replace with std::set::contains in C++20 - if (contains(get_ignored_symbols(), VD->getNameAsString())) + // Only consider private fields here. Non-private fields will be considered + // for export by VisitVarDecl. + if (VD->getAccess() != clang::AccessSpecifier::AS_private) return true; - clang::FullSourceLoc location = get_location(VD); - clang::SourceLocation insertion_point = VD->getBeginLoc(); - unexported_public_interface(location) - << VD - << clang::FixItHint::CreateInsertion(insertion_point, - export_macro + " "); + export_variable_if_needed(VD); return true; } }; diff --git a/Tests/PrivateMembers.hh b/Tests/PrivateMembers.hh new file mode 100644 index 0000000..b5679fb --- /dev/null +++ b/Tests/PrivateMembers.hh @@ -0,0 +1,94 @@ +// RUN: %idt --export-macro IDT_TEST_ABI %s 2>&1 | %FileCheck %s + +// CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} +class WithPrivateMembers { +public: + // CHECK: PrivateMembers.hh:[[@LINE+1]]:3: remark: unexported public interface 'publicMethod' + int publicMethod(); + // CHECK-NOT: PrivateMembers.hh:[[@LINE-1]]:3: remark: unexported public interface 'privateStaticField' + + static WithPrivateMembers *create(int x) { + return new WithPrivateMembers(); + } + + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} + inline int publicInlineMethod() { + if (privateMethod() > 0) + return privateMethod() + publicMethod(); + + WithPrivateMembers other; + auto x = *this + other; + + privateStaticInlineMethod(); + + return privateMethod() - publicMethod(); + } + + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} + static inline void publicStaticInlineMethod() { + privateStaticField += 1; + + if (privateStaticField > 0) + privateStaticMethod(); + + privateStaticInlineMethod(); + } + +private: + // CHECK: PrivateMembers.hh:[[@LINE+1]]:3: remark: unexported public interface 'WithPrivateMembers' + explicit WithPrivateMembers(); + // CHECK-NOT: PrivateMembers.hh:[[@LINE-1]]:{{.*}} + + // CHECK: PrivateMembers.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateMethod' + int privateMethod(); + // CHECK-NOT: PrivateMembers.hh:[[@LINE-1]]:{{.*}} + + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} + int privateInlineMethod() { + return 0; + } + + // CHECK: PrivateMembers.hh:[[@LINE+1]]:3: remark: unexported public interface 'operator+' + WithPrivateMembers operator+(const WithPrivateMembers &other) const; + // CHECK-NOT: PrivateMembers.hh:[[@LINE-1]]:{{.*}} + + // CHECK: PrivateMembers.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticField' + static int privateStaticField; + // CHECK-NOT: PrivateMembers.hh:[[@LINE-1]]:{{.*}} + + // CHECK: PrivateMembers.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticMethod' + static void privateStaticMethod(); + // CHECK-NOT: PrivateMembers.hh:[[@LINE-1]]:{{.*}} + + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} + static void privateStaticInlineMethod() {} + + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} + friend void friendFunction(const WithPrivateMembers &obj); + + // CHECK: PrivateMembers.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateMethodForFriend' + void privateMethodForFriend() const; + // CHECK-NOT: PrivateMembers.hh:[[@LINE-1]]:{{.*}} +}; + +// CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} +inline void friendFunction(const WithPrivateMembers &obj) { + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} + return obj.privateMethodForFriend(); +} + +// CHECK: PrivateMembers.hh:[[@LINE+1]]:7: remark: unexported public interface 'VTableWithPrivateMembers' +class VTableWithPrivateMembers { +public: + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} + virtual int publicVirtualMethod(); + + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} + inline int protectedInlineMethod() { + return privateMethod() + publicVirtualMethod(); + } + +private: + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} + int privateMethod(); +};