Skip to content

Commit b3bf35d

Browse files
andrurogerzandrurogerz
authored
emit fixes for improperly annotated symbols (#57)
1 parent 7d09df7 commit b3bf35d

3 files changed

Lines changed: 204 additions & 34 deletions

File tree

‎Sources/idt/idt.cc‎

Lines changed: 107 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
152152
clang::ASTContext &context_;
153153
clang::SourceManager &source_manager_;
154154
std::optional<unsigned> id_unexported_;
155+
std::optional<unsigned> id_improper_;
155156
std::optional<unsigned> id_exported_;
156157
PPCallbacks::FileIncludes &file_includes_;
157158

@@ -228,6 +229,18 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
228229
return diagnostics_engine.Report(location, *id_unexported_);
229230
}
230231

232+
clang::DiagnosticBuilder
233+
improperly_exported_interface(clang::SourceLocation location) {
234+
clang::DiagnosticsEngine &diagnostics_engine = context_.getDiagnostics();
235+
236+
if (!id_improper_)
237+
id_improper_ = diagnostics_engine.getCustomDiagID(
238+
clang::DiagnosticsEngine::Remark,
239+
"improperly exported symbol %0: %1");
240+
241+
return diagnostics_engine.Report(location, *id_improper_);
242+
}
243+
231244
clang::DiagnosticBuilder
232245
exported_private_interface(clang::SourceLocation location) {
233246
clang::DiagnosticsEngine &diagnostics_engine = context_.getDiagnostics();
@@ -280,9 +293,11 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
280293
if (const auto *VA = D->template getAttr<clang::VisibilityAttr>())
281294
return VA->getVisibility() == clang::VisibilityAttr::VisibilityType::Default;
282295

283-
if (llvm::isa<clang::RecordDecl>(D))
284-
return false;
296+
return false;
297+
}
285298

299+
template <typename Decl_>
300+
bool is_containing_record_exported(const Decl_ *D) const {
286301
// For non-record declarations, the DeclContext is the containing record.
287302
for (const clang::DeclContext *DC = D->getDeclContext(); DC; DC = DC->getParent())
288303
if (const auto *RD = llvm::dyn_cast<clang::RecordDecl>(DC))
@@ -291,13 +306,39 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
291306
return false;
292307
}
293308

309+
// Emit a FixIt if a symbol is annotated with a default visibility or DLL
310+
// export/import annotation. The FixIt will remove the annotation
311+
template <typename Decl_>
312+
void check_symbol_not_exported(const Decl_ *D, const std::string &message) {
313+
clang::SourceLocation SLoc;
314+
if (const auto *A = D->template getAttr<clang::DLLExportAttr>())
315+
if (!A->isInherited())
316+
SLoc = A->getLocation();
317+
318+
if (const auto *A = D->template getAttr<clang::DLLImportAttr>())
319+
if (!A->isInherited())
320+
SLoc = A->getLocation();
321+
322+
if (const auto *A = D->template getAttr<clang::VisibilityAttr>())
323+
if (!A->isInherited() &&
324+
A->getVisibility() == clang::VisibilityAttr::VisibilityType::Default)
325+
SLoc = A->getLocation();
326+
327+
if (SLoc.isInvalid())
328+
return;
329+
330+
if (SLoc.isMacroID())
331+
SLoc = source_manager_.getExpansionLoc(SLoc);
332+
333+
clang::CharSourceRange range =
334+
clang::CharSourceRange::getTokenRange(SLoc, SLoc);
335+
improperly_exported_interface(SLoc)
336+
<< D << message << clang::FixItHint::CreateRemoval(range);
337+
}
338+
294339
// Determine if a function needs exporting and add the export annotation as
295340
// required.
296341
void export_function_if_needed(const clang::FunctionDecl *FD) {
297-
// Check if the symbol is already exported.
298-
if (is_symbol_exported(FD))
299-
return;
300-
301342
// Ignore declarations from the system.
302343
if (is_in_system_header(FD))
303344
return;
@@ -306,41 +347,63 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
306347
if (!is_in_header(FD))
307348
return;
308349

309-
// We are only interested in non-dependent types.
310-
if (FD->isDependentContext())
350+
// Ignore friend declarations.
351+
if (FD->getFriendObjectKind() != clang::Decl::FOK_None)
311352
return;
312353

313-
// If the function has a body, it can be materialized by the user.
314-
if (FD->hasBody())
354+
// Ignore known forward declarations (builtins)
355+
if (contains(kIgnoredBuiltins, FD->getNameAsString()))
315356
return;
316357

317-
// Skip methods in template declarations.
318-
if (FD->getTemplateInstantiationPattern())
358+
// TODO(compnerd) replace with std::set::contains in C++20
359+
if (contains(get_ignored_symbols(), FD->getNameAsString()))
319360
return;
320361

321-
// Ignore friend declarations.
322-
if (FD->getFriendObjectKind() != clang::Decl::FOK_None)
362+
// Skip functions contained in classes that are already exported.
363+
if (is_containing_record_exported(FD)) {
364+
// Exporting a symbol contained in an already exported class/struct will
365+
// fail compilation on Windows.
366+
check_symbol_not_exported(FD, "containing class is exported");
367+
return;
368+
}
369+
370+
// Skip any function defined inline, it can be materialized by the user.
371+
if (FD->isThisDeclarationADefinition()) {
372+
check_symbol_not_exported(FD, "function is defined inline");
323373
return;
374+
}
375+
376+
// Pure virtual methods cannot be exported.
377+
if (const auto *MD = llvm::dyn_cast<clang::CXXMethodDecl>(FD))
378+
if (MD->isPureVirtual()) {
379+
check_symbol_not_exported(FD, "pure virtual method");
380+
return;
381+
}
324382

325383
// Ignore deleted and defaulted functions (e.g. operators).
326384
if (FD->isDeleted() || FD->isDefaulted())
327385
return;
328386

387+
// We are only interested in non-dependent types.
388+
if (FD->isDependentContext())
389+
return;
390+
391+
// Skip methods in template declarations.
392+
if (FD->getTemplateInstantiationPattern())
393+
return;
394+
329395
// Skip template class template argument deductions.
330396
if (llvm::isa<clang::CXXDeductionGuideDecl>(FD))
331397
return;
332398

333-
// Pure virtual methods cannot be exported.
334-
if (const auto *MD = llvm::dyn_cast<clang::CXXMethodDecl>(FD))
335-
if (MD->isPureVirtual())
336-
return;
337-
338-
// Ignore known forward declarations (builtins)
339-
if (contains(kIgnoredBuiltins, FD->getNameAsString()))
399+
// If the function has a body, it can be materialized by the user. This
400+
// check is distinct from isThisDeclarationADefinition() because it will
401+
// return true if the function has a body anywhere in the translation unit.
402+
if (FD->hasBody())
340403
return;
341404

342-
// TODO(compnerd) replace with std::set::contains in C++20
343-
if (contains(get_ignored_symbols(), FD->getNameAsString()))
405+
// Check if the symbol is already exported.
406+
if (is_symbol_exported(FD))
344407
return;
345408

346409
// Use the inner start location so that the annotation comes after
@@ -360,10 +423,6 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
360423
// Determine if a variable needs exporting and add the export annotation as
361424
// required. This only applies to extern globals and static member fields.
362425
void export_variable_if_needed(const clang::VarDecl *VD) {
363-
// Check if the symbol is already exported.
364-
if (is_symbol_exported(VD))
365-
return;
366-
367426
// Ignore declarations from the system.
368427
if (is_in_system_header(VD))
369428
return;
@@ -376,18 +435,28 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
376435
if (VD->getParentFunctionOrMethod())
377436
return;
378437

379-
// Skip static fields that have initializers.
380-
if (VD->hasInit())
438+
// TODO(compnerd) replace with std::set::contains in C++20
439+
if (contains(get_ignored_symbols(), VD->getNameAsString()))
440+
return;
441+
442+
// Skip variables that have initializers.
443+
if (VD->hasInit()) {
444+
check_symbol_not_exported(VD, "variable initialized at declaration");
381445
return;
446+
}
382447

383448
// Skip all other local and global variables unless they are extern.
384449
if (!(VD->isStaticDataMember() ||
385-
VD->getStorageClass() == clang::StorageClass::SC_Extern))
450+
VD->getStorageClass() == clang::StorageClass::SC_Extern)) {
451+
check_symbol_not_exported(VD, "variable not static or extern");
386452
return;
453+
}
387454

388-
// Skip fields in template declarations.
389-
if (VD->getTemplateInstantiationPattern() != nullptr)
455+
// Skip variables contained in classes that are already exported.
456+
if (is_containing_record_exported(VD)) {
457+
check_symbol_not_exported(VD, "containing class is exported");
390458
return;
459+
}
391460

392461
// Skip static variables declared in template class unless the template is
393462
// fully specialized.
@@ -400,8 +469,12 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
400469
return;
401470
}
402471

403-
// TODO(compnerd) replace with std::set::contains in C++20
404-
if (contains(get_ignored_symbols(), VD->getNameAsString()))
472+
// Skip fields in template declarations.
473+
if (VD->getTemplateInstantiationPattern() != nullptr)
474+
return;
475+
476+
// Check if the symbol is already exported.
477+
if (is_symbol_exported(VD))
405478
return;
406479

407480
clang::SourceLocation SLoc = VD->getBeginLoc();

‎Tests/Negative.hh‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
// NOTE: We expect idt to return a non-zero exit code on Windows because the
2+
// compiler will actually produce errors when __declspec(dllexport) appears in
3+
// unsupported locations (not the case for visibility attributes). So we have
4+
// to run "not idt" when running on Windows so the test doesn't fail due to
5+
// the non-zero exit code from idt.
6+
7+
// RUN: %if system-windows %{ \
8+
// RUN: not %idt --export-macro IDT_TEST_ABI %s 2>&1 | %FileCheck %s \
9+
// RUN: %} %else %{ \
10+
// RUN: %idt --export-macro IDT_TEST_ABI %s 2>&1 | %FileCheck %s \
11+
// RUN: %}
12+
13+
#if defined(_WIN32)
14+
#define IDS_TEST_ABI __declspec(dllexport)
15+
#else
16+
#define IDS_TEST_ABI __attribute__((visibility("default")))
17+
#endif
18+
19+
// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'staticFunction'
20+
IDS_TEST_ABI static void staticFunction() {}
21+
22+
// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'inlineFunction'
23+
IDS_TEST_ABI inline void inlineFunction() {}
24+
25+
// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'function'
26+
IDS_TEST_ABI void function() {}
27+
28+
// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'constVariable'
29+
IDS_TEST_ABI const int constVariable = 0;
30+
31+
// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'staticConstVariable'
32+
IDS_TEST_ABI static const int staticConstVariable = 0;
33+
34+
// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'constexprVariable'
35+
IDS_TEST_ABI constexpr int constexprVariable = 0;
36+
37+
// CHECK: Negative.hh:[[@LINE+1]]:1: remark: improperly exported symbol 'staticConstexprVariable'
38+
IDS_TEST_ABI static constexpr int staticConstexprVariable = 0;
39+
40+
// CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
41+
struct Struct {
42+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'Struct'
43+
IDS_TEST_ABI Struct() = default;
44+
45+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'Struct'
46+
IDS_TEST_ABI Struct(const Struct&) = delete;
47+
48+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'operator='
49+
IDS_TEST_ABI void operator=(const Struct&) = delete;
50+
51+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticMethod'
52+
IDS_TEST_ABI static void staticMethod() {}
53+
54+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'method'
55+
IDS_TEST_ABI void method() {}
56+
57+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'pureVirtualMethod'
58+
IDS_TEST_ABI virtual void pureVirtualMethod() = 0;
59+
60+
// CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
61+
IDS_TEST_ABI inline void inlineMethod() const;
62+
63+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticConstField'
64+
IDS_TEST_ABI static const int staticConstField = 0;
65+
66+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticConstexprField'
67+
IDS_TEST_ABI static constexpr int staticConstexprField = 0;
68+
};
69+
70+
// CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
71+
void Struct::inlineMethod() const {}
72+
73+
// CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
74+
struct IDS_TEST_ABI ExportedStruct {
75+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticMethod'
76+
IDS_TEST_ABI static void staticMethod();
77+
78+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'method'
79+
IDS_TEST_ABI void method();
80+
81+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticConstField'
82+
IDS_TEST_ABI static const int staticConstField;
83+
84+
// CHECK: Negative.hh:[[@LINE+1]]:3: remark: improperly exported symbol 'staticField'
85+
IDS_TEST_ABI static int staticField;
86+
87+
// CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
88+
IDS_TEST_ABI friend void friendMethod();
89+
};
90+
91+
// CHECK-NOT: Negative.hh:[[@LINE+1]]:{{.*}}
92+
IDS_TEST_ABI void friendMethod();

‎Tests/lit.cfg‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,3 +47,8 @@ config.test_exec_root = os.path.join(ids_obj_root, 'Tests')
4747

4848
config.substitutions.append(('%FileCheck', config.filecheck_path))
4949
config.substitutions.append(('%idt', lit_config.params['idt']))
50+
51+
config.supports_substitutions = True
52+
53+
if platform.system() == 'Windows':
54+
config.available_features.add('system-windows')

0 commit comments

Comments
 (0)