[PATCH] D16632: clang-cl: Take dllexport from original function decl into account
This revision was automatically updated to reflect the committed changes. Closed by commit rL326990: Propagate DLLAttr to friend re-declarations of member functions (authored by sberg, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D16632?vs=137374&id=137548#toc Repository: rL LLVM https://reviews.llvm.org/D16632 Files: cfe/trunk/lib/Sema/SemaDeclCXX.cpp cfe/trunk/test/CodeGenCXX/dllexport.cpp Index: cfe/trunk/test/CodeGenCXX/dllexport.cpp === --- cfe/trunk/test/CodeGenCXX/dllexport.cpp +++ cfe/trunk/test/CodeGenCXX/dllexport.cpp @@ -290,6 +290,16 @@ __declspec(dllexport) void friend1() {} void friend2() {} +// MSC-DAG: define dso_local dllexport void @"\01?func@Befriended@@SAXXZ"() +// GNU-DAG: define dso_local dllexport void @_ZN10Befriended4funcEv() +struct __declspec(dllexport) Befriended { + static void func(); + struct Befriending { +friend void Befriended::func(); + }; +}; +void Befriended::func() {} + // Implicit declarations can be redeclared with dllexport. // MSC-DAG: define dso_local dllexport noalias i8* @"\01??2@{{YAPAXI|YAPEAX_K}}@Z"( // GNU-DAG: define dso_local dllexport noalias i8* @_Znw{{[yj]}}( Index: cfe/trunk/lib/Sema/SemaDeclCXX.cpp === --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp @@ -5685,6 +5685,21 @@ cast(ClassAttr->clone(getASTContext())); NewAttr->setInherited(true); Member->addAttr(NewAttr); + + if (MD) { +// Propagate DLLAttr to friend re-declarations of MD that have already +// been constructed. +for (FunctionDecl *FD = MD->getMostRecentDecl(); FD; + FD = FD->getPreviousDecl()) { + if (FD->getFriendObjectKind() == Decl::FOK_None) +continue; + assert(!getDLLAttr(FD) && + "friend re-decl should not already have a DLLAttr"); + NewAttr = cast(ClassAttr->clone(getASTContext())); + NewAttr->setInherited(true); + FD->addAttr(NewAttr); +} + } } } Index: cfe/trunk/test/CodeGenCXX/dllexport.cpp === --- cfe/trunk/test/CodeGenCXX/dllexport.cpp +++ cfe/trunk/test/CodeGenCXX/dllexport.cpp @@ -290,6 +290,16 @@ __declspec(dllexport) void friend1() {} void friend2() {} +// MSC-DAG: define dso_local dllexport void @"\01?func@Befriended@@SAXXZ"() +// GNU-DAG: define dso_local dllexport void @_ZN10Befriended4funcEv() +struct __declspec(dllexport) Befriended { + static void func(); + struct Befriending { +friend void Befriended::func(); + }; +}; +void Befriended::func() {} + // Implicit declarations can be redeclared with dllexport. // MSC-DAG: define dso_local dllexport noalias i8* @"\01??2@{{YAPAXI|YAPEAX_K}}@Z"( // GNU-DAG: define dso_local dllexport noalias i8* @_Znw{{[yj]}}( Index: cfe/trunk/lib/Sema/SemaDeclCXX.cpp === --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp @@ -5685,6 +5685,21 @@ cast(ClassAttr->clone(getASTContext())); NewAttr->setInherited(true); Member->addAttr(NewAttr); + + if (MD) { +// Propagate DLLAttr to friend re-declarations of MD that have already +// been constructed. +for (FunctionDecl *FD = MD->getMostRecentDecl(); FD; + FD = FD->getPreviousDecl()) { + if (FD->getFriendObjectKind() == Decl::FOK_None) +continue; + assert(!getDLLAttr(FD) && + "friend re-decl should not already have a DLLAttr"); + NewAttr = cast(ClassAttr->clone(getASTContext())); + NewAttr->setInherited(true); + FD->addAttr(NewAttr); +} + } } } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D16632: clang-cl: Take dllexport from original function decl into account
rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D16632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D16632: clang-cl: Take dllexport from original function decl into account
sberg updated this revision to Diff 137374. sberg added a comment. Turns out DLLAttr-inherited-from-class is only added to members during Sema::CheckCompletedClass -> Sema::checkClassLevelDLLAttribute, when friend re-decls of those members may already have been created. https://reviews.llvm.org/D16632 Files: clang/lib/Sema/SemaDeclCXX.cpp clang/test/CodeGenCXX/dllexport.cpp Index: clang/test/CodeGenCXX/dllexport.cpp === --- clang/test/CodeGenCXX/dllexport.cpp +++ clang/test/CodeGenCXX/dllexport.cpp @@ -290,6 +290,16 @@ __declspec(dllexport) void friend1() {} void friend2() {} +// MSC-DAG: define dso_local dllexport void @"\01?func@Befriended@@SAXXZ"() +// GNU-DAG: define dso_local dllexport void @_ZN10Befriended4funcEv() +struct __declspec(dllexport) Befriended { + static void func(); + struct Befriending { +friend void Befriended::func(); + }; +}; +void Befriended::func() {} + // Implicit declarations can be redeclared with dllexport. // MSC-DAG: define dso_local dllexport noalias i8* @"\01??2@{{YAPAXI|YAPEAX_K}}@Z"( // GNU-DAG: define dso_local dllexport noalias i8* @_Znw{{[yj]}}( Index: clang/lib/Sema/SemaDeclCXX.cpp === --- clang/lib/Sema/SemaDeclCXX.cpp +++ clang/lib/Sema/SemaDeclCXX.cpp @@ -5688,6 +5688,21 @@ cast(ClassAttr->clone(getASTContext())); NewAttr->setInherited(true); Member->addAttr(NewAttr); + + if (MD) { +// Propagate DLLAttr to friend re-declarations of MD that have already +// been constructed. +for (FunctionDecl *FD = MD->getMostRecentDecl(); FD; + FD = FD->getPreviousDecl()) { + if (FD->getFriendObjectKind() == Decl::FOK_None) +continue; + assert(!getDLLAttr(FD) && + "friend re-decl should not already have a DLLAttr"); + NewAttr = cast(ClassAttr->clone(getASTContext())); + NewAttr->setInherited(true); + FD->addAttr(NewAttr); +} + } } } Index: clang/test/CodeGenCXX/dllexport.cpp === --- clang/test/CodeGenCXX/dllexport.cpp +++ clang/test/CodeGenCXX/dllexport.cpp @@ -290,6 +290,16 @@ __declspec(dllexport) void friend1() {} void friend2() {} +// MSC-DAG: define dso_local dllexport void @"\01?func@Befriended@@SAXXZ"() +// GNU-DAG: define dso_local dllexport void @_ZN10Befriended4funcEv() +struct __declspec(dllexport) Befriended { + static void func(); + struct Befriending { +friend void Befriended::func(); + }; +}; +void Befriended::func() {} + // Implicit declarations can be redeclared with dllexport. // MSC-DAG: define dso_local dllexport noalias i8* @"\01??2@{{YAPAXI|YAPEAX_K}}@Z"( // GNU-DAG: define dso_local dllexport noalias i8* @_Znw{{[yj]}}( Index: clang/lib/Sema/SemaDeclCXX.cpp === --- clang/lib/Sema/SemaDeclCXX.cpp +++ clang/lib/Sema/SemaDeclCXX.cpp @@ -5688,6 +5688,21 @@ cast(ClassAttr->clone(getASTContext())); NewAttr->setInherited(true); Member->addAttr(NewAttr); + + if (MD) { +// Propagate DLLAttr to friend re-declarations of MD that have already +// been constructed. +for (FunctionDecl *FD = MD->getMostRecentDecl(); FD; + FD = FD->getPreviousDecl()) { + if (FD->getFriendObjectKind() == Decl::FOK_None) +continue; + assert(!getDLLAttr(FD) && + "friend re-decl should not already have a DLLAttr"); + NewAttr = cast(ClassAttr->clone(getASTContext())); + NewAttr->setInherited(true); + FD->addAttr(NewAttr); +} + } } } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16632: clang-cl: Take dllexport from original function decl into account
sberg added a comment. Yeah, my first naive finding when encountering the error was that it went away when unconditionally using FD->getCanonicalDecl() instead of FD in that if-else-if block. But that caused other parts of clang-test to fail. The current version passes all tests (happens to), but indeed does not feel "right" at all. http://reviews.llvm.org/D16632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16632: clang-cl: Take dllexport from original function decl into account
hans added a comment. Hi Stephan, I would rather see that we could get this right in the AST. The problem is that the Befriended::func() definition doesn't have dllexport attached: `-CXXMethodDecl 0x5ba1cf0 parent 0x5b4f288 prev 0x5b4f750 col:18 used func 'void (void)' `-CompoundStmt 0x5ba1de0 If I drop the friend declaration, it looks like this: `-CXXMethodDecl 0x6af2a08 parent 0x6aa0288 prev 0x6aa04c0 col:18 used func 'void (void)' |-CompoundStmt 0x6af2b30 `-DLLExportAttr 0x6af2b20 Inherited It's a tricky situation though. I suspect what's happening is that when we build the AST for the friend decl, we haven't yet propagated the dllexport attribute from the 'Befriended' class to 'func'. If the attribute was put directly on 'func', the friend declaration would pick it up: struct Befriended { static void __declspec(dllexport) func(); struct Befriending { friend void Befriended::func(); }; }; void Befriended::func() {} |-CXXRecordDecl 0x6fe4288 line:1:9 struct Befriended definition | |-CXXRecordDecl 0x6fe43a0 col:9 implicit struct Befriended | |-CXXMethodDecl 0x6fe4480 col:37 func 'void (void)' static | | `-DLLExportAttr 0x6fe4528 | `-CXXRecordDecl 0x6fe4560 line:3:10 struct Befriending definition | |-CXXRecordDecl 0x6fe4670 col:10 implicit struct Befriending | `-FriendDecl 0x6fe4868 col:29 | `-CXXMethodDecl 0x6fe4740 parent 0x6fe4288 prev 0x6fe4480 col:29 func 'void (void)' | `-DLLExportAttr 0x6fe4858 Inherited `-CXXMethodDecl 0x6fe48c8 parent 0x6fe4288 prev 0x6fe4740 col:18 func 'void (void)' |-CompoundStmt 0x7036710 `-DLLExportAttr 0x6fe49e0 Inherited We need to figure out the ordering of applying the dllexport class attribute vs. parsing inner classes. http://reviews.llvm.org/D16632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16632: clang-cl: Take dllexport from original function decl into account
rnk added a subscriber: rnk. rnk added a comment. Hans knows all about dllexport now. http://reviews.llvm.org/D16632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D16632: clang-cl: Take dllexport from original function decl into account
sberg created this revision. sberg added a reviewer: rnk. sberg added a subscriber: cfe-commits. ...in cases where a member function is redeclared as a friend of a nested class. (LibreOffice happens to get tripped up by this.) http://reviews.llvm.org/D16632 Files: lib/CodeGen/CodeGenModule.cpp test/CodeGenCXX/dllexport.cpp Index: test/CodeGenCXX/dllexport.cpp === --- test/CodeGenCXX/dllexport.cpp +++ test/CodeGenCXX/dllexport.cpp @@ -264,6 +264,16 @@ __declspec(dllexport) void friend1() {} void friend2() {} +// MSC-DAG: define dllexport void @"\01?func@Befriended@@SAXXZ"() +// GNU-DAG: define dllexport void @_ZN10Befriended4funcEv() +struct __declspec(dllexport) Befriended { + static void func(); + struct Befriending { +friend void Befriended::func(); + }; +}; +void Befriended::func() {} + // Implicit declarations can be redeclared with dllexport. // MSC-DAG: define dllexport noalias i8* @"\01??2@{{YAPAXI|YAPEAX_K}}@Z"( // GNU-DAG: define dllexport noalias i8* @_Znw{{[yj]}}( Index: lib/CodeGen/CodeGenModule.cpp === --- lib/CodeGen/CodeGenModule.cpp +++ lib/CodeGen/CodeGenModule.cpp @@ -765,7 +765,8 @@ if (FD->hasAttr()) F->setDLLStorageClass(llvm::GlobalVariable::DLLImportStorageClass); - else if (FD->hasAttr()) + else if (FD->hasAttr() || + FD->getCanonicalDecl()->hasAttr()) F->setDLLStorageClass(llvm::GlobalVariable::DLLExportStorageClass); else F->setDLLStorageClass(llvm::GlobalVariable::DefaultStorageClass); Index: test/CodeGenCXX/dllexport.cpp === --- test/CodeGenCXX/dllexport.cpp +++ test/CodeGenCXX/dllexport.cpp @@ -264,6 +264,16 @@ __declspec(dllexport) void friend1() {} void friend2() {} +// MSC-DAG: define dllexport void @"\01?func@Befriended@@SAXXZ"() +// GNU-DAG: define dllexport void @_ZN10Befriended4funcEv() +struct __declspec(dllexport) Befriended { + static void func(); + struct Befriending { +friend void Befriended::func(); + }; +}; +void Befriended::func() {} + // Implicit declarations can be redeclared with dllexport. // MSC-DAG: define dllexport noalias i8* @"\01??2@{{YAPAXI|YAPEAX_K}}@Z"( // GNU-DAG: define dllexport noalias i8* @_Znw{{[yj]}}( Index: lib/CodeGen/CodeGenModule.cpp === --- lib/CodeGen/CodeGenModule.cpp +++ lib/CodeGen/CodeGenModule.cpp @@ -765,7 +765,8 @@ if (FD->hasAttr()) F->setDLLStorageClass(llvm::GlobalVariable::DLLImportStorageClass); - else if (FD->hasAttr()) + else if (FD->hasAttr() || + FD->getCanonicalDecl()->hasAttr()) F->setDLLStorageClass(llvm::GlobalVariable::DLLExportStorageClass); else F->setDLLStorageClass(llvm::GlobalVariable::DefaultStorageClass); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits