The problem is that your patch caused the reproducer to trigger an assert, which it didn't do before, causing our builds to break.
Your patch seems like it does fix the PR, but it can't break currently working builds. $ build.release/bin/clang -cc1 -triple i386-pc-windows-msvc19.11.0 -emit-pch -fms-extensions -fms-compatibility -fms-compatibility-version=19.11 -std=c++14 -fdelayed-template-parsing -x c++-header /tmp/a.cc -o /dev/null clang: /work/llvm.combined/llvm/tools/clang/lib/Serialization/ASTWriter.cpp:4723: clang::ASTFileSignature clang::ASTWriter::WriteASTCore(clang::Sema&, llvm::StringRef, const string&, clang::Module*): Assertion `SemaRef.PendingLocalImplicitInstantiations.empty() && "There are local ones at end of translation unit!"' failed. build.release/bin/clang(_ZN4llvm3sys15PrintStackTraceERNS_11raw_ostreamE+0x1a)[0x55c535576aaa] build.release/bin/clang(_ZN4llvm3sys17RunSignalHandlersEv+0x3e)[0x55c53557492e] build.release/bin/clang(+0x2424a7c)[0x55c535574a7c] /lib/x86_64-linux-gnu/libpthread.so.0(+0x110c0)[0x7f4aeeb9d0c0] /lib/x86_64-linux-gnu/libc.so.6(gsignal+0xcf)[0x7f4aed732fcf] /lib/x86_64-linux-gnu/libc.so.6(abort+0x16a)[0x7f4aed7343fa] /lib/x86_64-linux-gnu/libc.so.6(+0x2be37)[0x7f4aed72be37] /lib/x86_64-linux-gnu/libc.so.6(+0x2bee2)[0x7f4aed72bee2] build.release/bin/clang(_ZN5clang9ASTWriter12WriteASTCoreERNS_4SemaEN4llvm9StringRefERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEPNS_6ModuleE+0x29fa)[0x55c536444cea] build.release/bin/clang(_ZN5clang9ASTWriter8WriteASTERNS_4SemaERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEPNS_6ModuleEN4llvm9StringRefEb+0xad)[0x55c536444dfd] build.release/bin/clang(_ZN5clang12PCHGenerator21HandleTranslationUnitERNS_10ASTContextE+0x80)[0x55c536465fe0] build.release/bin/clang(_ZN5clang17MultiplexConsumer21HandleTranslationUnitERNS_10ASTContextE+0x28)[0x55c535b53488] build.release/bin/clang(_ZN5clang8ParseASTERNS_4SemaEbb+0x350)[0x55c5362c5d30] build.release/bin/clang(_ZN5clang14FrontendAction7ExecuteEv+0x11e)[0x55c535b2958e] build.release/bin/clang(_ZN5clang16CompilerInstance13ExecuteActionERNS_14FrontendActionE+0x176)[0x55c535af54d6] build.release/bin/clang(_ZN5clang25ExecuteCompilerInvocationEPNS_16CompilerInstanceE+0x981)[0x55c535bc2561] build.release/bin/clang(_Z8cc1_mainN4llvm8ArrayRefIPKcEES2_Pv+0xc50)[0x55c533e1a890] build.release/bin/clang(main+0x1295)[0x55c533d9b585] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf1)[0x7f4aed7202b1] build.release/bin/clang(_start+0x2a)[0x55c533e1684a] Stack dump: 0. Program arguments: build.release/bin/clang -cc1 -triple i386-pc-windows-msvc19.11.0 -emit-pch -fms-extensions -fms-compatibility -fms-compatibility-version=19.11 -std=c++14 -fdelayed-template-parsing -x c++-header /tmp/a.cc -o /dev/null 1. <eof> parser at end of file Aborted On Mon, Feb 19, 2018 at 10:18 PM, Ammarguellat, Zahira <zahira.ammarguel...@intel.com> wrote: > Hans, > > > > The reproducer below generates wrong export symbols compared to MSVC but I > am not sure it is related to my change. > > Compiling this with MSVC generates these symbols: > > > > ksh-3.2$ dumpbin /directives t3.obj | grep EXPORT > > /EXPORT:??4?$d@H@@QEAAAEAV0@AEBV0@@Z > > /EXPORT:??4?$d@H@@QEAAAEAV0@$$QEAV0@@Z > > /EXPORT:??4?$h@H@f@@QEAAAEAV01@AEBV01@@Z > > /EXPORT:??4?$h@H@f@@QEAAAEAV01@$$QEAV01@@Z > > /EXPORT:??4i@@QEAAAEAV0@AEBV0@@Z > > /EXPORT:??4i@@QEAAAEAV0@$$QEAV0@@Z > > > > > > Compiling with clang (my patches no included) generate these symbols: > > ksh-3.2$ dumpbin /directives t3.o | grep EXPORT > > /EXPORT:??4?$d@H@@QEAAAEAV0@AEBV0@@Z > > /EXPORT:??4?$d@H@@QEAAAEAV0@$$QEAV0@@Z > > /EXPORT:??4?$h@H@f@@QEAAAEAV01@AEBV01@@Z > > /EXPORT:??4?$h@H@f@@QEAAAEAV01@$$QEAV01@@Z > > /EXPORT:??4i@@QEAAAEAV0@AEBV0@@Z > > /EXPORT:??4i@@QEAAAEAV0@$$QEAV0@@Z > > /EXPORT:?e@?$d@H@@0W4b@a@@B,DATA รง This is not generated in MSVC. > > > > Although this is a bug that needs to be fixes, it is in my opinion > un-related to the patches I have proposed. > > > > Your thoughts? > > > > Thanks. > > -Zahira > > > > > > -----Original Message----- > From: hwennb...@google.com [mailto:hwennb...@google.com] On Behalf Of Hans > Wennborg > Sent: Monday, February 19, 2018 5:11 AM > To: cfe-commits <cfe-commits@lists.llvm.org>; Ammarguellat, Zahira > <zahira.ammarguel...@intel.com> > Subject: Re: r324991 - Fix for PR32992. Static const classes not exported. > > > > Reduced repro: > > > > $ clang -cc1 -triple i386-pc-windows-msvc19.11.0 -emit-pch -fms-extensions > -fms-compatibility -fms-compatibility-version=19.11 > > -std=c++14 -fdelayed-template-parsing -x c++-header a.ii -o /dev/null > > > > a.ii: > > > > namespace a { > > enum b { c }; > > } > > template <typename> class d { static constexpr a::b e = a::c; }; namespace f > { template <typename g = int> class h : d<g> {}; } using f::h; class > __declspec(dllexport) i : h<> {}; > > > > On Wed, Feb 14, 2018 at 4:22 PM, Hans Wennborg <h...@chromium.org> wrote: > >> I ended up having to revert this in r325133 as it broke the Chromium > >> build. Please see > >> https://bugs.chromium.org/p/chromium/issues/detail?id=812231#c1 for a > >> reproducer. > >> > >> On Tue, Feb 13, 2018 at 10:19 AM, Hans Wennborg via cfe-commits > >> <cfe-commits@lists.llvm.org> wrote: > >>> Author: hans > >>> Date: Tue Feb 13 01:19:43 2018 > >>> New Revision: 324991 > >>> > >>> URL: http://llvm.org/viewvc/llvm-project?rev=324991&view=rev > >>> Log: > >>> Fix for PR32992. Static const classes not exported. > >>> > >>> Patch by zahiraam! > >>> > >>> Differential Revision: https://reviews.llvm.org/D42968 > >>> > >>> Modified: > >>> cfe/trunk/lib/Sema/SemaDeclCXX.cpp > >>> cfe/trunk/test/CodeGenCXX/dllexport.cpp > >>> > >>> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp > >>> URL: > >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cp > >>> p?rev=324991&r1=324990&r2=324991&view=diff > >>> ===================================================================== > >>> ========= > >>> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original) > >>> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Tue Feb 13 01:19:43 2018 > >>> @@ -5476,7 +5476,7 @@ static void CheckAbstractClassUsage(Abst > >>> } > >>> } > >>> > >>> -static void ReferenceDllExportedMethods(Sema &S, CXXRecordDecl > >>> *Class) { > >>> +static void ReferenceDllExportedMembers(Sema &S, CXXRecordDecl > >>> +*Class) { > >>> Attr *ClassAttr = getDLLAttr(Class); > >>> if (!ClassAttr) > >>> return; > >>> @@ -5491,6 +5491,16 @@ static void ReferenceDllExportedMethods( > >>> return; > >>> > >>> for (Decl *Member : Class->decls()) { > >>> + // Defined static variables that are members of an exported base > >>> + // class must be marked export too. Push them to implicit >>> instantiation > >>> + // queue. > >>> + auto *VD = dyn_cast<VarDecl>(Member); > >>> + if (VD && Member->getAttr<DLLExportAttr>() && > >>> + VD->getStorageClass() == SC_Static && > >>> + TSK == TSK_ImplicitInstantiation) > >>> + S.PendingLocalImplicitInstantiations.push_back( > >>> + std::make_pair(VD, VD->getLocation())); > >>> + > >>> auto *MD = dyn_cast<CXXMethodDecl>(Member); > >>> if (!MD) > >>> continue; > >>> @@ -10902,12 +10912,12 @@ void Sema::ActOnFinishCXXNonNestedClass( > >>> > >>> void Sema::referenceDLLExportedClassMethods() { > >>> if (!DelayedDllExportClasses.empty()) { > >>> - // Calling ReferenceDllExportedMethods might cause the current >>> function to > >>> + // Calling ReferenceDllExportedMembers might cause the current > >>> + function to > >>> // be called again, so use a local copy of DelayedDllExportClasses. > >>> SmallVector<CXXRecordDecl *, 4> WorkList; > >>> std::swap(DelayedDllExportClasses, WorkList); > >>> for (CXXRecordDecl *Class : WorkList) > >>> - ReferenceDllExportedMethods(*this, Class); > >>> + ReferenceDllExportedMembers(*this, Class); > >>> } > >>> } > >>> > >>> > >>> Modified: cfe/trunk/test/CodeGenCXX/dllexport.cpp > >>> URL: > >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/dllexpo > >>> rt.cpp?rev=324991&r1=324990&r2=324991&view=diff > >>> ===================================================================== > >>> ========= > >>> --- cfe/trunk/test/CodeGenCXX/dllexport.cpp (original) > >>> +++ cfe/trunk/test/CodeGenCXX/dllexport.cpp Tue Feb 13 01:19:43 2018 > >>> @@ -28,6 +28,7 @@ struct External { int v; }; > >>> > >>> // The vftable for struct W is comdat largest because we have RTTI. > >>> // M32-DAG: $"\01??_7W@@6B@" = comdat largest > >>> +// M32-DAG: $"\01?smember@?$Base@H@PR32992@@0HA" = comdat any > >>> > >>> > >>> > >>> //===---------------------------------------------------------------- > >>> ------===// @@ -977,3 +978,21 @@ class __declspec(dllexport) > >>> ACE_Service_ // MSVC2015-DAG: define weak_odr dllexport > >>> {{.+}}ACE_Service_Object@@Q{{.+}}@$$Q > >>> // The declarations should not be exported. > >>> // MSVC2013-NOT: define weak_odr dllexport > >>> {{.+}}ACE_Service_Object@@Q{{.+}}@$$Q > >>> + > >>> +namespace PR32992 { > >>> +// Static data members of a instantiated base class should be exported. > >>> +template <class T> > >>> +class Base { > >>> + virtual void myfunc() {} > >>> + static int smember; > >>> +}; > >>> +// MS-DAG: @"\01?smember@?$Base@H@PR32992@@0HA" = weak_odr > >>> +dllexport global i32 77, comdat, align 4 template <class T> int > >>> +Base<T>::smember = 77; template <class T> class > >>> +__declspec(dllexport) Derived2 : Base<T> { > >>> + void myfunc() {} > >>> +}; > >>> +class Derived : public Derived2<int> { > >>> + void myfunc() {} > >>> +}; > >>> +} // namespace PR32992 > >>> > >>> > >>> _______________________________________________ > >>> cfe-commits mailing list > >>> cfe-commits@lists.llvm.org > >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
