Page MenuHomePhabricator

Add full semantic support for dllimport/export attributes
AbandonedPublic

Authored by nrieck on Jul 5 2013, 6:13 PM.

Details

Reviewers
None
Summary

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.
  1. dllexport requires a definition. The extern keyword can be used to force a declaration.
  1. dllimport requires a declaration.
  1. 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.
  1. dllexport takes precedence over dllimport. If both are specified, dllimport is ignored and a warning is issued.
  1. 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. 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.
  1. 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.
  1. 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.

Diff Detail

Event Timeline

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";
+ }

  OS << "    }\n";
  continue;
}

If clone() isn't working, shouldn't you just fix it? (If I'm
misunderstanding this change, please add a comment.)

@@ -259,9 +259,9 @@

if (Triple.getOS() == llvm::Triple::Win32 ||
    Triple.getOS() == llvm::Triple::MinGW32) {
  switch (Attr.getKind()) {
  • case AttributeList::AT_DLLImport: HandleDLLImportAttr(D, Attr, S);

+ case AttributeList::AT_DLLImport: handleDLLImportAttr(D, Attr, S);

return true;
  • case AttributeList::AT_DLLExport: HandleDLLExportAttr(D, Attr, S);

+ case AttributeList::AT_DLLExport: handleDLLExportAttr(D, Attr, S);

                                  return true;
default:                          break;
}

This doesn't look like it's actually a change. If there's something
weird going on with whitespace, please commit separately.

In general, please go through your patch to remove whitespace-only changes.

+ If we have a function-scope variable without explicit storage class, then
+
give it external linkage.
+ VarDecl* VD = dyn_cast<VarDecl>(D);
+ if (!IsExported && VD && VD->getLexicalDeclContext()->isFunctionOrMethod() &&
+ VD->getStorageClass() == SC_None) {
+ VD->setStorageClass(SC_Extern);
+ }

setStorageClass changes the storage class *as written*. That doesn't
seem appropriate here.

+ // Attribute cannot be applied to decls with non-external linkage.
+ if (!ND->isExternallyVisible()) {
+ bool IsExported = (Attr.getKind() == AttributeList::AT_DLLExport);
+ S.Diag(ND->getLocation(), diag::err_attribute_dllimpexp_not_extern_linkage)
+ << ND->getDeclName() << (IsExported ? 1 : 0);
+ ND->setInvalidDecl();
+ return false;

}

Not sure isExternallyVisible() is the right predicate here; it doesn't
really make sense to apply DLLExport to e.g. a static local variable.
Maybe hasExternalFormalLinkage()?

This patch is a little overwhelming overall: there are a lot of subtle
changes. Can you split out the non-RecordDecl-related changes into a
separate patch?

-Eli

nrieck abandoned this revision.May 30 2014, 10:23 AM

This revision has been superseded by incremental patches which have made it into ToT.