Skip to content

emit fixes for improperly annotated symbols #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
compnerd merged 2 commits into from
Jul 8, 2025
Merged

emit fixes for improperly annotated symbols #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60672a4
emit fixes for improperly annotated symbols
andrurogerz Jun 25, 2025
4aad642
PR feedback
andrurogerz Jul 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 107 additions & 34 deletions Sources/idt/idt.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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))
Expand All @@ -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 *A = D->template getAttr<clang::DLLExportAttr>())
if (!A->isInherited())
SLoc = A->getLocation();

if (const auto *A = D->template getAttr<clang::DLLImportAttr>())
if (!A->isInherited())
SLoc = A->getLocation();

if (const auto *A = D->template getAttr<clang::VisibilityAttr>())
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;
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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.
Expand All @@ -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();
Expand Down
92 changes: 92 additions & 0 deletions Tests/Negative.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
5 changes: 5 additions & 0 deletions Tests/lit.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')