dllexport/dllimport should be allowed for full classes/structs

[rubenvb]

Bugzilla Link	11170
Resolution	FIXED
Resolved on	Jun 26, 2014 13:31
Version	trunk
OS	Windows NT
Blocks	#7187 llvm/llvm-bugzilla-archive#13707 llvm/llvm-bugzilla-archive#18256 llvm/llvm-bugzilla-archive#19150
CC	@asl,@majnemer,@ehsan,@zmodem,@Ivan171,@jrmuizel,@tritao,@oscarfv,@rnk,@timurrrr

Extended Description

Currently, Clang does not support dll[ex|im]porting full classes, although this is common practice and perfectly legal (see for example the Qt codebase).

[llvmbot]

Didn't bug 10978 help you?

[rubenvb]

Didn't bug 10978 help you?

No, that only removed the "unknown attribute" warnings for x86_64. This bug is for dllexporting whole classes, as in:

Testcase is quite simple:

class __declspec(dllexport) A
{};

Clang output:

testcase.cpp:2:18: warning: 'dllexport' attribute only applies to variables and
functions
class __declspec(dllexport) A
^
:133:38: note: expanded from:
#define __declspec(a) attribute((a))
^
1 warning generated.

See http://msdn.microsoft.com/en-us/library/81h27t8c.aspx for why this is wrong.

[tritao]

Confirmed, as of r156715.

[tritao]

Relaxes the restrictions on DLL exports / imports

[tritao]

Refactor of DLL attributes handling

[rubenvb]

I'd like to try your patch, but I get this when I try to apply it:

Ruben@Ruben_Laptop /m/Development/Source/LLVM/tools/clang
$ patch -p0 < ../../DLL-Attributes.patch.txt
(Stripping trailing CRs from patch.)
patching file include/clang/AST/DeclCXX.h
Hunk #​1 succeeded at 779 with fuzz 1.
(Stripping trailing CRs from patch.)
patching file include/clang/Basic/DiagnosticSemaKinds.td
Hunk #​1 succeeded at 1494 (offset 1 line).
patch: **** malformed patch at line 53: Index: lib/AST/Decl.cpp

[tritao]

Regenerated DLL attributes patch

[tritao]

I added a regenerated patch (contents should be the same). Let me know if that works.

[rubenvb]

I added a regenerated patch (contents should be the same). Let me know if that
works.

This patch applies cleanly to SVN head.

I get a compiler error (and a bunch of warnings) though:

[ 79%] Building CXX object tools/clang/lib/Sema/CMakeFiles/clangSema.dir/SemaDeclCXX.cpp.obj
M:\Development\Source\LLVM\tools\clang\lib\Sema\SemaDeclCXX.cpp: In function 'void HandleRecordMemberDLLAttr(clang::Decl*, clang::Sema*, clang::Attr*, clang::CXXRecordDecl*)':
M:\Development\Source\LLVM\tools\clang\lib\Sema\SemaDeclCXX.cpp:4762:46: warning: comparison between 'enum clang::attr::Kind' and 'enum clang::AttributeList::Kind' [-Wenum-compare]
M:\Development\Source\LLVM\tools\clang\lib\Sema\SemaDeclCXX.cpp:4784:47: warning: comparison between 'enum clang::attr::Kind' and 'enum clang::AttributeList::Kind' [-Wenum-compare]
M:\Development\Source\LLVM\tools\clang\lib\Sema\SemaDeclCXX.cpp: In function 'void HandleDLLAttrs(clang::CXXRecordDecl*, clang::Sema*)':
M:\Development\Source\LLVM\tools\clang\lib\Sema\SemaDeclCXX.cpp:4802:47: warning: comparison between 'enum clang::attr::Kind' and 'enum clang::AttributeList::Kind' [-Wenum-compare]
M:\Development\Source\LLVM\tools\clang\lib\Sema\SemaDeclCXX.cpp:4830:51: error:
'HandleClassMemberDLLAttr' was not declared in this scope
M:\Development\Source\LLVM\tools\clang\lib\Sema\SemaDeclCXX.cpp:4838:51: error:
'HandleClassMemberDLLAttr' was not declared in this scope
M:\Development\Source\LLVM\tools\clang\lib\Sema\SemaDeclCXX.cpp:4846:51: error:
'HandleClassMemberDLLAttr' was not declared in this scope
M:\Development\Source\LLVM\tools\clang\lib\Sema\SemaDeclCXX.cpp: At global scope:
M:\Development\Source\LLVM\tools\clang\lib\Sema\SemaDeclCXX.cpp:4759:13: warning: 'void HandleRecordMemberDLLAttr(clang::Decl*, clang::Sema*, clang::Attr*, clang::CXXRecordDecl*)' defined but not used [-Wunused-function]
mingw32-make[2]: *** [tools/clang/lib/Sema/CMakeFiles/clangSema.dir/SemaDeclCXX.cpp.obj] Error 1
mingw32-make[1]: *** [tools/clang/lib/Sema/CMakeFiles/clangSema.dir/all] Error 2
mingw32-make: *** [all] Error 2

[tritao]

Damn. To make it compile change the call from HandleClassMemberDLLAttr() to HandleRecordMemberDLLAttr() and it should work.

Seems also there's a warning comparing the enum values, I'll take a look into it later.

[rubenvb]

With Clang r161110, your patch applied (and the typos corrected), I get a crash compiling Qt after a large number of dllimport ignored warnings:

Example warning:
....\include\QtCore/../../../../Source/Qt/src/corelib/tools/qchar.h:385:20: warning:
'setRow' redeclared without dllimport attribute: previous dllimport
ignored
inline void QChar::setRow(uchar arow)
^

Stack dump:
0. Program arguments: M:/Development/mingw32-dw2/bin/clang++.exe -cc1 -triple i686-pc-mingw32 -S -disable-free -disable-llvm-verifier -main-file-name qtmain_win.cpp -mrelocation-model static -mdisable-fp-elim -fmath-errno -mconstructor-aliases -target-cpu pentium4 -momit-leaf-frame-pointer -g -coverage-file R:/qtmain_win-733541.s -resource-dir M:/Development/mingw32-dw2/bin..\lib\clang\3.2 -D QT_THREAD_SUPPORT -D UNICODE -D QT_LARGEFILE_SUPPORT -D QT_NEEDS_QMAIN -D QT_NO_CAST_TO_ASCII -D QT_ASCII_CAST_WARNINGS -D QT3_SUPPORT -D QT_MOC_COMPAT -D QT_USE_QSTRINGBUILDER -D _USE_MATH_DEFINES -D QT_HAVE_MMX -D QT_HAVE_SSE -D QT_HAVE_MMXEXT -D QT_HAVE_SSE2 -I ....\include -I m:\Development\Source\Qt\src\winmain\tmp -I ....\include\QtCore -I m:\Development\x86\Qt\include\qtmain -I tmp\rcc\debug_shared -I m:\Development\Source\Qt\src\winmain\tmp -I ....\include\ActiveQt -I tmp\moc\debug_shared -I m:\Development\Source\Qt\src\winmain -I . -I m:\Development\Source\Qt\mkspecs\win32-g++-clang -fmodule-cache-path R:\clang-module-cache -Wall -Wextra -fdeprecated-macro -fno-dwarf-directory-asm -ferror-limit 19 -fmessage-length 80 -mstackrealign -fno-use-cxa-atexit -fobjc-runtime=gcc -fcxx-exceptions -fexceptions -fdiagnostics-show-option -fcolor-diagnostics -o R:/qtmain_win-733541.s -x c++ m:\Development\Source\Qt\src\winmain\qtmain_win.cpp

1.  M:/Development/mingw32-dw2/bin\..\lib\clang\3.2/../../../include/c++/4.6.3\bits/ostream_insert.h:117:79: current parser token ';'
   

2.  M:/Development/mingw32-dw2/bin\..\lib\clang\3.2/../../../include/c++/4.6.3\bits/ostream_insert.h:37:1: parsing namespace 'std'
   

clang++: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 3.2 (trunk 161110)
Target: i686-pc-mingw32
Thread model: posix

I can attach the other crash files, but only if that helps. I could also give you instructions to test building Qt yourself if you think that's more helpful.

[tritao]

Thanks for testing this, it's really helpful. I'm working on other MS ABI patches, but once I finish them I'll look into this again.

About the crash, a real stack trace would be more helpful in figuring out the problem.

[rubenvb]

Clang crash diagnostic msg
Attached the "diagnostic msg" for the crash. (zip was too large, used 7z instead)

[llvmbot]

Taking this as part of a larger patch for proper dllexport/dllimport support.

I use the same idea of propagating the attributes to the members. But this doesn't work with class templates because the inherited flag is not preserved when the attributes are cloned for the instantiation. Might be a bug/oversight in tablegen.

[rubenvb]

Any updates on this? Clang top of trunk still warns on unused attribute, and it seems more is broken with dllexport, but that's for later.

I (and many people) would already be happy with non-template class dllexport, I think, and this would be a great step forward.

[oscarfv]

Ruben,

Nico's work is in http://llvm-reviews.chandlerc.com/D1110 which was just accepted and he made a series of commits today (See r199203..r199207 on the LLVM repo). He reverted the larger patch short afterwards due to some problem on Clang but it seems that, at last, this issue is going to be resolved.

[llvmbot]

D1110 doesn't actually fix this bug, but is a requirement. I have this bug mostly fixed (a problem are virtual classes mainly because of ABI issues). I just have to break it down into more review-friendly pieces.

[timurrrr]

Can you please elaborate on what are the problems with virtual classes?
Feel free to file separate bugs for me to take a look.

[jrmuizel]

D1110 doesn't actually fix this bug, but is a requirement. I have this bug
mostly fixed (a problem are virtual classes mainly because of ABI issues). I
just have to break it down into more review-friendly pieces.

Any chance you can post your work in progress? This blocking building mozilla so it would be nice to try with your patches.

[rnk]

I've searched by inbox for pending patches to review, but I don't see any. I'm happy to review away when one is out. Nico already laid the groundwork in LLVM, so this should be a Small Matter of Code in Clang.

[rnk]

*** Bug llvm/llvm-bugzilla-archive#19516 has been marked as a duplicate of this bug. ***

[zmodem]

*** Bug llvm/llvm-bugzilla-archive#19738 has been marked as a duplicate of this bug. ***

[zmodem]

This is now supported (modulo bugs).

[tritao]

mentioned in issue llvm/llvm-bugzilla-archive#13707

[rnk]

mentioned in issue llvm/llvm-bugzilla-archive#18256

[rnk]

mentioned in issue llvm/llvm-bugzilla-archive#18256

[jrmuizel]

mentioned in issue llvm/llvm-bugzilla-archive#19150

[rnk]

mentioned in issue llvm/llvm-bugzilla-archive#19516

[rnk]

mentioned in issue llvm/llvm-bugzilla-archive#19726

[zmodem]

mentioned in issue llvm/llvm-bugzilla-archive#19738

[llvmbot]

mentioned in issue #7187