Thread: Latest ecpg patch broke MSVC build
This morning's ecpg patch certainly seems to have been snake-bit.
Although the Windows gcc buildfarm members seem happy, the MSVC ones
are all failing with
Linking... Creating library Release\libecpg\libecpg.lib and object Release\libecpg\libecpg.exp
libecpg.exp: error LNK2001: unresolved external symbol DllMain@12 .\Release\libecpg\libecpg.dll : fatal error
LNK1120:1 unresolved externals
I see that DllMain() got added to misc.c, so it's not obvious what's
wrong here. Some adjustment needed in the MSVC build scripts maybe?
regards, tom lane
Tom Lane wrote: > This morning's ecpg patch certainly seems to have been snake-bit. > Although the Windows gcc buildfarm members seem happy, the MSVC ones > are all failing with > > Linking... > Creating library Release\libecpg\libecpg.lib and object Release\libecpg\libecpg.exp > libecpg.exp : error LNK2001: unresolved external symbol DllMain@12 > .\Release\libecpg\libecpg.dll : fatal error LNK1120: 1 unresolved externals > > I see that DllMain() got added to misc.c, so it's not obvious what's > wrong here. Some adjustment needed in the MSVC build scripts maybe? > > > It is building with thread.c but it should not be unless I am misreading the Makefile. The makefile processing in Project.pm doesn't look nearly powerful enough to handle this: # thread.c is needed only for non-WIN32 implementation of path.c ifneq ($(PORTNAME), win32) OBJS += thread.o endif It will ignore the if and endif lines and process the OBJS line :-( A quick fix is probably to put some whitespace in front of "OBJS", although that seems horribly fragile. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> It is building with thread.c but it should not be unless I am misreading
> the Makefile. The makefile processing in Project.pm doesn't look nearly
> powerful enough to handle this:
> # thread.c is needed only for non-WIN32 implementation of path.c
> ifneq ($(PORTNAME), win32)
> OBJS += thread.o
> endif
Hmm, sounds like a problem, but why was it not a problem before?
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> It is building with thread.c but it should not be unless I am misreading >> the Makefile. The makefile processing in Project.pm doesn't look nearly >> powerful enough to handle this: >> > > >> # thread.c is needed only for non-WIN32 implementation of path.c >> ifneq ($(PORTNAME), win32) >> OBJS += thread.o >> endif >> > > Hmm, sounds like a problem, but why was it not a problem before? > > > Good point. I don't know. I guess something else must be causing the build failure. cheers andrew
On Sun, Sep 30, 2007 at 11:30:35PM -0400, Andrew Dunstan wrote: > > > Tom Lane wrote: > >This morning's ecpg patch certainly seems to have been snake-bit. > >Although the Windows gcc buildfarm members seem happy, the MSVC ones > >are all failing with > > > >Linking... > > Creating library Release\libecpg\libecpg.lib and object > > Release\libecpg\libecpg.exp > > libecpg.exp : error LNK2001: unresolved external symbol DllMain@12 > > .\Release\libecpg\libecpg.dll : fatal error LNK1120: 1 unresolved > > externals > > > >I see that DllMain() got added to misc.c, so it's not obvious what's > >wrong here. Some adjustment needed in the MSVC build scripts maybe? > > > > > > > > It is building with thread.c but it should not be unless I am misreading It's been building with thread.c before this patch. And the problem doesn't go away if you ermove thread.c. The problem seems to be that it tries to export a decorated DllMain (DllMain@12) which is listed in the object file, but it's unable to export it. Will need to dig further. The reason it doesn't happen on mingw is likely the horrible kludge that is export-all-symbols-in-all-files that we've only partially been able to emulate. Since this is an actual API library, perhaps a proper fix is to create a .def file listing the exports in it, the same way we do for libpq? And then we could (should!) also filter the exports the same ways as we do for libpq these days. (see the exports.txt file in libpq) I'll try to find time to look forther at this meanwhile, but if someone can confirm that donig an explicit export list is a good way to go, I can confirm that donig that fixes the build problem :-) //Magnus
On Sun, Sep 30, 2007 at 11:46:00PM -0400, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > It is building with thread.c but it should not be unless I am misreading > > the Makefile. The makefile processing in Project.pm doesn't look nearly > > powerful enough to handle this: > > > # thread.c is needed only for non-WIN32 implementation of path.c > > ifneq ($(PORTNAME), win32) > > OBJS += thread.o > > endif > > Hmm, sounds like a problem, but why was it not a problem before? It's not realliy a problem since the stuff in thread.c is #ifdefed away on Windows in most cases anyway. All we do is import a small piece of code we'll never use.. //Magnus
Magnus Hagander schrieb: > On Sun, Sep 30, 2007 at 11:30:35PM -0400, Andrew Dunstan wrote: > >> Tom Lane wrote: >> >>> This morning's ecpg patch certainly seems to have been snake-bit. >>> Although the Windows gcc buildfarm members seem happy, the MSVC ones >>> are all failing with >>> >>> Linking... >>> Creating library Release\libecpg\libecpg.lib and object >>> Release\libecpg\libecpg.exp >>> libecpg.exp : error LNK2001: unresolved external symbol DllMain@12 >>> .\Release\libecpg\libecpg.dll : fatal error LNK1120: 1 unresolved >>> externals >>> >>> I see that DllMain() got added to misc.c, so it's not obvious what's >>> wrong here. Some adjustment needed in the MSVC build scripts maybe? >>> >>> >>> >>> >> It is building with thread.c but it should not be unless I am misreading >> > > It's been building with thread.c before this patch. And the problem doesn't > go away if you ermove thread.c. > > > The problem seems to be that it tries to export a decorated DllMain > (DllMain@12) which is listed in the object file, but it's unable to export > it. Will need to dig further. > > The reason it doesn't happen on mingw is likely the horrible kludge that is > export-all-symbols-in-all-files that we've only partially been able to > emulate. > > Since this is an actual API library, perhaps a proper fix is to create a > .def file listing the exports in it, the same way we do for libpq? And then > we could (should!) also filter the exports the same ways as we do for libpq > these days. > > (see the exports.txt file in libpq) > > I'll try to find time to look forther at this meanwhile, but if someone can > confirm that donig an explicit export list is a good way to go, I can > confirm that donig that fixes the build problem :-) > > //Magnus > > ---------------------------(end of broadcast)--------------------------- > TIP 6: explain analyze is your friend > According to: Module-Definition (.def) File EXPORT http://msdn2.microsoft.com/en-us/library/ms856515.aspx whitespace is required between the name and the ordinal in a .def-file, hence in the .def-file DllMain @12 should be used instead of DllMain@12. Therefor I think the fix should be addressed in tools/msvc/gendef.pl, see attached diff. Anyway there is just a single occurence of an ordinal in the .def-files: $ grep '@' `find -name "*.def"` ./libecpg/LIBECPG.def: DllMain @12 The ordinal 12 seems to be the default for DllMain. -Hannes *** ../pgsql-cvshead/src/tools/msvc/gendef.pl Thu May 3 16:04:03 2007 --- src/tools/msvc/gendef.pl Wed Oct 3 00:53:23 2007 *************** *** 38,43 **** --- 38,46 ---- next if $pieces[6] =~ /^__NULL_IMPORT/; next if $pieces[6] =~ /^\?\?_C/; + # whitespace required between name and ordinal + $pieces[6] =~ s/@/ @/; + push @def, $pieces[6]; } close(F);
> > Since this is an actual API library, perhaps a proper fix is to create a > > .def file listing the exports in it, the same way we do for libpq? And then > > we could (should!) also filter the exports the same ways as we do for libpq > > these days. > > > > (see the exports.txt file in libpq) > > > > I'll try to find time to look forther at this meanwhile, but if someone can > > confirm that donig an explicit export list is a good way to go, I can > > confirm that donig that fixes the build problem :-) > > > > //Magnus > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 6: explain analyze is your friend > > > > According to: > > Module-Definition (.def) File EXPORT > http://msdn2.microsoft.com/en-us/library/ms856515.aspx > > whitespace is required between the name and the ordinal in a > .def-file, hence in the .def-file DllMain @12 should be used > instead of DllMain@12. you're reading the problem wrong. The 12 is not the ordinal, it's a part of the decorated name. /Magnus
Magnus Hagander schrieb: >>> Since this is an actual API library, perhaps a proper fix is to create a >>> .def file listing the exports in it, the same way we do for libpq? And then >>> we could (should!) also filter the exports the same ways as we do for libpq >>> these days. >>> >>> (see the exports.txt file in libpq) >>> >>> I'll try to find time to look forther at this meanwhile, but if someone can >>> confirm that donig an explicit export list is a good way to go, I can >>> confirm that donig that fixes the build problem :-) >>> >>> //Magnus >>> >>> ---------------------------(end of broadcast)--------------------------- >>> TIP 6: explain analyze is your friend >>> >>> >> According to: >> >> Module-Definition (.def) File EXPORT >> http://msdn2.microsoft.com/en-us/library/ms856515.aspx >> >> whitespace is required between the name and the ordinal in a >> .def-file, hence in the .def-file DllMain @12 should be used >> instead of DllMain@12. >> > > you're reading the problem wrong. The 12 is not the ordinal, it's a part of the decorated name. > > > /Magnus > > > ---------------------------(end of broadcast)--------------------------- > TIP 2: Don't 'kill -9' the postmaster > but, we are compiling C so the names shouldn't be decorated. undecorating yields the same name: c:\hannes\>undname DllMain@12 Microsoft (R) C++ Name Undecorator Copyright (C) Microsoft Corporation. All rights reserved. Undecoration of :- "DllMain@12" is :- "DllMain@12" compiling a little test program: cat >dllmain.c <<EOF #include <windows.h> BOOL WINAPI DllMain(HANDLE module, DWORD reason, LPVOID reserved) { return TRUE; } EOF yields the same exported symbol C:\Hannes>cl /c dllmain.c Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 14.00.50727.762 for 80x86 Copyright (C) Microsoft Corporation. All rights reserved. C:\Hannes>dumpbin /symbols dllmain.obj Microsoft (R) COFF/PE Dumper Version 8.00.50727.762 Copyright (C) Microsoft Corporation. All rights reserved. Dump of file dllmain.obj File Type: COFF OBJECT COFF SYMBOL TABLE [snip] 008 00000000 SECT3 notype () External | _DllMain@12 [snip] When creating a new dll with VC 6.0, same here C:\Hannes\testdll\Debug>dumpbin /symbols testdll.obj | grep Main 01B 00000000 SECT6 notype () External | _DllMain@12 as with libecpg C:\Hannes\pgsql\Debug\libecpg>dumpbin /symbols misc.obj | grep Main 05E 000007B0 SECT5 notype () External | _DllMain@12 Am I missing something? -Hannes
On Wed, Oct 03, 2007 at 09:35:51AM +0200, Hannes Eder wrote: > Magnus Hagander schrieb: > >>>Since this is an actual API library, perhaps a proper fix is to create a > >>>.def file listing the exports in it, the same way we do for libpq? And > >>>then > >>>we could (should!) also filter the exports the same ways as we do for > >>>libpq > >>>these days. > >>> > >>>(see the exports.txt file in libpq) > >>> > >>>I'll try to find time to look forther at this meanwhile, but if someone > >>>can > >>>confirm that donig an explicit export list is a good way to go, I can > >>>confirm that donig that fixes the build problem :-) > >>> > >>>//Magnus > >>> > >>>---------------------------(end of broadcast)--------------------------- > >>>TIP 6: explain analyze is your friend > >>> > >>> > >>According to: > >> > >>Module-Definition (.def) File EXPORT > >>http://msdn2.microsoft.com/en-us/library/ms856515.aspx > >> > >>whitespace is required between the name and the ordinal in a > >>.def-file, hence in the .def-file DllMain @12 should be used > >>instead of DllMain@12. > >> > > > > you're reading the problem wrong. The 12 is not the ordinal, it's a part > > of the decorated name. > > > > > >/Magnus > > > > > >---------------------------(end of broadcast)--------------------------- > >TIP 2: Don't 'kill -9' the postmaster > > > but, we are compiling C so the names shouldn't be decorated. We're not talking C++ decoration, we're talking Windows API decoration. Take a look at for example: http://www.geocities.com/yongweiwu/stdcall.htm (there is a reference on MSDN as well, btu I can't find it right now) The @12 is "12 bytes in the argument list to the function". The reason is to make sure the caller calls it with the right number of arguments so as to prevent stack issues. //Magnus
Note that unless there's some tools issue, DllMain doesn't need to be exported to function properly. A DLL's initialization routine is marked as the entry point in the PE header, same as main() in classic C. It might be simpler to just get rid of the export.
In hindsight, all these ecpg changes should have been made between beta1 and beta2 when we have time to deal with the fallout, not right before beta1. --------------------------------------------------------------------------- Tom Lane wrote: > This morning's ecpg patch certainly seems to have been snake-bit. > Although the Windows gcc buildfarm members seem happy, the MSVC ones > are all failing with > > Linking... > Creating library Release\libecpg\libecpg.lib and object Release\libecpg\libecpg.exp > libecpg.exp : error LNK2001: unresolved external symbol DllMain@12 > .\Release\libecpg\libecpg.dll : fatal error LNK1120: 1 unresolved externals > > I see that DllMain() got added to misc.c, so it's not obvious what's > wrong here. Some adjustment needed in the MSVC build scripts maybe? > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 3: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faq -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
> In hindsight, all these ecpg changes should have been made between beta1 > and beta2 when we have time to deal with the fallout, not right before > beta1. > Or considered new features and held back for 8.4. Not picking on Michael, but the resemblance to the /contrib discussionis striking. Ecpg is another part of core PostgreSQL that lives by slightly different rules. /Magnus
On Tue, Oct 09, 2007 at 08:15:35AM +0200, Magnus Hagander wrote: > > In hindsight, all these ecpg changes should have been made between beta1 > > and beta2 when we have time to deal with the fallout, not right before > > beta1. This one I totally agree with. > Or considered new features and held back for 8.4. Not picking on Michael, but the resemblance to the /contrib discussionis striking. Ecpg is another part of core PostgreSQL that lives by slightly different rules. But this one I don't. At least not the "new features" part. Had I considered the patch a new feature I wouldn't have committed it. To me it looked like a bug fix and I still see it as such. Yes, we could have documented the bug instead, but still I don't see how we could argue that getting multithreading to work on Windows is a feature when it's already working on all other platforms a and is also compilable, but not working in some/most cases, on Windows. Michael -- Michael Meskes Email: Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org Go SF 49ers! Go Rhein Fire! Use Debian GNU/Linux! Use PostgreSQL!
On Tue, Oct 09, 2007 at 10:00:51AM +0200, Michael Meskes wrote: > On Tue, Oct 09, 2007 at 08:15:35AM +0200, Magnus Hagander wrote: > > > In hindsight, all these ecpg changes should have been made between beta1 > > > and beta2 when we have time to deal with the fallout, not right before > > > beta1. > > This one I totally agree with. > > > Or considered new features and held back for 8.4. Not picking on Michael, but the resemblance to the /contrib discussionis striking. Ecpg is another part of core PostgreSQL that lives by slightly different rules. > > But this one I don't. At least not the "new features" part. Had I > considered the patch a new feature I wouldn't have committed it. To me > it looked like a bug fix and I still see it as such. Yes, we could have > documented the bug instead, but still I don't see how we could argue > that getting multithreading to work on Windows is a feature when it's > already working on all other platforms a and is also compilable, but not > working in some/most cases, on Windows. We'retalking abuot different patches I think ;-) Things like: http://archives.postgresql.org/pgsql-committers/2007-09/msg00465.php http://archives.postgresql.org/pgsql-committers/2007-09/msg00408.php aren't win32 fixes. They're making parts of ecpg thread-safe that weren't before. And they're the ones that *caused* the win32 specific patches to be needed. That said, I'm sure one could argue they were bug-fixes, but I'm fairliy certain they would *not* be accepted as "bug fixes" if it were backend code. //Magnus