<div dir="ltr">On Fri, Jul 5, 2013 at 9:13 PM, Nico Rieck <span dir="ltr"><<a href="mailto:nico.rieck@gmail.com" target="_blank">nico.rieck@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">This is the first patch in a series to get full support for the dllexport/dllimport attributes and handles the semantic side.<br>
<br>
In particular it:<br>
- Adds support for class-wide export/import attributes (PR11170)<br>
- Handles dllexport/dllimport on inline functions and templates<br>
- Extends tests for globals and functions.<br>
<br>
It deviates from MSVC's behavior in the following cases:<br>
- 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.<br>
- 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.<br>
<br>
In some cases a dllimport/dllexport attribute is ignored. If this happens on<br>
a member of an exported/imported class, we need a way to drop the attribute.<br>
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.<br>
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.<br>
<br>
<br>
The semantics this patch should cover are:<br>
<br>
1. dllexport and dllimport can be applied to functions, static data<br>
members and global variables. Both can also be applied to typedefs<br>
and enums (without effect) if -fms-extensions is specified, for all<br>
other cases the attributes are ignored and a warning is issued. It is<br>
an error to apply them to non-static data members.<br>
<br>
2. dllexport requires a definition. The extern keyword can be used to<br>
force a declaration.<br></blockquote><div><br></div><div style>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
3. dllimport requires a declaration.<br>
<br>
4. dllexport exposes the function or variable with its decorated name.<br>
a) For C++, this includes name mangling.<br>
b) For C or extern "C" functions, this includes platform-specific<br>
decoration for the calling convention.<br>
<br>
5. dllexport takes precedence over dllimport. If both are specified,<br>
dllimport is ignored and a warning is issued.<br>
<br>
6. Classes<br>
a) When applied to a class, dllexport or dllimport affect all static<br>
and non-static member functions and all static data members,<br>
including all non-deleted special members.<br>
b) Everything that is accessible to the DLL's client according to C++<br>
access rules is exported (including private data members referenced<br>
in inline functions).<br>
c) Members can be imported or exported selectively.<br></blockquote><div><br></div><div style>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.</div>
<div style><br></div><div style>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
d) Explicit dllexport/dllimport attributes on members of exported/<br>
imported classes are forbidden.<br>
e) Base classes of an exported class should be exported. Emits a<br>
warning otherwise. (-Wmissing-dllexport)<br>
f) Virtual tables and typeinfo follow the emission rules of the<br>
Itanium ABI, i.e. for an exported/imported dynamic class with key<br>
function, the needed globals are exported/imported. If there is no<br>
key function, they are emitted everywhere and are not exported/<br>
imported.<br></blockquote><div><br></div><div style>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
7. Inline functions<br>
a) dllexport can be applied to inline function definitions. The<br>
function is always instantiated and exported, presumed to be<br>
imported by another program.<br>
b) dllimport can be applied to inline function definitions. Such<br>
functions can be expanded, but never instantiated.<br>
c) C99 inline functions cannot be exported or imported.<br>
<br>
8. Templates<br>
a) If a class template is exported, explicit and implicit<br>
instantiations are also exported.<br>
b) Non-exported/imported class templates can be exported/imported by<br>
decorating an explicit instantiation.<br>
c) Base classes that are template specializations must be instantiated<br>
explicitly to export them.<br>
d) If a class is marked as dllexport, any specializations of class<br>
templates in the class hierarchy are implicitly marked as<br>
dllexport. This means that class templates are explicitly<br>
instantiated and the class's members must be defined<br></blockquote><div><br></div><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+++ lib/Sema/SemaDeclCXX.cpp<br>@@ -4119,6 +4119,115 @@<br> }<br> }<br> <br>+// Returns a DLL attribute from the declaration.<br>+static InheritableAttr *getDLLAttrFromDecl(Decl *D) {<br>+ if (DLLImportAttr *ImpAttr = D->getAttr<DLLImportAttr>())<br>
+ return ImpAttr;<br>+ if (DLLExportAttr* ExpAttr = D->getAttr<DLLExportAttr>())</blockquote></div><div><br></div><div style>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.</div>
<div style><br></div><div style><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+++ lib/Sema/SemaDecl.cpp<br>
@@ -5116,6 +5116,30 @@<br> isIncompleteDeclExternC(*this, NewVD))<br> RegisterLocallyScopedExternCDecl(NewVD, S);<br> <br>+ if (D.isRedeclaration() && !Previous.empty()) {<br>+ VarDecl *OldVD = dyn_cast<VarDecl>(Previous.getRepresentativeDecl());<br>
+ if (OldVD) {<br>+ const DLLImportAttr *OldImportAttr = OldVD->getAttr<DLLImportAttr>();<br>+ const DLLExportAttr *OldExportAttr = OldVD->getAttr<DLLExportAttr>();<br>+ const DLLImportAttr *NewImportAttr = NewVD->getAttr<DLLImportAttr>();<br>
+ const DLLExportAttr *NewExportAttr = NewVD->getAttr<DLLExportAttr>();<br>+<br>+ bool AddsAttr = (!OldImportAttr && NewImportAttr) ||<br>+ (!OldExportAttr && NewExportAttr);<br>
+ bool HasMixed = (OldImportAttr && NewExportAttr) ||<br>+ (OldExportAttr && NewImportAttr);<br>+<br>+ // NB: We don't follow MSVC here and convert a declaration to dllexport if<br>
+ // a redeclaration omits dllimport.<br>+ if (AddsAttr && !HasMixed) {<br>+ Diag(NewVD->getLocation(), diag::err_attribute_dllimpexp_redeclaration)<br>+ << NewVD;<br>+ Diag(OldVD->getLocation(), diag::note_previous_declaration);<br>
+ NewVD->setInvalidDecl();<br>+ }<br>+ }<br>+ }<br>+<br> return NewVD;<br> }<br> <br>@@ -6804,6 +6828,41 @@<br> }<br> }<br> <br>+ if (D.isRedeclaration() && !Previous.empty()) {<br>+ FunctionDecl *OldFD = dyn_cast<FunctionDecl>(Previous.getRepresentativeDecl());<br>
+ if (OldFD) {<br>+ const DLLImportAttr *OldImportAttr = OldFD->getAttr<DLLImportAttr>();<br>+ const DLLExportAttr *OldExportAttr = OldFD->getAttr<DLLExportAttr>();<br>+ const DLLImportAttr *NewImportAttr = NewFD->getAttr<DLLImportAttr>();<br>
+ const DLLExportAttr *NewExportAttr = NewFD->getAttr<DLLExportAttr>();<br>+<br>+ bool AddsAttr = (!OldImportAttr && NewImportAttr) ||<br>+ (!OldExportAttr && NewExportAttr);<br>
+ bool HasMixed = (OldImportAttr && NewExportAttr) ||<br>+ (OldExportAttr && NewImportAttr);<br>+ bool RemovesImport = OldImportAttr &&<br>+ (!NewImportAttr || NewImportAttr->isInherited());<br>
+<br>+ // NB: We don't follow MSVC here and convert a declaration to dllexport if<br>+ // a redeclaration omits dllimport.<br>+ if (!isFunctionTemplateSpecialization && AddsAttr && !HasMixed &&<br>
+ !OldFD->isImplicit()) {<br>+ Diag(NewFD->getLocation(), diag::err_attribute_dllimpexp_redeclaration)<br>+ << NewFD;<br>+ Diag(OldFD->getLocation(), diag::note_previous_declaration);<br>
+ NewFD->setInvalidDecl();<br>+ } else if (RemovesImport && !HasMixed && !NewFD->isInlined()) {<br>+ Diag(NewFD->getLocation(),<br>+ diag::warn_redeclaration_without_attribute_prev_attribute_ignored)<br>
+ << NewFD << "dllimport";<br>+ Diag(OldFD->getLocation(), diag::note_previous_declaration);<br>+ Diag(OldImportAttr->getLocation(), diag::note_previous_attribute);<br>+ OldFD->dropAttr<DLLImportAttr>();<br>
+ NewFD->dropAttr<DLLImportAttr>();<br>+ } </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ }<br>+ }<br>+</blockquote><div><br></div><div style>Can you share these two blocks of code, and put the final diagnostic in an "if (NewD->getAs<FunctionDecl>())" or something?</div><div><br></div>
<div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+++ lib/Sema/SemaDeclCXX.cpp<br>@@ -4119,6 +4119,115 @@<br>
}<br> }<br> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+// Returns a DLL attribute from the declaration.<br>
+static InheritableAttr *getDLLAttrFromDecl(Decl *D) {<br>+ if (DLLImportAttr *ImpAttr = D->getAttr<DLLImportAttr>())<br>+ return ImpAttr;<br>+ if (DLLExportAttr* ExpAttr = D->getAttr<DLLExportAttr>())<br>
+ return ExpAttr;<br>+ return NULL;<br>+}<br>+<br>+static void checkRecordMemberDLLAttr(Sema &S, Decl *MemberDecl,<br>+ Attr *RecordAttr, CXXRecordDecl *RD) {<br></blockquote><div>
<br></div><div>Woah, RD != MemberDecl->getParent(). That took me a while to figure out. Please name it something more meaningful like ExposedDecl.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ if (!MemberDecl)<br>+ return;<br>+<br>+ InheritableAttr* MemberAttr = getDLLAttrFromDecl(MemberDecl);<br></blockquote><div><br></div><div style>Can you exit early here if (!RecordAttr && !MemberAttr)?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+ bool IsRecordExported = RecordAttr &&<br>
+ RecordAttr->getKind() == attr::DLLExport;<br>+ bool IsMemberExported = MemberAttr &&<br>+ MemberAttr->getKind() == attr::DLLExport;<br>+<br>+ // If the class has a DLL attribute, the member cannot declare one explicitly.<br>
+ if (RecordAttr && MemberAttr && !MemberAttr->isInherited()) {<br>+ S.Diag(MemberAttr->getLocation(),<br>+ diag::err_attribute_dllimpexp_member_attr_not_allowed)<br>+ << (IsRecordExported ? 1 : 0)<br>
+ << (IsMemberExported ? 1 : 0);<br>+ return;<br>+ }<br>+<br>+ // Warn if type to check is a non-exported class.<br>+ if (IsRecordExported || IsMemberExported) {<br>+ if (RD && !RD->hasAttr<DLLExportAttr>()) {<br>
+ S.Diag(MemberDecl->getLocation(),<br>+ diag::warn_attribute_dllexport_accessible_type)<br>+ << RD->getDeclName();<br>+ }<br>+ }<br>+<br>+ // Let the member inherit the record attribute.<br>
+ const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(MemberDecl);<br>+ bool Ignored = MD && MD->isDeleted();<br>+ if (RecordAttr && !MemberAttr && !Ignored) {<br>+ InheritableAttr* NewAttr =<br>
+ cast<InheritableAttr>(RecordAttr->clone(S.getASTContext()));<br>+ NewAttr->setInherited(true);<br>+ MemberDecl->addAttr(NewAttr);<br>+ MemberDecl->addAttr(::new (S.getASTContext())<br>+ UsedAttr(MemberDecl->getLocation(), S.getASTContext()));<br>
</blockquote><div><br></div><div style>Shouldn't you only add UsedAttr if this is an export?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ }<br>+}<br>+<br>+static void forceDefinitionOfDefaultedMembers(Sema &S, CXXRecordDecl *Class) {<br>+ for (CXXRecordDecl::method_iterator I = Class->method_begin();<br>+ I != Class->method_end(); ++I) {<br>
</blockquote><div><br></div><div>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.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+ CXXMethodDecl *MD = *I;<br>+ if (MD->isDefaulted() && !(isa<CXXDestructorDecl>(MD) && MD->isTrivial()))<br>
+ S.MarkFunctionReferenced(MD->getLocation(), MD);<br>+ }<br>+}<br>+<br>+// Performs DLL attribute semantic checks on the declaration.<br>+static void checkDLLAttributes(Sema &S, CXXRecordDecl *RD) {<br>+ if (!RD)<br>
+ return;<br>+<br>+ InheritableAttr* RecordAttr = getDLLAttrFromDecl(RD);<br>+ bool IsRecordExported = RecordAttr &&<br>+ RecordAttr->getKind() == attr::DLLExport;<br>+ bool IsRecordImported = RecordAttr &&<br>
+ RecordAttr->getKind() == attr::DLLImport;<br>+<br>+ if (IsRecordImported)<br>+ S.ForceDeclarationOfImplicitMembers(RD);<br>+ else if (IsRecordExported) {<br>+ S.ForceDeclarationOfImplicitMembers(RD);<br>
+ forceDefinitionOfDefaultedMembers(S, RD);<br>+ }<br></blockquote><div><br></div><div style>This seems more natural as</div><div style>if (RecordAttr)</div><div style> S.ForceDeclarationOfImplicitMembers(RD);<br></div>
<div style>if (IsRecordExported)</div><div style> forceDefinitionOfDefaultedMembers(S, RD);</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+<br>+ if (IsRecordExported) {<br>+ // All base classes of an exportable class must be exportable.<br></blockquote><div><br></div><div style>What happens in the corner case where the separate members are exported, but the base classes are not exported? Link errors, I guess?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+ for (CXXRecordDecl::base_class_iterator I = RD->bases_begin();<br>
+ I != RD->bases_end(); ++I) {<br>+ CXXBaseSpecifier *BS = I;<br>+ const CXXRecordDecl *BD = BS->getType()->getAsCXXRecordDecl();<br>+<br>+ if (BD && !BD->hasAttr<DLLExportAttr>()) {<br>
+ S.Diag(BS->getLocStart(), diag::warn_attribute_dllexport_base_class)<br>+ << BD->getDeclName();<br>+ }<br>+ }<br>+ }<br>+<br>+ // Check static data members.<br>+ for (CXXRecordDecl::static_data_iterator I = RD->static_data_begin();<br>
+ I != RD->static_data_end(); ++I) {<br>+ VarDecl *MD = *I;<br>+ CXXRecordDecl *SRD = MD->getType()->getAsCXXRecordDecl();<br>+ checkRecordMemberDLLAttr(S, MD, RecordAttr, SRD);<br>
+ }<br>+<br>+ // Check member functions.<br></blockquote><div><br></div><div style>... and their return values. I glossed right over getResultType().</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ for (CXXRecordDecl::method_iterator I = RD->method_begin();<br>+ I != RD->method_end(); ++I) {<br>+ CXXMethodDecl *MD = *I;<br>+ CXXRecordDecl *SRD = MD->getResultType()->getAsCXXRecordDecl();<br>
</blockquote><div><br></div><div style>SRD doesn't really make sense here. ExposedRD maybe?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ checkRecordMemberDLLAttr(S, MD, RecordAttr, SRD);<br>+ }<br>+}<br>+</blockquote></div><div><br></div><div><br></div><div style>Nice tests!</div><div style><br></div><div style><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+++ utils/TableGen/ClangAttrEmitter.cpp<br><blockquote>@@ -1335,6 +1335,7 @@<br> << " default:\n"<br> << " break;\n";<br> <br>+ Record *InhClass = Records.getClass("InheritableAttr");<br>
for (std::vector<Record*>::iterator I = Attrs.begin(), E = Attrs.end();<br> I != E; ++I) {<br> Record &R = **I;<br>@@ -1355,7 +1356,13 @@<br> bool TDependent = R.getValueAsBit("TemplateDependent");<br>
<br> if (!TDependent) {<br>- OS << " return A->clone(C);\n";<br>+ if (R.isSubClassOf(InhClass)) {<br>+ OS << " InheritableAttr* CA = A->clone(C);\n";<br>
+ OS << " if (CA) CA->setInherited(A->isInherited());\n";<br>+ OS << " return CA;\n";<br>+ } else {<br>+ OS << " return A->clone(C);\n";<br>
+ }</blockquote></blockquote><div><br></div><div style>Ditto what Eli said, you can probably fix the clone() body emitted in EmitClangAttrImpl(). </div><div><br></div></div></div></div></div></div>