Skip to content

export classes with pure virtual destructors #53

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

Closed
wants to merge 1 commit into from
Closed

export classes with pure virtual destructors #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 12 additions & 1 deletion Sources/idt/idt.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,12 +424,23 @@ class visitor : public clang::RecursiveASTVisitor<visitor> {
// 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())
for (const auto *MD : RD->methods()) {
if ((should_export_record =
!(MD->isPureVirtual() || MD->isDefaulted() || MD->isDeleted()) &&
(MD->isVirtual() && !MD->hasBody())))
break;

// Unlike other pure virtual functions, pure virtual destructors require
// an out-of-line implementation. If a pure virtual destructor is found
// that has no body and is not defaulted, it must have an out-of-line
// implementation outside of the translation unit. In this case, treat it
// like any other out-of-line virtual method decl and export the record.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow this comment. Why is this the case? Are you referring to the ELF specific behaviour of the virtual dtor serving as a key function? If so, that is an itanium ABI specific behaviour. We can query the C++ model that is in use for evaluating this then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, nothing ABI specific. I will rework this comment so it is more clear.

My intent was to explain that we have two cases for the implementation of a pure virtual destructor. Since pure virtual destructors are the only pure virtual methods that require an implementation (afaict), we know there must be one somewhere. If it is in the translation unit, we don't have to export it:

struct Example {
  virtual ~Example() = 0;
};

Example::Example() {}

If it is not in the translation unit, we assume the destructor implementation is in a .cpp file somewhere, so it does need to be exported.

struct Example {
  virtual ~Example() = 0
};

Alternatively: we could just ALWAYS export classes that have pure virtual functions. This may lead to exporting a bit more than necessary, but will also catch the case where a class has other pure virtual functions with implementations. Does that sound better?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the case now makes more sense! If we can get away with it, that would be nice ... but, doesn't the virtual dtor result in a vtable which also needs to be exported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't the virtual dtor result in a vtable which also needs to be exported?

That's correct. This change results in a class getting exported whenever it has a pure virtual dtor.

Having given this more thought, I think we should stick with the more constrained case and export only when there is a pure virtual dtor. If we export a class whenever it contains any pure virtual function, that's going to export a lot more unnecessarily.

if ((should_export_record = llvm::isa<clang::CXXDestructorDecl>(MD) &&
MD->isPureVirtual()) &&
!(MD->hasBody() || MD->isDefaulted()))
break;
}

if (!should_export_record)
return;

Expand Down
47 changes: 47 additions & 0 deletions Tests/PureVirtualDestructor.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// RUN: %idt --export-macro IDT_TEST_ABI %s 2>&1 | %FileCheck %s

// PureVirtualClass should get annotated because it contains a pure virtual
// destructor with an out-of-line definition.

// CHECK: PureVirtualDestructor.hh:[[@LINE+1]]:8: remark: unexported public interface 'PureVirtualClass'
struct PureVirtualClass {
// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE-1]]:{{.*}}

// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE+1]]:{{.*}}
virtual ~PureVirtualClass() = 0;
// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE-1]]:{{.*}}

// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE+1]]:{{.*}}
virtual void virtualMethod() = 0;
};

// AnotherPureVirtualClass should get NOT get annotated because its pure virtual
// destructor has an out-of-line definition within this translation unit.

// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE+1]]:{{.*}}
struct AnotherPureVirtualClass {
// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE+1]]:{{.*}}
virtual ~AnotherPureVirtualClass() = 0;

// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE+1]]:{{.*}}
virtual void virtualMethod() = 0;
};

// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE+1]]:{{.*}}
AnotherPureVirtualClass::~AnotherPureVirtualClass() {}

// YetAnotherPureVirtualClass should get NOT get annotated because its pure
// virtual destructor has an out-of-line defaulted definition within this
// translation unit.

// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE+1]]:{{.*}}
struct YetAnotherPureVirtualClass {
// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE+1]]:{{.*}}
virtual ~YetAnotherPureVirtualClass() = 0;

// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE+1]]:{{.*}}
virtual void virtualMethod() = 0;
};

// CHECK-NOT: PureVirtualDestructor.hh:[[@LINE+1]]:{{.*}}
YetAnotherPureVirtualClass::~YetAnotherPureVirtualClass() = default;