Latest ecpg patch broke MSVC build

Started by Tom Laneover 18 years ago15 messages

tgl@sss.pgh.pa.us

over 18 years ago

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

Andrew Dunstan

andrew@dunslane.net

over 18 years ago

In reply to: Tom Lane (#1)

Re: Latest ecpg patch broke MSVC build

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

Tom Lane

tgl@sss.pgh.pa.us

over 18 years ago

In reply to: Andrew Dunstan (#2)

Re: Latest ecpg patch broke MSVC build

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

Andrew Dunstan

andrew@dunslane.net

over 18 years ago

In reply to: Tom Lane (#3)

Re: Latest ecpg patch broke MSVC build

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

Magnus Hagander

magnus@hagander.net

over 18 years ago

In reply to: Andrew Dunstan (#2)

Re: Latest ecpg patch broke MSVC build

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

Magnus Hagander

magnus@hagander.net

over 18 years ago

In reply to: Tom Lane (#3)

Re: Latest ecpg patch broke MSVC build

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

Hannes Eder

Hannes@HannesEder.net

over 18 years ago

In reply to: Magnus Hagander (#5)

1 attachment(s)

Re: Latest ecpg patch broke MSVC build

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

Attachments:

gendef.pl-200710031.difftext/x-diff; name=gendef.pl-200710031.diffDownload

*** ../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);

Magnus Hagander

magnus@hagander.net

over 18 years ago

In reply to: Hannes Eder (#7)

Re: Latest ecpg patch broke MSVC build

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

Hannes Eder

Hannes@HannesEder.net

over 18 years ago

In reply to: Magnus Hagander (#8)

Re: Latest ecpg patch broke MSVC build

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

#10

Magnus Hagander

magnus@hagander.net

over 18 years ago

In reply to: Hannes Eder (#9)

Re: Latest ecpg patch broke MSVC build

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

#11

Trevor Talbot

quension@gmail.com

over 18 years ago

In reply to: Magnus Hagander (#10)

Re: Latest ecpg patch broke MSVC build

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.

#12

Bruce Momjian

bruce@momjian.us

over 18 years ago

In reply to: Tom Lane (#1)

Re: Latest ecpg patch broke MSVC build

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. +

#13

Magnus Hagander

magnus@hagander.net

over 18 years ago

In reply to: Bruce Momjian (#12)

Re: Latest ecpg patch broke MSVC build

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 discussion is striking. Ecpg is another part of core PostgreSQL that lives by slightly different rules.

/Magnus

Import Notes

Resolved by subject fallback

#14

Michael Meskes

meskes@postgresql.org

over 18 years ago

In reply to: Magnus Hagander (#13)

Re: Latest ecpg patch broke MSVC build

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 discussion is 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!

#15

Magnus Hagander

magnus@hagander.net

over 18 years ago

In reply to: Michael Meskes (#14)

Re: Latest ecpg patch broke MSVC build

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 discussion is 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