Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

Albe Laurenz <laurenz.albe@wien.gv.at> writes:

Currently, PG_FUNCTION_INFO_V1 is defined as
/*
* Macro to build an info function associated with the given function name.
* Win32 loadable functions usually link with 'dlltool --export-all', but it
* doesn't hurt to add PGDLLIMPORT in case they don't.
*/
#define PG_FUNCTION_INFO_V1(funcname) \
Datum funcname(PG_FUNCTION_ARGS); \
extern PGDLLEXPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \
const Pg_finfo_record * \
CppConcat(pg_finfo_,funcname) (void) \
{ \
static const Pg_finfo_record my_finfo = { 1 }; \
return &my_finfo; \
} \
extern int no_such_variable

Is there a good reason why the "funcname" declaration is not decorated
with PGDLLEXPORT?

The lack of complaints about this suggest that it's not actually necessary
to do so. So what this makes me wonder is whether we can't drop the
DLLEXPORT on the finfo function too. I'd rather reduce the number of
Microsoft-isms in the code, not increase it.

BTW, I admit I don't understand the comment.

It seems like an obvious typo. Or, possibly, sloppy thinking that
contributed to failure to recognize that the keyword isn't needed.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:

Albe Laurenz <laurenz.albe@wien.gv.at> writes:

Currently, PG_FUNCTION_INFO_V1 is defined as

[...]

Is there a good reason why the "funcname" declaration is not decorated
with PGDLLEXPORT?

The lack of complaints about this suggest that it's not actually necessary
to do so. So what this makes me wonder is whether we can't drop the
DLLEXPORT on the finfo function too. I'd rather reduce the number of
Microsoft-isms in the code, not increase it.

I understand the sentiment.

But it seems to actually cause a problem on Windows, at least there was a
complaint here: http://stackoverflow.com/q/39964233

Adding PGDLLEXPORT solved the problem there.

I guess that there are not more complaints about that because
few people build C functions on Windows themselves (lack of PGXS)
and those who do are probably knowledgeable enough that they can
fix it themselves by sticking an extra declaration with PGDLLEXPORT
into their source file.

PostgreSQL itself seems to use export files that explicitly declare the
exported symbols, so it gets away without these decorations.

BTW, I admit I don't understand the comment.

It seems like an obvious typo. Or, possibly, sloppy thinking that
contributed to failure to recognize that the keyword isn't needed.

Looking through the history, it seems to be as follows:
- In 5fde8613, the comment was added (mistakenly) together with a DLLIMPORT decoration.
- In 77e50a61, the decoration was changed to PGDLLEXPORT, but the comment forgotten.
- In e7128e8d, the function prototype was added to the macro, but
the PGDLLEXPORT decoration was forgotten.

The dlltool mentioned is MinGW, so it doesn't apply to people building
with MSVC.

Maybe the comment should be like this:

/*
* Macro to build an info function associated with the given function name.
* Win32 loadable functions usually link with 'dlltool --export-all' or use
* a .DEF file, but it doesn't hurt to add PGDLLEXPORT in case they don't.
*/

Yours,
Laurenz Albe

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Albe Laurenz <laurenz.albe@wien.gv.at> writes:

Tom Lane wrote:

The lack of complaints about this suggest that it's not actually necessary
to do so. So what this makes me wonder is whether we can't drop the
DLLEXPORT on the finfo function too. I'd rather reduce the number of
Microsoft-isms in the code, not increase it.

I understand the sentiment.

But it seems to actually cause a problem on Windows, at least there was a
complaint here: http://stackoverflow.com/q/39964233

Adding PGDLLEXPORT solved the problem there.

I guess that there are not more complaints about that because
few people build C functions on Windows themselves (lack of PGXS)
and those who do are probably knowledgeable enough that they can
fix it themselves by sticking an extra declaration with PGDLLEXPORT
into their source file.

PostgreSQL itself seems to use export files that explicitly declare the
exported symbols, so it gets away without these decorations.

Except that we don't. There aren't PGDLLEXPORT markings for any
functions exported from contrib modules, and we don't use dlltool
on them either. By your argument, none of contrib would work on
Windows builds at all, but we have a ton of buildfarm evidence and
successful field use to the contrary. How is that all working?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:

PostgreSQL itself seems to use export files that explicitly declare the
exported symbols, so it gets away without these decorations.

Except that we don't. There aren't PGDLLEXPORT markings for any
functions exported from contrib modules, and we don't use dlltool
on them either. By your argument, none of contrib would work on
Windows builds at all, but we have a ton of buildfarm evidence and
successful field use to the contrary. How is that all working?

I thought it was the job of src/tools/msvc/gendef.pl to generate
.DEF files? In the buildfarm logs I can find lines like:

Generating CITEXT.DEF from directory Release\citext, platform x64
Generating POSTGRES_FDW.DEF from directory Release\postgres_fdw, platform x64

which are emitted by gendef.pl.

Yours,
Laurenz Albe

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Albe Laurenz <laurenz.albe@wien.gv.at> writes:

Tom Lane wrote:

Except that we don't. There aren't PGDLLEXPORT markings for any
functions exported from contrib modules, and we don't use dlltool
on them either. By your argument, none of contrib would work on
Windows builds at all, but we have a ton of buildfarm evidence and
successful field use to the contrary. How is that all working?

I thought it was the job of src/tools/msvc/gendef.pl to generate
.DEF files?

Hm, okay, so after further review and googling:

MSVC: gendef.pl is used to build a .DEF file, creating the equivalent
of --export-all-symbols behavior.

Mingw: we use handmade .DEF files for libpq and a few other libraries,
otherwise Makefile.shlib explicitly specifies -Wl,--export-all-symbols.

Cygwin: per https://sourceware.org/binutils/docs-2.17/ld/WIN32.html
--export-all-symbols is the default unless you use a .DEF file or have at
least one symbol marked __declspec(dllexport) --- but the Cygwin build
defines PGDLLEXPORT as empty, so there won't be any of the latter.

So it works for us, but the stackoverflow complainant is evidently using
some homebrew build process that does none of the above.

I'm okay with adding PGDLLEXPORT to the extern, but we should update
that comment to note that it's not necessary with any of our standard
Windows build processes. (For that matter, the comment fails to explain
why this macro is providing an extern for the base function at all...)

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:

I'm okay with adding PGDLLEXPORT to the extern, but we should update
that comment to note that it's not necessary with any of our standard
Windows build processes. (For that matter, the comment fails to explain
why this macro is providing an extern for the base function at all...)

Here is a patch for that, including an attempt to improve the comment.

Yours,
Laurenz Albe

Albe Laurenz <laurenz.albe@wien.gv.at> writes:

Tom Lane wrote:

I'm okay with adding PGDLLEXPORT to the extern, but we should update
that comment to note that it's not necessary with any of our standard
Windows build processes. (For that matter, the comment fails to explain
why this macro is providing an extern for the base function at all...)

Here is a patch for that, including an attempt to improve the comment.

Pushed with some further twiddling of the comment.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:

Albe Laurenz <laurenz.albe@wien.gv.at> writes:

Tom Lane wrote:

I'm okay with adding PGDLLEXPORT to the extern, but we should update
that comment to note that it's not necessary with any of our standard
Windows build processes. (For that matter, the comment fails to explain
why this macro is providing an extern for the base function at all...)

Here is a patch for that, including an attempt to improve the comment.

Pushed with some further twiddling of the comment.

Well, the buildfarm doesn't like that even a little bit. It seems that
the MSVC compiler does not like seeing both "extern Datum foo(...)" and
"extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in
a .h file is failing. There is also quite a bit of phase-of-the-moon
behavior in here, because in some cases some functions are raising errors
and others that look entirely the same are not.

We could plaster all those declarations with PGDLLEXPORT, but I'm rather
considerably tempted to go back to the way things were, instead. I do
not like patches that end up with Microsoft-droppings everywhere.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:

Well, the buildfarm doesn't like that even a little bit. It seems that
the MSVC compiler does not like seeing both "extern Datum foo(...)" and
"extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in
a .h file is failing. There is also quite a bit of phase-of-the-moon
behavior in here, because in some cases some functions are raising errors
and others that look entirely the same are not.

I take back the latter comment --- I was comparing HEAD source code
against errors reported on back branches, and at least some of the
discrepancies are explained by additions/removals of separate "extern"
declarations. But still, if we want to do this we're going to need to
put PGDLLEXPORT in every V1 function extern declaration. We might be
able to remove some of the externs as unnecessary instead, but any way
you slice it it's going to be messy.

For the moment I've reverted the change.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 13 Oct. 2016 05:28, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:

I wrote:

Albe Laurenz <laurenz.albe@wien.gv.at> writes:

Tom Lane wrote:

I'm okay with adding PGDLLEXPORT to the extern, but we should update
that comment to note that it's not necessary with any of our standard
Windows build processes. (For that matter, the comment fails to

explain

why this macro is providing an extern for the base function at all...)

Here is a patch for that, including an attempt to improve the comment.

Pushed with some further twiddling of the comment.

Well, the buildfarm doesn't like that even a little bit. It seems that
the MSVC compiler does not like seeing both "extern Datum foo(...)" and
"extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in
a .h file is failing. There is also quite a bit of phase-of-the-moon
behavior in here, because in some cases some functions are raising errors
and others that look entirely the same are not.

Pretty sure we discussed and did exactly this before around 9.4. Will check
archives...

Yeah. Here's the thread.

/messages/by-id/27019.1397571477@sss.pgh.pa.us

I think the discussion last time came down to you and I disagreeing about
Microsoft droppings too ;)

Tom Lane wrote:

Well, the buildfarm doesn't like that even a little bit. It seems that
the MSVC compiler does not like seeing both "extern Datum foo(...)" and
"extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in
a .h file is failing. There is also quite a bit of phase-of-the-moon
behavior in here, because in some cases some functions are raising errors
and others that look entirely the same are not.

I take back the latter comment --- I was comparing HEAD source code
against errors reported on back branches, and at least some of the
discrepancies are explained by additions/removals of separate "extern"
declarations. But still, if we want to do this we're going to need to
put PGDLLEXPORT in every V1 function extern declaration. We might be
able to remove some of the externs as unnecessary instead, but any way
you slice it it's going to be messy.

For the moment I've reverted the change.

Sorry for having caused the inconvenience.

Actually I would say that the correct solution is to remove the function
declarations from all the header files in contrib, since from commit e7128e8d
on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right?
Of course one would have to check first that the SQL functions don't try to
call each other, which would require extra forward declarations...

If you are willing to consider that, I'd be happy to prepare a patch.

But I'd understand if you think that this is too much code churn for too little
benefit, even if it could be considered a clean-up.

In that case, I'd argue that in the sample in doc/src/sgml/xfunc.sgml
the function definitions should be changed to read

PGDLLEXPORT Datum foo(PG_FUNCTION_ARGS)

instead of

Datum foo(PG_FUNCTION_ARGS)

because without that the sample fails if you try to build it with MSVC
like the stackoverflow question did.

I'd be happy to prepare a patch for that as well.

Yours,
Laurenz Albe

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:

But I'd understand if you think that this is too much code churn for too little
benefit, even if it could be considered a clean-up.

In that case, I'd argue that in the sample in doc/src/sgml/xfunc.sgml
the function definitions should be changed to read

PGDLLEXPORT Datum foo(PG_FUNCTION_ARGS)

instead of

Datum foo(PG_FUNCTION_ARGS)

because without that the sample fails if you try to build it with MSVC
like the stackoverflow question did.

Since I didn't hear from you, I assume that you are not in favour of
removing the SQL function declarations from contrib.

So I went ahead and wrote a patch to add PGDLLEXPORT to the C function sample.

While at it, I noticed that there are no instructions for building and
linking C functions with MSVC, so I have added a patch for that as well.

Yours,
Laurenz Albe

On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz <laurenz.albe@wien.gv.at> wrote:

Tom Lane wrote:

Well, the buildfarm doesn't like that even a little bit. It seems that
the MSVC compiler does not like seeing both "extern Datum foo(...)" and
"extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in
a .h file is failing. There is also quite a bit of phase-of-the-moon
behavior in here, because in some cases some functions are raising errors
and others that look entirely the same are not.

I take back the latter comment --- I was comparing HEAD source code
against errors reported on back branches, and at least some of the
discrepancies are explained by additions/removals of separate "extern"
declarations. But still, if we want to do this we're going to need to
put PGDLLEXPORT in every V1 function extern declaration. We might be
able to remove some of the externs as unnecessary instead, but any way
you slice it it's going to be messy.

For the moment I've reverted the change.

Sorry for having caused the inconvenience.

Actually I would say that the correct solution is to remove the function
declarations from all the header files in contrib, since from commit e7128e8d
on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right?

Right. Why isn't that already the case? Commit e7128e8d seems to
have tried. Did it just miss a bunch of cases?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz <laurenz.albe@wien.gv.at> wrote:

Actually I would say that the correct solution is to remove the function
declarations from all the header files in contrib, since from commit e7128e8d
on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right?

Right. Why isn't that already the case? Commit e7128e8d seems to
have tried. Did it just miss a bunch of cases?

That only works to the extent that there are no cross-file references to
those symbols within the extension. If that's true for 99% or more of
them then this is probably a good way to go. If it's only true for, say,
75%, I'm not sure we want to decimate the headers that way. We'd end
up with something doubly ugly: both haphazard-looking lists of only
some of the symbols, and PGDLLEXPORT marks on those that remain.

As for the core problem, I wonder why we aren't recommending that
third-party modules be built using the same infrastructure contrib
uses, rather than people ginning up their own infrastructure and
then finding out the hard way that that means they need PGDLLEXPORT
marks.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz <laurenz.albe@wien.gv.at> wrote:

Actually I would say that the correct solution is to remove the function
declarations from all the header files in contrib, since from commit e7128e8d
on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right?

Right. Why isn't that already the case? Commit e7128e8d seems to
have tried. Did it just miss a bunch of cases?

That only works to the extent that there are no cross-file references to
those symbols within the extension. If that's true for 99% or more of
them then this is probably a good way to go. If it's only true for, say,
75%, I'm not sure we want to decimate the headers that way. We'd end
up with something doubly ugly: both haphazard-looking lists of only
some of the symbols, and PGDLLEXPORT marks on those that remain.

I wouldn't think that cross-file references would be especially
common. Functions that take PG_FUNCTION_ARGS and return Datum aren't
a lot of fun to call from C. But maybe I'm wrong.

As for the core problem, I wonder why we aren't recommending that
third-party modules be built using the same infrastructure contrib
uses, rather than people ginning up their own infrastructure and
then finding out the hard way that that means they need PGDLLEXPORT
marks.

So, they'd need to generate export files somehow?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2016-10-17 16:16:37 -0400, Robert Haas wrote:

I wouldn't think that cross-file references would be especially
common. Functions that take PG_FUNCTION_ARGS and return Datum aren't
a lot of fun to call from C. But maybe I'm wrong.

There's a fair number of DirectFunctionCall$Ns over the backend.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

As for the core problem, I wonder why we aren't recommending that
third-party modules be built using the same infrastructure contrib
uses, rather than people ginning up their own infrastructure and
then finding out the hard way that that means they need PGDLLEXPORT
marks.

So, they'd need to generate export files somehow?

My point is that that's a solved problem. Perhaps the issue is that
we haven't made our src/tools/msvc infrastructure available for outside
use in the way that we've exported our Unix build infrastructure through
PGXS. But if so, I should think that working on that is the thing to do.

[ wanders away wondering what cmake does with this... ]

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Oct 17, 2016 at 4:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

As for the core problem, I wonder why we aren't recommending that
third-party modules be built using the same infrastructure contrib
uses, rather than people ginning up their own infrastructure and
then finding out the hard way that that means they need PGDLLEXPORT
marks.

So, they'd need to generate export files somehow?

My point is that that's a solved problem. Perhaps the issue is that
we haven't made our src/tools/msvc infrastructure available for outside
use in the way that we've exported our Unix build infrastructure through
PGXS. But if so, I should think that working on that is the thing to do.

Yeah, I don't know. For my money, decorating the function definitions
in place seems easier than having to maintain a separate export list,
especially if it can be hidden under the carpet using the existing
stupid macro tricks. But I am not a Windows expert.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

17 октября 2016 г. 23:42:30 MSK, Tom Lane wrote:

[ wanders away wondering what cmake does with this... ]

CMake can export all symbols using only one setting -
WINDOWS_EXPORT_ALL_SYMBOLS for shared libraries and special for Postgres I
made "export all" for executable files. You can try this in
cmake-3.7.0-rc1.
Current postgres_cmake is using the modified gendef.pl (use only
stdin/stdout for objtool without any files).

regards,
Yury
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers