diff --git a/Sources/idt/idt.cc b/Sources/idt/idt.cc index 0436705..d383a91 100644 --- a/Sources/idt/idt.cc +++ b/Sources/idt/idt.cc @@ -152,6 +152,7 @@ class visitor : public clang::RecursiveASTVisitor { clang::ASTContext &context_; clang::SourceManager &source_manager_; std::optional id_unexported_; + std::optional id_improper_; std::optional id_exported_; PPCallbacks::FileIncludes &file_includes_; @@ -228,6 +229,18 @@ class visitor : public clang::RecursiveASTVisitor { 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 { if (const auto *VA = D->template getAttr()) return VA->getVisibility() == clang::VisibilityAttr::VisibilityType::Default; - if (llvm::isa(D)) - return false; + return false; + } + template + 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(DC)) @@ -291,13 +306,39 @@ class visitor : public clang::RecursiveASTVisitor { 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 + void check_symbol_not_exported(const Decl_ *D, const std::string &message) { + clang::SourceLocation SLoc; + if (const auto *A = D->template getAttr()) + if (!A->isInherited()) + SLoc = A->getLocation(); + + if (const auto *A = D->template getAttr()) + if (!A->isInherited()) + SLoc = A->getLocation(); + + if (const auto *A = D->template getAttr()) + if (!A->isInherited() && + A->getVisibility() == clang::VisibilityAttr::VisibilityType::Default) + SLoc = A->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 { 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(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(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())) + // 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 { // 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 { 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 { 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(); diff --git a/Tests/Negative.hh b/Tests/Negative.hh new file mode 100644 index 0000000..f3bc364 --- /dev/null +++ b/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(); diff --git a/Tests/lit.cfg b/Tests/lit.cfg index 28f2a20..6f52d09 100644 --- a/Tests/lit.cfg +++ b/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')