Re: narwhal and PGDLLIMPORT

On 02/12/2014 07:22 AM, Tom Lane wrote:
> So the early returns from currawong are interesting:
>
> "d:\bf\root\HEAD\pgsql.920\pgsql.sln" (default target) (1) ->
> (contrib\pg_buffercache target) ->
> pg_buffercache_pages.obj : error LNK2001: unresolved external symbol _MainLWLockArray
> .\Release\pg_buffercache\pg_buffercache.dll : fatal error LNK1120: 1 unresolved externals
>
>
> "d:\bf\root\HEAD\pgsql.920\pgsql.sln" (default target) (1) ->
> (contrib\pg_stat_statements target) ->
> pg_stat_statements.obj : error LNK2001: unresolved external symbol _MainLWLockArray
> .\Release\pg_stat_statements\pg_stat_statements.dll : fatal error LNK1120: 1 unresolved externals
>
>
> "d:\bf\root\HEAD\pgsql.920\pgsql.sln" (default target) (1) ->
> (contrib\test_shm_mq target) ->
> worker.obj : error LNK2001: unresolved external symbol _ProcDiePending
> worker.obj : error LNK2001: unresolved external symbol _proc_exit_inprogress
> .\Release\test_shm_mq\test_shm_mq.dll : fatal error LNK1120: 2 unresolved externals

Great, that's what I was hoping to see - proper errors where we've
omitted things, not silent miscompilation.

> I guess this is good news in one sense: now we're getting results from an
> MSVC machine that are consistent with what narwhal has been telling us.
> But how is it that your patch caused this to be reported when it wasn't
> before? And how come we're not getting the results we hoped for, that
> these symbols would be effectively exported without needing PGDLLIMPORT?

Unfortunately I don't think we can get to "totally invisible" with the
MS toolchain. We're still going to have to annotate exported symbols,
but at least we can tell where code tries to import something that
hasn't been annotated.

This just fixes the bug in the .def generator that was permitting wrong
code to compile silently and incorrectly, rather than fail to link. It
makes omitted PGDLLIMPORT's obvious, but it doesn't appear to be
possible to tell the toolchain that the *importing side* should
auto-detect whether an extern is from another DLL and automatically
__declspec("dllimport") it.

As I understand it, what's happening here is that the importing side
(the contrib/tool) is trying to do old-style DLL linkage, where the
export library for the DLL is expected to expose a symbol referring to
*a pointer to the real data* that gets statically linked into the
importing side. We're trying to link to those pointer symbols and are
failing because the `DATA` annotation suppresses their generation.

(Remember that in Microsoft-land you don't actually link to a .DLL at
compilation time, you statically link to the export library .LIB for the
DLL, which contains stubs and fixup information).

If we instead emitted "CONST" in the .DEF file, these indirect pointers
would be generated by the linker and added to the export library, and
we'd link to those instead. Our code doesn't expect the extra level of
indirection introduced and would fail (mostly) at runtime, much like
what was happening when we were linking to function stubs.

The only way I can see to actually get unix-like linkage is to use
__declspec("dllimport") on the *importing side*, e.g. when compiling
contribs etc. That means we still need the much-hated windows-droppings,
but we can at least see when they got left out.

(On a side note, I'm starting to suspect - and need to verify - that as
a result of not using PGDLLIMPORT on exported functions we're actually
doing old-style thunk function calls from extensions into postgres.exe,
not direct function calls, for a similar reason.)

I'm going to go wash my brain out now, it feels dirty.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services