[PATCH] Add full semantic support for dllimport/export attributes
Reid Kleckner
rnk at google.com
Sun Jul 7 14:26:44 PDT 2013
On Fri, Jul 5, 2013 at 9:13 PM, Nico Rieck <nico.rieck at gmail.com> wrote:
> This is the first patch in a series to get full support for the
> dllexport/dllimport attributes and handles the semantic side.
>
> In particular it:
> - Adds support for class-wide export/import attributes (PR11170)
> - Handles dllexport/dllimport on inline functions and templates
> - Extends tests for globals and functions.
>
> It deviates from MSVC's behavior in the following cases:
> - If a dllimported global variable is redeclared without dllimport, the
> attribute is ignored with a warning. GCC does the same, MSVC makes the
> global dllexport.
> - Out-of-line member function definitions of dllimported class templates
> are allowed if the definition is declared as inline. MSVC allows this for
> classes, but not for class templates.
>
> In some cases a dllimport/dllexport attribute is ignored. If this happens
> on
> a member of an exported/imported class, we need a way to drop the
> attribute.
> This is implemented by propagating these attributes to every member during
> semantic analysis. To differentiate them from explicit member attributes,
> they have their inherited status set.
> To make this work a small change (or possibly fix?) to table-gen was
> required so that the inherited status of an attribute is preserved when
> instantiating template attributes.
>
>
> The semantics this patch should cover are:
>
> 1. dllexport and dllimport can be applied to functions, static data
> members and global variables. Both can also be applied to typedefs
> and enums (without effect) if -fms-extensions is specified, for all
> other cases the attributes are ignored and a warning is issued. It is
> an error to apply them to non-static data members.
>
> 2. dllexport requires a definition. The extern keyword can be used to
> force a declaration.
>
This only applies to class or template definitions, right? You can
certainly put dllexport on function declarations in TUs that don't define
the function.
> 3. dllimport requires a declaration.
>
> 4. dllexport exposes the function or variable with its decorated name.
> a) For C++, this includes name mangling.
> b) For C or extern "C" functions, this includes platform-specific
> decoration for the calling convention.
>
> 5. dllexport takes precedence over dllimport. If both are specified,
> dllimport is ignored and a warning is issued.
>
> 6. Classes
> a) When applied to a class, dllexport or dllimport affect all static
> and non-static member functions and all static data members,
> including all non-deleted special members.
> b) Everything that is accessible to the DLL's client according to C++
> access rules is exported (including private data members referenced
> in inline functions).
> c) Members can be imported or exported selectively.
>
This selectivity presumably allows you to split a class definition across
multiple dlls. What linkage does the class data (vtables, RTTI, etc) get
then? If I define two constructors for a class and split them into two
dlls, do the typeid()s of two objects constructed from each dll compare
equal? I'm not actually sure if the standard says anything about that.
I can see how this might work in an ELF shared object world because the
loader will take the first symbol definition, but I don't see how this
could work in a PE DLL world. It might even be worth a warning.
> d) Explicit dllexport/dllimport attributes on members of exported/
> imported classes are forbidden.
> e) Base classes of an exported class should be exported. Emits a
> warning otherwise. (-Wmissing-dllexport)
> f) Virtual tables and typeinfo follow the emission rules of the
> Itanium ABI, i.e. for an exported/imported dynamic class with key
> function, the needed globals are exported/imported. If there is no
> key function, they are emitted everywhere and are not exported/
> imported.
>
That's interesting! So cl.exe *does* have key functions, but only at the
DLL level, not the object file level. I think that mostly handles the
corner case I raised above.
7. Inline functions
> a) dllexport can be applied to inline function definitions. The
> function is always instantiated and exported, presumed to be
> imported by another program.
> b) dllimport can be applied to inline function definitions. Such
> functions can be expanded, but never instantiated.
> c) C99 inline functions cannot be exported or imported.
>
> 8. Templates
> a) If a class template is exported, explicit and implicit
> instantiations are also exported.
> b) Non-exported/imported class templates can be exported/imported by
> decorating an explicit instantiation.
> c) Base classes that are template specializations must be instantiated
> explicitly to export them.
> d) If a class is marked as dllexport, any specializations of class
> templates in the class hierarchy are implicitly marked as
> dllexport. This means that class templates are explicitly
> instantiated and the class's members must be defined
>
+++ lib/Sema/SemaDeclCXX.cpp
> @@ -4119,6 +4119,115 @@
> }
> }
>
> +// Returns a DLL attribute from the declaration.
> +static InheritableAttr *getDLLAttrFromDecl(Decl *D) {
> + if (DLLImportAttr *ImpAttr = D->getAttr<DLLImportAttr>())
> + return ImpAttr;
> + if (DLLExportAttr* ExpAttr = D->getAttr<DLLExportAttr>())
Minor style comment: you put the star and ampersand on the left in a couple
places and Clang/LLVM are in the star-on-the-right camp.
+++ lib/Sema/SemaDecl.cpp
> @@ -5116,6 +5116,30 @@
> isIncompleteDeclExternC(*this, NewVD))
> RegisterLocallyScopedExternCDecl(NewVD, S);
>
> + if (D.isRedeclaration() && !Previous.empty()) {
> + VarDecl *OldVD = dyn_cast<VarDecl>(Previous.getRepresentativeDecl());
> + if (OldVD) {
> + const DLLImportAttr *OldImportAttr =
> OldVD->getAttr<DLLImportAttr>();
> + const DLLExportAttr *OldExportAttr =
> OldVD->getAttr<DLLExportAttr>();
> + const DLLImportAttr *NewImportAttr =
> NewVD->getAttr<DLLImportAttr>();
> + const DLLExportAttr *NewExportAttr =
> NewVD->getAttr<DLLExportAttr>();
> +
> + bool AddsAttr = (!OldImportAttr && NewImportAttr) ||
> + (!OldExportAttr && NewExportAttr);
> + bool HasMixed = (OldImportAttr && NewExportAttr) ||
> + (OldExportAttr && NewImportAttr);
> +
> + // NB: We don't follow MSVC here and convert a declaration to
> dllexport if
> + // a redeclaration omits dllimport.
> + if (AddsAttr && !HasMixed) {
> + Diag(NewVD->getLocation(),
> diag::err_attribute_dllimpexp_redeclaration)
> + << NewVD;
> + Diag(OldVD->getLocation(), diag::note_previous_declaration);
> + NewVD->setInvalidDecl();
> + }
> + }
> + }
> +
> return NewVD;
> }
>
> @@ -6804,6 +6828,41 @@
> }
> }
>
> + if (D.isRedeclaration() && !Previous.empty()) {
> + FunctionDecl *OldFD =
> dyn_cast<FunctionDecl>(Previous.getRepresentativeDecl());
> + if (OldFD) {
> + const DLLImportAttr *OldImportAttr =
> OldFD->getAttr<DLLImportAttr>();
> + const DLLExportAttr *OldExportAttr =
> OldFD->getAttr<DLLExportAttr>();
> + const DLLImportAttr *NewImportAttr =
> NewFD->getAttr<DLLImportAttr>();
> + const DLLExportAttr *NewExportAttr =
> NewFD->getAttr<DLLExportAttr>();
> +
> + bool AddsAttr = (!OldImportAttr && NewImportAttr) ||
> + (!OldExportAttr && NewExportAttr);
> + bool HasMixed = (OldImportAttr && NewExportAttr) ||
> + (OldExportAttr && NewImportAttr);
> + bool RemovesImport = OldImportAttr &&
> + (!NewImportAttr ||
> NewImportAttr->isInherited());
> +
> + // NB: We don't follow MSVC here and convert a declaration to
> dllexport if
> + // a redeclaration omits dllimport.
> + if (!isFunctionTemplateSpecialization && AddsAttr && !HasMixed &&
> + !OldFD->isImplicit()) {
> + Diag(NewFD->getLocation(),
> diag::err_attribute_dllimpexp_redeclaration)
> + << NewFD;
> + Diag(OldFD->getLocation(), diag::note_previous_declaration);
> + NewFD->setInvalidDecl();
> + } else if (RemovesImport && !HasMixed && !NewFD->isInlined()) {
> + Diag(NewFD->getLocation(),
> +
> diag::warn_redeclaration_without_attribute_prev_attribute_ignored)
> + << NewFD << "dllimport";
> + Diag(OldFD->getLocation(), diag::note_previous_declaration);
> + Diag(OldImportAttr->getLocation(), diag::note_previous_attribute);
> + OldFD->dropAttr<DLLImportAttr>();
> + NewFD->dropAttr<DLLImportAttr>();
> + }
+ }
> + }
> +
Can you share these two blocks of code, and put the final diagnostic in an
"if (NewD->getAs<FunctionDecl>())" or something?
+++ lib/Sema/SemaDeclCXX.cpp
> @@ -4119,6 +4119,115 @@
> }
> }
>
+// Returns a DLL attribute from the declaration.
> +static InheritableAttr *getDLLAttrFromDecl(Decl *D) {
> + if (DLLImportAttr *ImpAttr = D->getAttr<DLLImportAttr>())
> + return ImpAttr;
> + if (DLLExportAttr* ExpAttr = D->getAttr<DLLExportAttr>())
> + return ExpAttr;
> + return NULL;
> +}
> +
> +static void checkRecordMemberDLLAttr(Sema &S, Decl *MemberDecl,
> + Attr *RecordAttr, CXXRecordDecl *RD)
> {
>
Woah, RD != MemberDecl->getParent(). That took me a while to figure out.
Please name it something more meaningful like ExposedDecl.
> + if (!MemberDecl)
> + return;
> +
> + InheritableAttr* MemberAttr = getDLLAttrFromDecl(MemberDecl);
>
Can you exit early here if (!RecordAttr && !MemberAttr)?
> + bool IsRecordExported = RecordAttr &&
> + RecordAttr->getKind() == attr::DLLExport;
> + bool IsMemberExported = MemberAttr &&
> + MemberAttr->getKind() == attr::DLLExport;
> +
> + // If the class has a DLL attribute, the member cannot declare one
> explicitly.
> + if (RecordAttr && MemberAttr && !MemberAttr->isInherited()) {
> + S.Diag(MemberAttr->getLocation(),
> + diag::err_attribute_dllimpexp_member_attr_not_allowed)
> + << (IsRecordExported ? 1 : 0)
> + << (IsMemberExported ? 1 : 0);
> + return;
> + }
> +
> + // Warn if type to check is a non-exported class.
> + if (IsRecordExported || IsMemberExported) {
> + if (RD && !RD->hasAttr<DLLExportAttr>()) {
> + S.Diag(MemberDecl->getLocation(),
> + diag::warn_attribute_dllexport_accessible_type)
> + << RD->getDeclName();
> + }
> + }
> +
> + // Let the member inherit the record attribute.
> + const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(MemberDecl);
> + bool Ignored = MD && MD->isDeleted();
> + if (RecordAttr && !MemberAttr && !Ignored) {
> + InheritableAttr* NewAttr =
> + cast<InheritableAttr>(RecordAttr->clone(S.getASTContext()));
> + NewAttr->setInherited(true);
> + MemberDecl->addAttr(NewAttr);
> + MemberDecl->addAttr(::new (S.getASTContext())
> + UsedAttr(MemberDecl->getLocation(),
> S.getASTContext()));
>
Shouldn't you only add UsedAttr if this is an export?
> + }
> +}
> +
> +static void forceDefinitionOfDefaultedMembers(Sema &S, CXXRecordDecl
> *Class) {
> + for (CXXRecordDecl::method_iterator I = Class->method_begin();
> + I != Class->method_end(); ++I) {
>
LLVM's coding standards say to cache the iterator end in an 'E' variable
and make the loop exit condition be I != E. This happens in
checkDLLAttributes() too.
> + CXXMethodDecl *MD = *I;
> + if (MD->isDefaulted() && !(isa<CXXDestructorDecl>(MD) &&
> MD->isTrivial()))
> + S.MarkFunctionReferenced(MD->getLocation(), MD);
> + }
> +}
> +
> +// Performs DLL attribute semantic checks on the declaration.
> +static void checkDLLAttributes(Sema &S, CXXRecordDecl *RD) {
> + if (!RD)
> + return;
> +
> + InheritableAttr* RecordAttr = getDLLAttrFromDecl(RD);
> + bool IsRecordExported = RecordAttr &&
> + RecordAttr->getKind() == attr::DLLExport;
> + bool IsRecordImported = RecordAttr &&
> + RecordAttr->getKind() == attr::DLLImport;
> +
> + if (IsRecordImported)
> + S.ForceDeclarationOfImplicitMembers(RD);
> + else if (IsRecordExported) {
> + S.ForceDeclarationOfImplicitMembers(RD);
> + forceDefinitionOfDefaultedMembers(S, RD);
> + }
>
This seems more natural as
if (RecordAttr)
S.ForceDeclarationOfImplicitMembers(RD);
if (IsRecordExported)
forceDefinitionOfDefaultedMembers(S, RD);
> +
> + if (IsRecordExported) {
> + // All base classes of an exportable class must be exportable.
>
What happens in the corner case where the separate members are exported,
but the base classes are not exported? Link errors, I guess?
> + for (CXXRecordDecl::base_class_iterator I = RD->bases_begin();
> + I != RD->bases_end(); ++I) {
> + CXXBaseSpecifier *BS = I;
> + const CXXRecordDecl *BD = BS->getType()->getAsCXXRecordDecl();
> +
> + if (BD && !BD->hasAttr<DLLExportAttr>()) {
> + S.Diag(BS->getLocStart(),
> diag::warn_attribute_dllexport_base_class)
> + << BD->getDeclName();
> + }
> + }
> + }
> +
> + // Check static data members.
> + for (CXXRecordDecl::static_data_iterator I = RD->static_data_begin();
> + I != RD->static_data_end();
> ++I) {
> + VarDecl *MD = *I;
> + CXXRecordDecl *SRD = MD->getType()->getAsCXXRecordDecl();
> + checkRecordMemberDLLAttr(S, MD, RecordAttr, SRD);
> + }
> +
> + // Check member functions.
>
... and their return values. I glossed right over getResultType().
> + for (CXXRecordDecl::method_iterator I = RD->method_begin();
> + I != RD->method_end(); ++I) {
> + CXXMethodDecl *MD = *I;
> + CXXRecordDecl *SRD = MD->getResultType()->getAsCXXRecordDecl();
>
SRD doesn't really make sense here. ExposedRD maybe?
> + checkRecordMemberDLLAttr(S, MD, RecordAttr, SRD);
> + }
> +}
> +
Nice tests!
+++ utils/TableGen/ClangAttrEmitter.cpp
>
> @@ -1335,6 +1335,7 @@
> << " default:\n"
> << " break;\n";
>
> + Record *InhClass = Records.getClass("InheritableAttr");
> for (std::vector<Record*>::iterator I = Attrs.begin(), E = Attrs.end();
> I != E; ++I) {
> Record &R = **I;
> @@ -1355,7 +1356,13 @@
> bool TDependent = R.getValueAsBit("TemplateDependent");
>
> if (!TDependent) {
> - OS << " return A->clone(C);\n";
> + if (R.isSubClassOf(InhClass)) {
> + OS << " InheritableAttr* CA = A->clone(C);\n";
> + OS << " if (CA) CA->setInherited(A->isInherited());\n";
> + OS << " return CA;\n";
> + } else {
> + OS << " return A->clone(C);\n";
> + }
>
>
Ditto what Eli said, you can probably fix the clone() body emitted in
EmitClangAttrImpl().
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130707/262dd4bd/attachment.html>
More information about the cfe-commits
mailing list