Thread: Latest ecpg patch broke MSVC build

Latest ecpg patch broke MSVC build

From
Tom Lane
Date:
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


Re: Latest ecpg patch broke MSVC build

From
Andrew Dunstan
Date:

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





Re: Latest ecpg patch broke MSVC build

From
Tom Lane
Date:
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


Re: Latest ecpg patch broke MSVC build

From
Andrew Dunstan
Date:

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


Re: Latest ecpg patch broke MSVC build

From
Magnus Hagander
Date:
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


Re: Latest ecpg patch broke MSVC build

From
Magnus Hagander
Date:
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


Re: Latest ecpg patch broke MSVC build

From
Hannes Eder
Date:
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);

Re: Latest ecpg patch broke MSVC build

From
"Magnus Hagander"
Date:
> > 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



Re: Latest ecpg patch broke MSVC build

From
Hannes Eder
Date:
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



Re: Latest ecpg patch broke MSVC build

From
Magnus Hagander
Date:
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



Re: Latest ecpg patch broke MSVC build

From
"Trevor Talbot"
Date:
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.


Re: Latest ecpg patch broke MSVC build

From
Bruce Momjian
Date:
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. +


Re: Latest ecpg patch broke MSVC build

From
"Magnus Hagander"
Date:
> 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 


Re: Latest ecpg patch broke MSVC build

From
Michael Meskes
Date:
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!


Re: Latest ecpg patch broke MSVC build

From
Magnus Hagander
Date:
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