From 71d4658dadd0d4b17be50a56fd0b8cd8a66fd9e3 Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Tue, 29 Apr 2025 11:04:21 -0700 Subject: [PATCH 01/13] annotate private methods if needed --- Sources/idt/idt.cc | 137 ++++++++++++++++++++++------------------ Tests/PrivateMethods.hh | 42 ++++++++++++ 2 files changed, 119 insertions(+), 60 deletions(-) create mode 100644 Tests/PrivateMethods.hh diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index c902635..0f38b0d 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -280,6 +280,65 @@ class visitor : public clang::RecursiveASTVisitor { return true; } + bool export_function_if_needed(const clang::FunctionDecl *FD) { + // Check if the symbol is already exported. + if (is_symbol_exported(FD)) + return true; + + clang::FullSourceLoc location = get_location(FD); + + // Ignore declarations from the system. + if (source_manager_.isInSystemHeader(location)) + return false; + + // Skip declarations not in header files. + if (!is_in_header(FD)) + return false; + + // We are only interested in non-dependent types. + if (FD->isDependentContext()) + return false; + + // If the function has a body, it can be materialized by the user. + if (FD->hasBody()) + return false; + + // Ignore friend declarations. + if (FD->getFriendObjectKind() != clang::Decl::FOK_None) + return false; + + // Ignore deleted and defaulted functions (e.g. operators). + if (FD->isDeleted() || FD->isDefaulted()) + return false; + + // Skip template class template argument deductions. + if (llvm::isa(FD)) + return false; + + // Pure virtual methods cannot be exported. + if (const auto *MD = llvm::dyn_cast(FD)) + if (MD->isPureVirtual()) + return false; + + // Ignore known forward declarations (builtins) + if (contains(kIgnoredBuiltins, FD->getNameAsString())) + return false; + + // TODO(compnerd) replace with std::set::contains in C++20 + if (contains(get_ignored_symbols(), FD->getNameAsString())) + return false; + + 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; + } + public: visitor(clang::ASTContext &context, PPCallbacks::FileIncludes &file_includes) : context_(context), source_manager_(context.getSourceManager()), @@ -324,75 +383,33 @@ class visitor : public clang::RecursiveASTVisitor { } 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 friend declarations. - if (FD->getFriendObjectKind() != clang::Decl::FOK_None) - return true; - - // Ignore deleted and defaulted functions (e.g. operators). - if (FD->isDeleted() || FD->isDefaulted()) - return true; - - // Skip template class template argument deductions. - if (llvm::isa(FD)) - 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; - } - - // 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; - // Ignore known forward declarations (builtins) - if (contains(kIgnoredBuiltins, FD->getNameAsString())) + // Ignore private members. + if (const auto *MD = llvm::dyn_cast(FD)) + if (MD->getAccess() == clang::AccessSpecifier::AS_private) + return true; + + (void)export_function_if_needed(FD); + return true; + } + + // 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; - // TODO(compnerd) replace with std::set::contains in C++20 - if (contains(get_ignored_symbols(), FD->getNameAsString())) + const clang::CXXMethodDecl *MD = llvm::dyn_cast(FD); + if (!MD) return true; - 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 + " "); + (void)export_function_if_needed(MD); return true; } diff --git a/Tests/PrivateMethods.hh b/Tests/PrivateMethods.hh new file mode 100644 index 0000000..fb0e161 --- /dev/null +++ b/Tests/PrivateMethods.hh @@ -0,0 +1,42 @@ +// RUN: %idt --export-macro IDT_TEST_ABI %s 2>&1 | %FileCheck %s + +// CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} +class WithPrivateMethods { +public: + // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'publicMethod' + int publicMethod(); + + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} + inline int publicInlineMethod() { + return privateMethod() + publicMethod(); + } + + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} + static inline void publicStaticInlineMethod() { + privateStaticMethod(); + } + +private: + // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateMethod' + int privateMethod(); + + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} + int privateInlineMethod() { + return 0; + } + + // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticMethod' + static void privateStaticMethod(); + + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} + friend void friendFunction(const WithPrivateMethods &obj); + + // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateMethodForFriend' + void privateMethodForFriend() const; +}; + +// CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} +inline void friendFunction(const WithPrivateMethods &obj) { + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} + return obj.privateMethodForFriend(); +} From 8b9a880c6647e897d1e08dfdd7f290650381ed33 Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Tue, 29 Apr 2025 12:22:53 -0700 Subject: [PATCH 02/13] track exported records in a set --- Sources/idt/idt.cc | 145 ++++++++++++++++++++-------------------- Tests/PrivateMethods.hh | 16 +++++ 2 files changed, 87 insertions(+), 74 deletions(-) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index 0f38b0d..d0b3de0 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -133,9 +133,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 unique locations of records that we have exported. Location + // is used only as a unique identifier for a record declaration. + std::set exported_record_locations_; void add_missing_include(clang::SourceLocation location) { if (include_header.empty()) @@ -238,95 +238,66 @@ 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; - 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) { - // Check if the class is already exported. - if (is_symbol_exported(RD)) - return true; - // Skip exporting template classes. For fully-specialized template classes, - // isTemplated() returns false so they will be annotated if needed. - if (RD->isTemplated()) + if (!llvm::isa(D) && !llvm::isa(D)) return false; - // If a class declaration contains an out-of-line virtual method, annotate - // the class instead of its individual members. This ensures its vtable is - // exported on non-Windows platforms. Do this regardless of the method's - // access level. - bool should_export_record = false; - for (const auto *MD : RD->methods()) - if ((should_export_record = - !(MD->isPureVirtual() || MD->isDefaulted() || MD->isDeleted()) && - (MD->isVirtual() && !MD->hasBody()))) - break; - - if (!should_export_record) - return false; + // For variable and function declarations, check to see if the containing + // record, if any, is exported. + for (const clang::DeclContext *DC = D->getDeclContext(); DC; DC = DC->getParent()) + if (const auto *RD = llvm::dyn_cast(DC)) + return is_symbol_exported(RD) || + (exported_record_locations_.find(RD->getLocation()) != exported_record_locations_.end()); - // Insert the annotation immediately before the tag name, which is the - // position returned by getLocation. - clang::LangOptions LO = RD->getASTContext().getLangOpts(); - clang::SourceLocation SLoc = RD->getLocation(); - const clang::SourceLocation location = - context_.getFullLoc(SLoc).getExpansionLoc(); - unexported_public_interface(location) - << RD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); - - return true; + return false; } - bool export_function_if_needed(const clang::FunctionDecl *FD) { + void export_function_if_needed(const clang::FunctionDecl *FD) { // Check if the symbol is already exported. if (is_symbol_exported(FD)) - return true; - - clang::FullSourceLoc location = get_location(FD); + return; // Ignore declarations from the system. + clang::FullSourceLoc location = get_location(FD); if (source_manager_.isInSystemHeader(location)) - return false; + return; // Skip declarations not in header files. if (!is_in_header(FD)) - return false; + return; // We are only interested in non-dependent types. if (FD->isDependentContext()) - return false; + return; // If the function has a body, it can be materialized by the user. if (FD->hasBody()) - return false; + return; // Ignore friend declarations. if (FD->getFriendObjectKind() != clang::Decl::FOK_None) - return false; + return; // Ignore deleted and defaulted functions (e.g. operators). if (FD->isDeleted() || FD->isDefaulted()) - return false; + return; // Skip template class template argument deductions. if (llvm::isa(FD)) - return false; + return; // Pure virtual methods cannot be exported. if (const auto *MD = llvm::dyn_cast(FD)) if (MD->isPureVirtual()) - return false; + return; // Ignore known forward declarations (builtins) if (contains(kIgnoredBuiltins, FD->getNameAsString())) - return false; + return; // TODO(compnerd) replace with std::set::contains in C++20 if (contains(get_ignored_symbols(), FD->getNameAsString())) - return false; + return; clang::SourceLocation insertion_point = FD->getTemplatedKind() == clang::FunctionDecl::TK_NonTemplate @@ -336,7 +307,47 @@ class visitor : public clang::RecursiveASTVisitor { << FD << clang::FixItHint::CreateInsertion(insertion_point, export_macro + " "); - return true; + } + + // Determine if a tagged type needs exporting at the record level. + void export_record_if_needed(clang::CXXRecordDecl *RD) { + // Check if the class is already exported. + if (is_symbol_exported(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; + + // If a class declaration contains an out-of-line virtual method, annotate + // the class instead of its individual members. This ensures its vtable is + // exported on non-Windows platforms. Do this regardless of the method's + // access level. + bool should_export_record = false; + for (const auto *MD : RD->methods()) + if ((should_export_record = + !(MD->isPureVirtual() || MD->isDefaulted() || MD->isDeleted()) && + (MD->isVirtual() && !MD->hasBody()))) + break; + + if (!should_export_record) + return; + + // Insert the annotation immediately before the tag name, which is the + // position returned by getLocation. + clang::LangOptions LO = RD->getASTContext().getLangOpts(); + clang::SourceLocation SLoc = RD->getLocation(); + const clang::SourceLocation location = + context_.getFullLoc(SLoc).getExpansionLoc(); + unexported_public_interface(location) + << RD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); + + // Track the set of records that we have explicitly exported. This info is + // used later when examining members to determine whether or not they should + // be individually exported. + exported_record_locations_.insert(SLoc); + return; } public: @@ -345,17 +356,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 @@ -383,17 +389,13 @@ class visitor : public clang::RecursiveASTVisitor { } bool VisitFunctionDecl(clang::FunctionDecl *FD) { - // 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; - - // Ignore private members. + // Ignore private member functions. Any that require export will be + // determined by VisitCalLExpr instead. if (const auto *MD = llvm::dyn_cast(FD)) if (MD->getAccess() == clang::AccessSpecifier::AS_private) return true; - (void)export_function_if_needed(FD); + export_function_if_needed(FD); return true; } @@ -409,7 +411,7 @@ class visitor : public clang::RecursiveASTVisitor { if (!MD) return true; - (void)export_function_if_needed(MD); + export_function_if_needed(MD); return true; } @@ -419,11 +421,6 @@ class visitor : public clang::RecursiveASTVisitor { 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; diff --git a/Tests/PrivateMethods.hh b/Tests/PrivateMethods.hh index fb0e161..e5caeba 100644 --- a/Tests/PrivateMethods.hh +++ b/Tests/PrivateMethods.hh @@ -40,3 +40,19 @@ inline void friendFunction(const WithPrivateMethods &obj) { // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} return obj.privateMethodForFriend(); } + +// CHECK: PrivateMethods.hh:[[@LINE+1]]:7: remark: unexported public interface 'VTableWithPrivateMethods' +class VTableWithPrivateMethods { +public: + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} + virtual int publicVirtualMethod(); + + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} + inline int protectedInlineMethod() { + return privateMethod() + publicVirtualMethod(); + } + +private: + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} + int privateMethod(); +}; From dff10060c17db8dfd37b02d1137c2d9aae9a220e Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Tue, 29 Apr 2025 13:30:50 -0700 Subject: [PATCH 03/13] also export privat fields as needed --- Sources/idt/idt.cc | 120 ++++++++++++++++++++++++++-------------- Tests/PrivateMethods.hh | 6 +- 2 files changed, 84 insertions(+), 42 deletions(-) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index d0b3de0..d4a5f58 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -252,6 +252,8 @@ class visitor : public clang::RecursiveASTVisitor { return false; } + // 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)) @@ -309,7 +311,55 @@ class visitor : public clang::RecursiveASTVisitor { export_macro + " "); } - // Determine if a tagged type needs exporting at the record level. + // 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; + + // 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 variable declarations not in header files. + if (!is_in_header(VD)) + return; + + // Skip all other local and global variables unless they are extern. + if (!VD->isStaticDataMember() && + VD->getStorageClass() != clang::StorageClass::SC_Extern) + 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::FullSourceLoc location = get_location(VD); + clang::SourceLocation insertion_point = VD->getBeginLoc(); + unexported_public_interface(location) + << VD + << clang::FixItHint::CreateInsertion(insertion_point, + 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)) @@ -388,9 +438,11 @@ 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) { - // Ignore private member functions. Any that require export will be - // determined by VisitCalLExpr instead. + // 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; @@ -411,57 +463,43 @@ class visitor : public clang::RecursiveASTVisitor { if (!MD) return true; - export_function_if_needed(MD); + // 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); + 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 (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/PrivateMethods.hh b/Tests/PrivateMethods.hh index e5caeba..d831394 100644 --- a/Tests/PrivateMethods.hh +++ b/Tests/PrivateMethods.hh @@ -13,7 +13,8 @@ public: // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} static inline void publicStaticInlineMethod() { - privateStaticMethod(); + if (privateStaticField > 0) + privateStaticMethod(); } private: @@ -25,6 +26,9 @@ private: return 0; } + // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticField' + static int privateStaticField; + // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticMethod' static void privateStaticMethod(); From 001ac009b206632cfe65065b14c07881df67f78a Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Tue, 29 Apr 2025 14:44:07 -0700 Subject: [PATCH 04/13] avoid redundant exports by tracking all symbols --- Sources/idt/idt.cc | 69 +++++++++++++++++++++++++++++------------ Tests/PrivateMethods.hh | 16 +++++++++- 2 files changed, 64 insertions(+), 21 deletions(-) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index d4a5f58..edf74d3 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -126,6 +126,29 @@ struct PPCallbacks : clang::PPCallbacks { FileIncludes &file_includes_; }; +// Track a set of clang::Decl declarations by unique ID. +class DeclSet { + std::set decls_; + + // Use the raw address of the canonical declaration object as a unique + // identifier. + 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 +156,9 @@ class visitor : public clang::RecursiveASTVisitor { std::optional id_exported_; PPCallbacks::FileIncludes &file_includes_; - // Accumulates the unique locations of records that we have exported. Location - // is used only as a unique identifier for a record declaration. - std::set exported_record_locations_; + // 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,8 +206,9 @@ class visitor : public clang::RecursiveASTVisitor { } clang::DiagnosticBuilder - unexported_public_interface(clang::SourceLocation location) { + unexported_public_interface(clang::SourceLocation location, const clang::Decl *D) { add_missing_include(location); + exported_decls_.insert(D); clang::DiagnosticsEngine &diagnostics_engine = context_.getDiagnostics(); @@ -228,6 +252,10 @@ class visitor : public clang::RecursiveASTVisitor { 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() || @@ -239,15 +267,14 @@ class visitor : public clang::RecursiveASTVisitor { if (const auto *VA = D->template getAttr()) return VA->getVisibility() == clang::VisibilityAttr::VisibilityType::Default; - if (!llvm::isa(D) && !llvm::isa(D)) + if (llvm::isa(D)) return false; - // For variable and function declarations, check to see if the containing - // record, if any, is exported. + // For non-record declarations, check to see if the containing record, if + // any, is exported. This exports the member as well. for (const clang::DeclContext *DC = D->getDeclContext(); DC; DC = DC->getParent()) if (const auto *RD = llvm::dyn_cast(DC)) - return is_symbol_exported(RD) || - (exported_record_locations_.find(RD->getLocation()) != exported_record_locations_.end()); + return is_symbol_exported(RD); return false; } @@ -276,6 +303,10 @@ class visitor : public clang::RecursiveASTVisitor { if (FD->hasBody()) return; + // Skip methods in template declarations. + if (FD->getTemplateInstantiationPattern() != nullptr) + return; + // Ignore friend declarations. if (FD->getFriendObjectKind() != clang::Decl::FOK_None) return; @@ -305,7 +336,7 @@ class visitor : public clang::RecursiveASTVisitor { FD->getTemplatedKind() == clang::FunctionDecl::TK_NonTemplate ? FD->getBeginLoc() : FD->getInnerLocStart(); - unexported_public_interface(location) + unexported_public_interface(location, FD) << FD << clang::FixItHint::CreateInsertion(insertion_point, export_macro + " "); @@ -335,6 +366,10 @@ class visitor : public clang::RecursiveASTVisitor { 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())) { @@ -351,10 +386,10 @@ class visitor : public clang::RecursiveASTVisitor { return; clang::FullSourceLoc location = get_location(VD); - clang::SourceLocation insertion_point = VD->getBeginLoc(); - unexported_public_interface(location) + clang::SourceLocation SLoc = VD->getBeginLoc(); + unexported_public_interface(location, VD) << VD - << clang::FixItHint::CreateInsertion(insertion_point, + << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); } @@ -390,14 +425,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(location, RD) << RD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); - - // Track the set of records that we have explicitly exported. This info is - // used later when examining members to determine whether or not they should - // be individually exported. - exported_record_locations_.insert(SLoc); - return; } public: diff --git a/Tests/PrivateMethods.hh b/Tests/PrivateMethods.hh index d831394..69c22fb 100644 --- a/Tests/PrivateMethods.hh +++ b/Tests/PrivateMethods.hh @@ -8,18 +8,28 @@ public: // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} inline int publicInlineMethod() { - return privateMethod() + publicMethod(); + if (privateMethod() > 0) + return privateMethod() + publicMethod(); + + privateStaticInlineMethod(); + + return privateMethod() - publicMethod(); } // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} static inline void publicStaticInlineMethod() { + privateStaticField += 1; + if (privateStaticField > 0) privateStaticMethod(); + + privateStaticInlineMethod(); } private: // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateMethod' int privateMethod(); + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateMethod' // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} int privateInlineMethod() { @@ -28,10 +38,14 @@ private: // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticField' static int privateStaticField; + // CHECK-NOT: PrivateMethods.hh:[[@LINE-1]]:3: remark: unexported public interface 'privateStaticField' // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticMethod' static void privateStaticMethod(); + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} + static void privateStaticInlineMethod() {} + // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} friend void friendFunction(const WithPrivateMethods &obj); From 474111e6f87e3900663f410a48e2087cf0ad1241 Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Tue, 29 Apr 2025 17:16:50 -0700 Subject: [PATCH 05/13] rename test cases --- Tests/PrivateMembers.hh | 76 +++++++++++++++++++++++++++++++++++++++++ Tests/PrivateMethods.hh | 76 ----------------------------------------- 2 files changed, 76 insertions(+), 76 deletions(-) create mode 100644 Tests/PrivateMembers.hh delete mode 100644 Tests/PrivateMethods.hh diff --git a/Tests/PrivateMembers.hh b/Tests/PrivateMembers.hh new file mode 100644 index 0000000..e882824 --- /dev/null +++ b/Tests/PrivateMembers.hh @@ -0,0 +1,76 @@ +// 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]]:{{.*}} + inline int publicInlineMethod() { + if (privateMethod() > 0) + return privateMethod() + publicMethod(); + + 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 'privateMethod' + int privateMethod(); + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateMethod' + + // CHECK-NOT: PrivateMembers.hh:[[@LINE+1]]:{{.*}} + int privateInlineMethod() { + return 0; + } + + // CHECK: PrivateMembers.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticField' + static int privateStaticField; + // CHECK-NOT: PrivateMembers.hh:[[@LINE-1]]:3: remark: unexported public interface 'privateStaticField' + + // CHECK: PrivateMembers.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticMethod' + static void privateStaticMethod(); + + // 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]]:{{.*}} +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(); +}; diff --git a/Tests/PrivateMethods.hh b/Tests/PrivateMethods.hh deleted file mode 100644 index 69c22fb..0000000 --- a/Tests/PrivateMethods.hh +++ /dev/null @@ -1,76 +0,0 @@ -// RUN: %idt --export-macro IDT_TEST_ABI %s 2>&1 | %FileCheck %s - -// CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} -class WithPrivateMethods { -public: - // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'publicMethod' - int publicMethod(); - - // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} - inline int publicInlineMethod() { - if (privateMethod() > 0) - return privateMethod() + publicMethod(); - - privateStaticInlineMethod(); - - return privateMethod() - publicMethod(); - } - - // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} - static inline void publicStaticInlineMethod() { - privateStaticField += 1; - - if (privateStaticField > 0) - privateStaticMethod(); - - privateStaticInlineMethod(); - } - -private: - // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateMethod' - int privateMethod(); - // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateMethod' - - // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} - int privateInlineMethod() { - return 0; - } - - // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticField' - static int privateStaticField; - // CHECK-NOT: PrivateMethods.hh:[[@LINE-1]]:3: remark: unexported public interface 'privateStaticField' - - // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateStaticMethod' - static void privateStaticMethod(); - - // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} - static void privateStaticInlineMethod() {} - - // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} - friend void friendFunction(const WithPrivateMethods &obj); - - // CHECK: PrivateMethods.hh:[[@LINE+1]]:3: remark: unexported public interface 'privateMethodForFriend' - void privateMethodForFriend() const; -}; - -// CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} -inline void friendFunction(const WithPrivateMethods &obj) { - // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} - return obj.privateMethodForFriend(); -} - -// CHECK: PrivateMethods.hh:[[@LINE+1]]:7: remark: unexported public interface 'VTableWithPrivateMethods' -class VTableWithPrivateMethods { -public: - // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} - virtual int publicVirtualMethod(); - - // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} - inline int protectedInlineMethod() { - return privateMethod() + publicVirtualMethod(); - } - -private: - // CHECK-NOT: PrivateMethods.hh:[[@LINE+1]]:{{.*}} - int privateMethod(); -}; From 4224c994ad989922d40ced017624d55ef30708af Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Tue, 29 Apr 2025 17:27:06 -0700 Subject: [PATCH 06/13] skip anything in system headers --- Sources/idt/idt.cc | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index edf74d3..271c787 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -250,6 +250,11 @@ 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. @@ -287,8 +292,7 @@ class visitor : public clang::RecursiveASTVisitor { return; // Ignore declarations from the system. - clang::FullSourceLoc location = get_location(FD); - if (source_manager_.isInSystemHeader(location)) + if (is_in_system_header(FD)) return; // Skip declarations not in header files. @@ -332,14 +336,12 @@ class visitor : public clang::RecursiveASTVisitor { if (contains(get_ignored_symbols(), FD->getNameAsString())) return; - clang::SourceLocation insertion_point = + clang::SourceLocation SLoc = FD->getTemplatedKind() == clang::FunctionDecl::TK_NonTemplate ? FD->getBeginLoc() : FD->getInnerLocStart(); - unexported_public_interface(location, FD) - << FD - << clang::FixItHint::CreateInsertion(insertion_point, - export_macro + " "); + unexported_public_interface(get_location(FD), FD) + << FD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); } // Determine if a variable needs exporting and add the export annotation as @@ -349,6 +351,10 @@ class visitor : public clang::RecursiveASTVisitor { if (is_symbol_exported(VD)) return; + // Ignore declarations from the system. + if (is_in_system_header(VD)) + return; + // Skip local variables. We are only interested in static fields. if (VD->getParentFunctionOrMethod()) return; @@ -385,12 +391,9 @@ class visitor : public clang::RecursiveASTVisitor { if (contains(get_ignored_symbols(), VD->getNameAsString())) return; - clang::FullSourceLoc location = get_location(VD); clang::SourceLocation SLoc = VD->getBeginLoc(); - unexported_public_interface(location, VD) - << VD - << clang::FixItHint::CreateInsertion(SLoc, - export_macro + " "); + unexported_public_interface(get_location(VD), VD) + << VD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); } // Determine if a tagged type needs exporting at the record level and add the @@ -400,6 +403,10 @@ class visitor : public clang::RecursiveASTVisitor { if (is_symbol_exported(RD)) 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()) From d6f226dd2108fb3d043b589214f6e2b6b725e0c3 Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Wed, 30 Apr 2025 08:19:30 -0700 Subject: [PATCH 07/13] also annotate private constructors as needed --- Sources/idt/idt.cc | 17 +++++++++++++++++ Tests/PrivateMembers.hh | 22 ++++++++++++++++++++-- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index 271c787..1660684 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -507,6 +507,23 @@ class visitor : public clang::RecursiveASTVisitor { return true; } + // 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; + + // 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); + + 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) { diff --git a/Tests/PrivateMembers.hh b/Tests/PrivateMembers.hh index e882824..b5679fb 100644 --- a/Tests/PrivateMembers.hh +++ b/Tests/PrivateMembers.hh @@ -5,12 +5,20 @@ 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(); @@ -27,21 +35,30 @@ public: } 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]]:3: remark: unexported public interface '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]]:3: remark: unexported public interface '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() {} @@ -51,6 +68,7 @@ private: // 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]]:{{.*}} From b5bc530e1cbe8a7236fa2850f2e25ae0261604a9 Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Wed, 30 Apr 2025 08:28:59 -0700 Subject: [PATCH 08/13] Update Sources/idt/idt.cc Co-authored-by: Saleem Abdulrasool --- Sources/idt/idt.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index 1660684..6545691 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -308,7 +308,7 @@ class visitor : public clang::RecursiveASTVisitor { return; // Skip methods in template declarations. - if (FD->getTemplateInstantiationPattern() != nullptr) + if (!FD->getTemplateInstantiationPattern()) return; // Ignore friend declarations. From f131b9ff1be9923e5b4f0d61eb5167952b083acc Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Wed, 30 Apr 2025 08:34:14 -0700 Subject: [PATCH 09/13] invert incorrect getTemplateInstantiationPattern check --- Sources/idt/idt.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index 6545691..813fefe 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -308,7 +308,7 @@ class visitor : public clang::RecursiveASTVisitor { return; // Skip methods in template declarations. - if (!FD->getTemplateInstantiationPattern()) + if (FD->getTemplateInstantiationPattern()) return; // Ignore friend declarations. From ee04dea229e8add440c6d23b348b3bf1648fd5bb Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Wed, 30 Apr 2025 08:49:35 -0700 Subject: [PATCH 10/13] PR feedback --- Sources/idt/idt.cc | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index 813fefe..56b5ce7 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -130,8 +130,7 @@ struct PPCallbacks : clang::PPCallbacks { class DeclSet { std::set decls_; - // Use the raw address of the canonical declaration object as a unique - // identifier. + // 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()); @@ -206,7 +205,12 @@ class visitor : public clang::RecursiveASTVisitor { } clang::DiagnosticBuilder - unexported_public_interface(clang::SourceLocation location, const clang::Decl *D) { + 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); exported_decls_.insert(D); @@ -275,8 +279,7 @@ class visitor : public clang::RecursiveASTVisitor { if (llvm::isa(D)) return false; - // For non-record declarations, check to see if the containing record, if - // any, is exported. This exports the member as well. + // 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); @@ -340,7 +343,7 @@ class visitor : public clang::RecursiveASTVisitor { FD->getTemplatedKind() == clang::FunctionDecl::TK_NonTemplate ? FD->getBeginLoc() : FD->getInnerLocStart(); - unexported_public_interface(get_location(FD), FD) + unexported_public_interface(FD) << FD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); } @@ -355,6 +358,10 @@ class visitor : public clang::RecursiveASTVisitor { 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; @@ -363,10 +370,6 @@ class visitor : public clang::RecursiveASTVisitor { if (VD->hasInit()) return; - // Skip all variable declarations not in header files. - if (!is_in_header(VD)) - return; - // Skip all other local and global variables unless they are extern. if (!VD->isStaticDataMember() && VD->getStorageClass() != clang::StorageClass::SC_Extern) @@ -392,7 +395,7 @@ class visitor : public clang::RecursiveASTVisitor { return; clang::SourceLocation SLoc = VD->getBeginLoc(); - unexported_public_interface(get_location(VD), VD) + unexported_public_interface(VD) << VD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); } @@ -432,7 +435,7 @@ class visitor : public clang::RecursiveASTVisitor { clang::SourceLocation SLoc = RD->getLocation(); const clang::SourceLocation location = context_.getFullLoc(SLoc).getExpansionLoc(); - unexported_public_interface(location, RD) + unexported_public_interface(RD, location) << RD << clang::FixItHint::CreateInsertion(SLoc, export_macro + " "); } From d6e91ae5beca24404c0fefb12131581b33cd410f Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Wed, 30 Apr 2025 13:15:02 -0700 Subject: [PATCH 11/13] PR feedback: use llmv::SmallPtrSet instead of std::set for decl tracking --- Sources/idt/idt.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index 56b5ce7..5067d7c 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 @@ -128,7 +128,10 @@ struct PPCallbacks : clang::PPCallbacks { // Track a set of clang::Decl declarations by unique ID. class DeclSet { - std::set decls_; + // Use the maximum allowed fixed-size for SmallPtrSet. If the set grows + // larger than this size, SmallPtrSet switches to "large set" behavior. + static constexpr size_t SMALL_SET_SIZE = 32; + llvm::SmallPtrSet decls_; // Use pointer identity of the canonical declaration object as a unique ID. template From bfa41a4d2c62b34e7777d91aad7a59457613f2d8 Mon Sep 17 00:00:00 2001 From: Saleem Abdulrasool Date: Mon, 5 May 2025 09:21:20 -0700 Subject: [PATCH 12/13] Apply suggestions from code review --- Sources/idt/idt.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index 5067d7c..4528d83 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -128,10 +128,7 @@ struct PPCallbacks : clang::PPCallbacks { // Track a set of clang::Decl declarations by unique ID. class DeclSet { - // Use the maximum allowed fixed-size for SmallPtrSet. If the set grows - // larger than this size, SmallPtrSet switches to "large set" behavior. - static constexpr size_t SMALL_SET_SIZE = 32; - llvm::SmallPtrSet decls_; + llvm::SmallPtrSet decls_; // Use pointer identity of the canonical declaration object as a unique ID. template @@ -374,8 +371,8 @@ class visitor : public clang::RecursiveASTVisitor { return; // Skip all other local and global variables unless they are extern. - if (!VD->isStaticDataMember() && - VD->getStorageClass() != clang::StorageClass::SC_Extern) + if (!(VD->isStaticDataMember() || + VD->getStorageClass() == clang::StorageClass::SC_Extern)) return; // Skip fields in template declarations. From 4dfbeda1299cd92479ba853dec739192dfe68de6 Mon Sep 17 00:00:00 2001 From: Andrew Rogers Date: Mon, 5 May 2025 09:37:41 -0700 Subject: [PATCH 13/13] PR feedback: add an explanatory commment --- Sources/idt/idt.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index 4528d83..a394fd0 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -212,6 +212,10 @@ class visitor : public clang::RecursiveASTVisitor { 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();