Thread: 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@anarazel.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@anarazel.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@anarazel.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@anarazel.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@anarazel.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
unsignedint)’} [-Wcast-function-type]
return (bgworker_main_type)
Greetings,
Andres Freund
On Sat, Mar 24, 2018 at 4:33 PM, Andres Freund <andres@anarazel.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
typesfrom ‘PGFunction’ {aka ‘long unsigned int (*)(struct FunctionCallInfoData *)’} to ‘void (*)(Datum)’ {aka ‘void
(*)(longunsigned 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@anarazel.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
typesfrom ‘PGFunction’ {aka ‘long unsigned int (*)(struct FunctionCallInfoData *)’} to ‘void (*)(Datum)’ {aka ‘void
(*)(longunsigned 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@anarazel.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@anarazel.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@anarazel.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
typesfrom ‘PGFunction’ {aka ‘long unsigned int (*)(struct FunctionCallInfoData *)’} to ‘void (*)(Datum)’ {aka ‘void
(*)(longunsigned 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@anarazel.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@anarazel.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@anarazel.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@gmail.com> writes: > On Thu, May 10, 2018 at 4:45 PM, Andres Freund <andres@anarazel.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