Re: Why does load_external_function() return PGFunction?

Hi,

We've several callers to load_external_function() that do not use the
returned value as a PGFunction. I'd vote for changing the return type to
void * and have fmgr.c cast it to PGFunction after verifying the
function's magic.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> We've several callers to load_external_function() that do not use the
> returned value as a PGFunction. I'd vote for changing the return type to
> void * and have fmgr.c cast it to PGFunction after verifying the
> function's magic.

void* isn't necessarily compatible with function pointers --- there are
platforms where they're physically different widths, though possibly
you'd never get PG to run on such hardware anyway.

I'd be OK with declaring it as a more generic function pointer type,
perhaps "void (*funcptr) ()".

However, given that a cast is going to be necessary anyway, it seems
like this is mostly useless churn...

regards, tom lane

On 2018-02-06 15:43:29 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > We've several callers to load_external_function() that do not use the
> > returned value as a PGFunction. I'd vote for changing the return type to
> > void * and have fmgr.c cast it to PGFunction after verifying the
> > function's magic.
>
> void* isn't necessarily compatible with function pointers --- there are
> platforms where they're physically different widths, though possibly
> you'd never get PG to run on such hardware anyway.

Fair point. Although we're relying on dlsym like infrastructure, which
returns just a void *.

> However, given that a cast is going to be necessary anyway, it seems
> like this is mostly useless churn...

Ok, can live with that too. Just though it was a bit weird...

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2018-02-06 15:43:29 -0500, Tom Lane wrote:
>> void* isn't necessarily compatible with function pointers --- there are
>> platforms where they're physically different widths, though possibly
>> you'd never get PG to run on such hardware anyway.

> Fair point. Although we're relying on dlsym like infrastructure, which
> returns just a void *.

Yeah. Presumably, a platform where they were really different would have
to provide some unstandardized variant of dlsym for fetching function
pointers. We could cope with that fairly easily as things stand, since
we have platform-specific wrappers for dlsym anyway. But if we made the
API for the wrappers dependent on data and code pointers being the same,
we'd be in trouble.

regards, tom lane

On 2/7/18 09:35, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
>> On 2018-02-06 15:43:29 -0500, Tom Lane wrote:
>>> void* isn't necessarily compatible with function pointers --- there are
>>> platforms where they're physically different widths, though possibly
>>> you'd never get PG to run on such hardware anyway.
>
>> Fair point. Although we're relying on dlsym like infrastructure, which
>> returns just a void *.
>
> Yeah. Presumably, a platform where they were really different would have
> to provide some unstandardized variant of dlsym for fetching function
> pointers. We could cope with that fairly easily as things stand, since
> we have platform-specific wrappers for dlsym anyway. But if we made the
> API for the wrappers dependent on data and code pointers being the same,
> we'd be in trouble.

Some years ago, this issue was identified as a defect in the POSIX
standard. At the time, it seemed they were moving in the direction of
changing the dlsym() signature to return an opaque function pointer.
However, it seems the latest version published by the Open Group says
effectively, while this is not required to work in ISO C, this standard
requires it to work.

FreeBSD has added a dlfunc() function as a workaround, but it apparently
hasn't spread anywhere else.

So, um, I don't know. Carry on. ;-)

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-02-06 15:43:29 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > We've several callers to load_external_function() that do not use the
> > returned value as a PGFunction. I'd vote for changing the return type to
> > void * and have fmgr.c cast it to PGFunction after verifying the
> > function's magic.
>
> void* isn't necessarily compatible with function pointers --- there are
> platforms where they're physically different widths, though possibly
> you'd never get PG to run on such hardware anyway.
>
> I'd be OK with declaring it as a more generic function pointer type,
> perhaps "void (*funcptr) ()".
>
> However, given that a cast is going to be necessary anyway, it seems
> like this is mostly useless churn...

I don't think it really changes the need, but it's worthwhile to note
that gcc-8 warns about this now:
/home/andres/src/postgresql/src/backend/postmaster/bgworker.c: In function ‘LookupBackgroundWorkerFunction’:
/home/andres/src/postgresql/src/backend/postmaster/bgworker.c:1246:9: warning: cast between incompatible function types from ‘PGFunction’ {aka ‘long unsigned int (*)(struct FunctionCallInfoData *)’} to ‘void (*)(Datum)’ {aka ‘void (*)(long unsigned int)’} [-Wcast-function-type]
return (bgworker_main_type)

Greetings,

Andres Freund

On Sat, Mar 24, 2018 at 4:33 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> I don't think it really changes the need, but it's worthwhile to note
> that gcc-8 warns about this now:
> /home/andres/src/postgresql/src/backend/postmaster/bgworker.c: In function ‘LookupBackgroundWorkerFunction’:
> /home/andres/src/postgresql/src/backend/postmaster/bgworker.c:1246:9: warning: cast between incompatible function types from ‘PGFunction’ {aka ‘long unsigned int (*)(struct FunctionCallInfoData *)’} to ‘void (*)(Datum)’ {aka ‘void (*)(long unsigned int)’} [-Wcast-function-type]
> return (bgworker_main_type)

That's probably going to mean we need, or at least want, to do
something about this at some point. Warning-free compiles are
desirable.

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

On 2018-03-26 11:14:03 -0400, Robert Haas wrote:
> On Sat, Mar 24, 2018 at 4:33 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > I don't think it really changes the need, but it's worthwhile to note
> > that gcc-8 warns about this now:
> > /home/andres/src/postgresql/src/backend/postmaster/bgworker.c: In function ‘LookupBackgroundWorkerFunction’:
> > /home/andres/src/postgresql/src/backend/postmaster/bgworker.c:1246:9: warning: cast between incompatible function types from ‘PGFunction’ {aka ‘long unsigned int (*)(struct FunctionCallInfoData *)’} to ‘void (*)(Datum)’ {aka ‘void (*)(long unsigned int)’} [-Wcast-function-type]
> > return (bgworker_main_type)
>
> That's probably going to mean we need, or at least want, to do
> something about this at some point. Warning-free compiles are
> desirable.

Right. We can suppress those if we want, either by adding another cast,
or by adding -Wno-cast-function-type. The latter wouldn't be entirely a
bad idea, it seems to be a warning of very limited benefit.

Greetings,

Andres Freund

Hi,

On 2018-02-06 15:43:29 -0500, Tom Lane wrote:
> void* isn't necessarily compatible with function pointers --- there are
> platforms where they're physically different widths, though possibly
> you'd never get PG to run on such hardware anyway.

Btw, given that we store function pointers in datums, that ship has
sailed a long while ago, no?

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2018-02-06 15:43:29 -0500, Tom Lane wrote:
>> void* isn't necessarily compatible with function pointers --- there are
>> platforms where they're physically different widths, though possibly
>> you'd never get PG to run on such hardware anyway.

> Btw, given that we store function pointers in datums, that ship has
> sailed a long while ago, no?

We do? Data pointers, sure, but I can't think of any use of the other.

regards, tom lane

On 2018-03-26 15:26:00 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2018-02-06 15:43:29 -0500, Tom Lane wrote:
> >> void* isn't necessarily compatible with function pointers --- there are
> >> platforms where they're physically different widths, though possibly
> >> you'd never get PG to run on such hardware anyway.
>
> > Btw, given that we store function pointers in datums, that ship has
> > sailed a long while ago, no?
>
> We do? Data pointers, sure, but I can't think of any use of the other.

It seems I've misremembered. For some reason I was quite certain that
the resowner callback facility, dynamic bgworkers, and fdws did so,
but...

Greetings,

Andres Freund

On 2018-03-26 11:31:36 -0700, Andres Freund wrote:
> On 2018-03-26 11:14:03 -0400, Robert Haas wrote:
> > On Sat, Mar 24, 2018 at 4:33 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > I don't think it really changes the need, but it's worthwhile to note
> > > that gcc-8 warns about this now:
> > > /home/andres/src/postgresql/src/backend/postmaster/bgworker.c: In function ‘LookupBackgroundWorkerFunction’:
> > > /home/andres/src/postgresql/src/backend/postmaster/bgworker.c:1246:9: warning: cast between incompatible function types from ‘PGFunction’ {aka ‘long unsigned int (*)(struct FunctionCallInfoData *)’} to ‘void (*)(Datum)’ {aka ‘void (*)(long unsigned int)’} [-Wcast-function-type]
> > > return (bgworker_main_type)
> >
> > That's probably going to mean we need, or at least want, to do
> > something about this at some point. Warning-free compiles are
> > desirable.
>
> Right. We can suppress those if we want, either by adding another cast,
> or by adding -Wno-cast-function-type. The latter wouldn't be entirely a
> bad idea, it seems to be a warning of very limited benefit.

I dug up a thread about the introduction of the warning:
https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00423.html

Sounds like we should add something like
typedef void (*GenericFuncPtr) (void);
or such? Similar to what Tom proposed upthread.

- Andres

On Mon, Mar 26, 2018 at 4:16 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> I dug up a thread about the introduction of the warning:
> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00423.html
>
> Sounds like we should add something like
> typedef void (*GenericFuncPtr) (void);
> or such? Similar to what Tom proposed upthread.

His proposal was void (*)() rather than void (*)(void). I see that
the email to which you linked expects the latter, but I guess I would
have expected the former to be an intentional statement that we don't
know what the parameter list is. My expectations may be wrong,
though, or just irrelevant.

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

On 2018-05-10 16:41:53 -0400, Robert Haas wrote:
> On Mon, Mar 26, 2018 at 4:16 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > I dug up a thread about the introduction of the warning:
> > https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00423.html
> >
> > Sounds like we should add something like
> > typedef void (*GenericFuncPtr) (void);
> > or such? Similar to what Tom proposed upthread.
>
> His proposal was void (*)() rather than void (*)(void). I see that
> the email to which you linked expects the latter, but I guess I would
> have expected the former to be an intentional statement that we don't
> know what the parameter list is. My expectations may be wrong,
> though, or just irrelevant.

Possible. But IIRC the parameter-unknown form isn't valid C++ and Peter
Eisentraut has done a good chunk of work to make it possible to compile
postgres as that. We shouldn't make his job harder. IMO the important
part isn't that the parameters fit exactly - we'll have to cast for the
return type anyway - but that it's declared as a pointer-to-function for
the hyptothetical supported platform that has different pointers to
functions than to other objects.

Greetings,

Andres Freund

On Thu, May 10, 2018 at 4:45 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Possible. But IIRC the parameter-unknown form isn't valid C++ and Peter
> Eisentraut has done a good chunk of work to make it possible to compile
> postgres as that. We shouldn't make his job harder.

Fair point.

> IMO the important
> part isn't that the parameters fit exactly - we'll have to cast for the
> return type anyway - but that it's declared as a pointer-to-function for
> the hyptothetical supported platform that has different pointers to
> functions than to other objects.

Probably the more relevant concern is what's going to compile
warning-free on all supported compilers. I think it's unlikely that
such a hypothetical supported platform actually exists, although maybe
I'm wrong.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, May 10, 2018 at 4:45 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> IMO the important
>> part isn't that the parameters fit exactly - we'll have to cast for the
>> return type anyway - but that it's declared as a pointer-to-function for
>> the hyptothetical supported platform that has different pointers to
>> functions than to other objects.

> Probably the more relevant concern is what's going to compile
> warning-free on all supported compilers.

+1

> I think it's unlikely that
> such a hypothetical supported platform actually exists, although maybe
> I'm wrong.

Such platforms certainly used to exist, and not that long ago either.
I think the last common example was that Intel compilers used to let
you choose the size of data pointers separately from the size of code
pointers, cf
https://en.wikipedia.org/wiki/Intel_Memory_Model

There's some other entertaining reading here:
https://en.wikipedia.org/wiki/Harvard_architecture

Those sorts of pushups have fallen into disfavor with the availability
of larger address spaces, and these days it's a bit hard to imagine
anybody porting PG to a new platform that works like that. But that's
why the C standard discourages considering code and data pointers as
being interchangeable.

regards, tom lane