Thread

Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2009-11-29T19:11:04Z

Hi,

The attached patch is offered for the 2010-01 commitfest.
It's also available in my git repository in the "submitted" branch:
   http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted

In palloc.h and pg_list.h, some inline functions are defined if
allowed by the compiler.  Previously the test was gcc-specific.
This patch enables inlining on more platforms, relying on the
Autoconf-generated "configure" script to determine whether
the compiler supports inline functions.

Depending on the compiler, the keyword for defining an inline
function might be spelled as inline, __inline, or __inline__,
or none of these.  "configure" finds out, and except in the
first case, puts a suitable "#define inline" into pg_config.h.
This hasn't changed.

What's new is that "configure" will add "#define HAVE_INLINE 1"
into pg_config.h if it detects that the compiler accepts inline
function definitions.  palloc.h and pg_list.h will condition
their inline function definitions on HAVE_INLINE instead of
the gcc-specific __GNUC__.

After applying the patch, run these commands to update
the configure script and pg_config.h.in:
	autoconf; autoheader

(Does anybody still use a C compiler that doesn't support
inline functions?)

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2009-11-29T21:46:29Z

Kurt Harriman <harriman@acm.org> writes:
> (Does anybody still use a C compiler that doesn't support
> inline functions?)

The question isn't so much that, it's whether the compiler supports
inline functions with the same behavior as gcc.  At minimum that
would require
	* not generating extra copies of the function
	* not whining about unreferenced static functions
How many compilers have you tested this patch against?  Which ones
does it actually offer any benefit for?

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-11-30T07:44:31Z

On 11/29/09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kurt Harriman <harriman@acm.org> writes:
>  > (Does anybody still use a C compiler that doesn't support
>  > inline functions?)

+1 for modern C.

> The question isn't so much that, it's whether the compiler supports
>  inline functions with the same behavior as gcc.  At minimum that
>  would require
>         * not generating extra copies of the function
>         * not whining about unreferenced static functions
>  How many compilers have you tested this patch against?  Which ones
>  does it actually offer any benefit for?

Those are not correctness problems.  Compilers with non-optimal or
missing 'inline' do belong together with compilers without working int64.
We may spend some effort to be able to compile on them, but they
should not affect our coding style.

'static inline' is superior to macros because of type-safety and
side-effects avoidance.  I'd suggest event removing any HAVE_INLINE
ifdefs and let autoconf undef the 'inline' if needed.  Less duplicated
code to maintain.  The existence of compilers in active use without
working 'inline' seems quite hypothetical these days.

-- 
marko

Re: Patch: Remove gcc dependency in definition of inline functions

Bruce Momjian <bruce@momjian.us> — 2009-11-30T12:06:18Z

Marko Kreen wrote:
> On 11/29/09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Kurt Harriman <harriman@acm.org> writes:
> >  > (Does anybody still use a C compiler that doesn't support
> >  > inline functions?)
> 
> +1 for modern C.
> 
> > The question isn't so much that, it's whether the compiler supports
> >  inline functions with the same behavior as gcc.  At minimum that
> >  would require
> >         * not generating extra copies of the function
> >         * not whining about unreferenced static functions
> >  How many compilers have you tested this patch against?  Which ones
> >  does it actually offer any benefit for?
> 
> Those are not correctness problems.  Compilers with non-optimal or
> missing 'inline' do belong together with compilers without working int64.
> We may spend some effort to be able to compile on them, but they
> should not affect our coding style.
> 
> 'static inline' is superior to macros because of type-safety and
> side-effects avoidance.  I'd suggest event removing any HAVE_INLINE
> ifdefs and let autoconf undef the 'inline' if needed.  Less duplicated
> code to maintain.  The existence of compilers in active use without
> working 'inline' seems quite hypothetical these days.

I thought one problem was that inline is a suggestion that the compiler
can ignore, while macros have to be implemented as specified.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: Patch: Remove gcc dependency in definition of inline functions

Peter Eisentraut <peter_e@gmx.net> — 2009-11-30T14:03:21Z

On mån, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote:
> I thought one problem was that inline is a suggestion that the compiler
> can ignore, while macros have to be implemented as specified.

Sure, but one could argue that a compiler that doesn't support inline
usefully is probably not the sort of compiler that you use for compiling
performance-relevant software anyway.  We can support such systems in a
degraded way for historical value and evaluation purposes as long as
it's pretty much free, like we support systems without working int8.

Re: Patch: Remove gcc dependency in definition of inline functions

Bruce Momjian <bruce@momjian.us> — 2009-11-30T14:16:48Z

Peter Eisentraut wrote:
> On m?n, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote:
> > I thought one problem was that inline is a suggestion that the compiler
> > can ignore, while macros have to be implemented as specified.
> 
> Sure, but one could argue that a compiler that doesn't support inline
> usefully is probably not the sort of compiler that you use for compiling
> performance-relevant software anyway.  We can support such systems in a
> degraded way for historical value and evaluation purposes as long as
> it's pretty much free, like we support systems without working int8.

The issue is that many compilers will take "inline" as a suggestion and
decide if it is worth-while to inline it --- I don't think it is inlined
unconditionally by any modern compilers.

Right now we think we are better at deciding what should be inlined than
the compiler --- of course, we might be wrong, and it would be good to
performance test this.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-11-30T15:30:13Z

On 11/30/09, Bruce Momjian <bruce@momjian.us> wrote:
> Peter Eisentraut wrote:
> > On m?n, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote:
>  > > I thought one problem was that inline is a suggestion that the compiler
>  > > can ignore, while macros have to be implemented as specified.
>  >
>  > Sure, but one could argue that a compiler that doesn't support inline
>  > usefully is probably not the sort of compiler that you use for compiling
>  > performance-relevant software anyway.  We can support such systems in a
>  > degraded way for historical value and evaluation purposes as long as
>  > it's pretty much free, like we support systems without working int8.
>
>
> The issue is that many compilers will take "inline" as a suggestion and
>  decide if it is worth-while to inline it --- I don't think it is inlined
>  unconditionally by any modern compilers.
>
>  Right now we think we are better at deciding what should be inlined than
>  the compiler --- of course, we might be wrong, and it would be good to
>  performance test this.

Note - my proposal would be to get rid of HAVE_INLINE, which
means we are already using inline functions unconditionally
on platforms that matter (gcc).  Keeping duplicate code
for obsolete compilers is pointless.

I'm not suggesting converting all existing macros to inline.  This
can happen slowly, and where it brings benefit (short but multi-arg
are probably most worthwhile to convert).  Also new macros are better
done as inlines.

About uninlining - usually if the compiler decides to uninline,
it is probably right anyway.  The prime example would be Linux kernel
where the 'inline' is used quite a lot as go-faster-magic-dust on totally
random functions.  (In .c files, not talking about headers)

Most compilers (gcc, vc, icc) support also force-inlining, which is
not taken as hint but command.  If you want to be on safe side, you
could define 'inline' as force-inline on compilers that support that.

-- 
marko

Re: Patch: Remove gcc dependency in definition of inline functions

james <james@mansionfamily.plus.com> — 2009-12-02T19:45:44Z

Marko Kreen wrote:
> Note - my proposal would be to get rid of HAVE_INLINE, which
> means we are already using inline functions unconditionally
> on platforms that matter (gcc).  Keeping duplicate code
> for obsolete compilers is pointless.
>   
Microsoft C doesn't matter?

I seem to remember that when the Win32 version became available it 
actually increased the
number of people trying postgres rather dramatically.  Did that count 
for nothing?

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-02T19:51:45Z

James Mansion <james@mansionfamily.plus.com> writes:
> Marko Kreen wrote:
>> Note - my proposal would be to get rid of HAVE_INLINE, which
>> means we are already using inline functions unconditionally
>> on platforms that matter (gcc).  Keeping duplicate code
>> for obsolete compilers is pointless.

> Microsoft C doesn't matter?

Breaking compilers that don't have inline at all isn't happening;
it wouldn't buy us anything much anyway.  The debate here is about
how much we can assume about the behavior of compilers that do
recognize the keyword.  In particular, do they behave sensibly
when finding an unreferenced static inline function, which is what
would occur in many modules if we allow them to see inline functions
in headers.

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-12-02T21:30:12Z

On 12/2/09, James Mansion <james@mansionfamily.plus.com> wrote:
> Marko Kreen wrote:
>
> > Note - my proposal would be to get rid of HAVE_INLINE, which
> > means we are already using inline functions unconditionally
> > on platforms that matter (gcc).  Keeping duplicate code
> > for obsolete compilers is pointless.
> >
> >
>  Microsoft C doesn't matter?
>
>  I seem to remember that when the Win32 version became available it actually
> increased the
>  number of people trying postgres rather dramatically.  Did that count for
> nothing?

The "(gcc)" above meant the inline functions are already used with gcc.

I have no reason to think Microsoft's inlining works worse than gcc's.

IOW - if the compiler does not support 'static inline' we should fall back
to plain 'static' functions, instead maintaining duplicate macros.
Such compilers would take a efficiency hit, but as they are practically
non-existent they dont matter.

Microsoft C does support inline, so it would not be affected.

-- 
marko

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2009-12-06T10:21:42Z

Tom Lane wrote:
> Kurt Harriman <harriman@acm.org> writes:
>> (Does anybody still use a C compiler that doesn't support
>> inline functions?)

> The question isn't so much that, it's whether the compiler supports
> inline functions with the same behavior as gcc.  

With this patch, if the compiler doesn't accept the "inline" keyword
(or __inline or __inline__), then HAVE_INLINE remains undefined, and
the out-of-line definitions are used.

So, these concerns would be applicable in case there is a compiler
which accepts the keyword but ignores it, thus fooling "configure".

(Also these concerns become applicable if we eliminate the
out-of-line definitions altogether as some have suggested.)

> At minimum that would require
> 	* not generating extra copies of the function

Even if someone uses such a compiler, maybe the extra copies are not
too bad.  These are small functions.  If not inlined, the compiled
code size on x86-32 is
	list_head		 48 bytes
	list_tail		 48 bytes
	list_length		 48 bytes
	MemoryContextSwitchTo	 32 bytes
			 Total	176 bytes
In src/backend there are 478 .c files that #include postgres.h, so the
potential bloat is about 82 KB (= 478 * 176).

This would also occur anytime the user specifies compiler options that
suppress inlining, such as for a debug build; but then the executable
size doesn't matter usually.

> 	* not whining about unreferenced static functions

I could update the patch so that "configure" will test for this.

(BTW, MSVC is a whiner, but clams up if __forceinline is used.)

> How many compilers have you tested this patch against?  

Only a small number.  By submitting it to pgsql-hackers, I hope that
it can be exposed to broader coverage.

> Which ones does it actually offer any benefit for?

MSVC is one.

I hope eventually it will be found that all compilers of interest
do a good enough job with static inline functions so that we can
use a lot more of them.  For example, I'd like to expunge the
abominable heap_getattr() and fastgetattr() macros in access/htup.h
and replace them with an inline function.  Such improvements are
less easily justifiable if they are limited to gcc.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2009-12-06T11:20:10Z

Bruce Momjian wrote:
> Peter Eisentraut wrote:
>> On m?n, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote:
>>> I thought one problem was that inline is a suggestion that the compiler
>>> can ignore, while macros have to be implemented as specified.
>> Sure, but one could argue that a compiler that doesn't support inline
>> usefully is probably not the sort of compiler that you use for compiling
>> performance-relevant software anyway.  We can support such systems in a
>> degraded way for historical value and evaluation purposes as long as
>> it's pretty much free, like we support systems without working int8.
> 
> The issue is that many compilers will take "inline" as a suggestion and
> decide if it is worth-while to inline it --- I don't think it is inlined
> unconditionally by any modern compilers.
> 
> Right now we think we are better at deciding what should be inlined than
> the compiler --- of course, we might be wrong, and it would be good to
> performance test this.

Just to clarify, this patch doesn't add new inline functions or
convert any macros to inline functions.  At present, PostgreSQL
header files define four functions as inline for gcc only.  This
patch merely opens up those existing inline functions to be used
with any compiler that accepts them, not just gcc.  If this goes
well, it may be a preparatory step toward future adoption of more
inline functions and fewer macros.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2009-12-15T11:15:15Z

Hi,

Attached is a revised patch, offered for the 2010-01 commitfest.
It's also available in my git repository in the "submitted" branch:
  http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted

In this version, the "configure" script tests whether a static
inline function can be defined without incurring a warning when
not referenced.  If successful, the preprocessor symbol PG_INLINE
is defined in pg_config.h to the appropriate keyword: inline,
__inline, __inline__, or __forceinline.  Otherwise PG_INLINE
remains undefined.

palloc.h and pg_list.h condition their inline function
definitions on PG_INLINE instead of the gcc-specific __GNUC__.
Thus the functions can be inlined on more platforms, not only
gcc.

Ordinary out-of-line calls are still used if the compiler doesn't
recognize inline functions, or spews warnings when static inline
functions are defined but not referenced.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-12-15T12:05:13Z

On 12/15/09, Kurt Harriman <harriman@acm.org> wrote:
>  Attached is a revised patch, offered for the 2010-01 commitfest.
>  It's also available in my git repository in the "submitted" branch:
>
> http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted
>
>  In this version, the "configure" script tests whether a static
>  inline function can be defined without incurring a warning when
>  not referenced.  If successful, the preprocessor symbol PG_INLINE
>  is defined in pg_config.h to the appropriate keyword: inline,
>  __inline, __inline__, or __forceinline.  Otherwise PG_INLINE
>  remains undefined.
>
>  palloc.h and pg_list.h condition their inline function
>  definitions on PG_INLINE instead of the gcc-specific __GNUC__.
>  Thus the functions can be inlined on more platforms, not only
>  gcc.
>
>  Ordinary out-of-line calls are still used if the compiler doesn't
>  recognize inline functions, or spews warnings when static inline
>  functions are defined but not referenced.

-1.  The PG_INLINE is ugly.

In actual C code we should see only "inline" keyword, no XX_INLINE,
__inline, __inline__, etc.  It's up to configure to make sure "inline"
is something that C compiler accepts or simply defined to empty string
otherwise.

I assume you are playing with force-inline to avoid warnings on some
compiler?  Do you a actual compiler that behaves like that?  Unless
it is some popular compiler (as "in actual use") it is premature
complexity.  Please remove that.

We may want to have force-inline in the future, when we start converting
some more complex macros to inline functions, but then it should cover
all main compilers, and current patch does not try to do it.

So my suggestions:

1. Make sure "inline" is defined, empty or to something that works.
   (plain AC_C_INLINE seems to do that)
2. Convert current __inline, __inline__ sites to "inline"

and

3. Remove #ifdefs and duplicate macros, keeping only inline funcs.

There does not seem to be any holding counter-arguments why we should
not do that, so lets go ahead?

-- 
marko

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2009-12-15T21:12:47Z

On 12/15/2009 4:05 AM, Marko Kreen wrote:
> On 12/15/09, Kurt Harriman<harriman@acm.org>  wrote:
>>   Attached is a revised patch, offered for the 2010-01 commitfest.
>>   It's also available in my git repository in the "submitted" branch:
>>
>> http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted
>>

> -1.  The PG_INLINE is ugly.
>
> In actual C code we should see only "inline" keyword, no XX_INLINE,
> __inline, __inline__, etc.  It's up to configure to make sure "inline"
> is something that C compiler accepts or simply defined to empty string
> otherwise.
>
> I assume you are playing with force-inline to avoid warnings on some
> compiler?  Do you a actual compiler that behaves like that?  Unless
> it is some popular compiler (as "in actual use") it is premature
> complexity.  Please remove that.
>
> We may want to have force-inline in the future, when we start converting
> some more complex macros to inline functions, but then it should cover
> all main compilers, and current patch does not try to do it.
>
> So my suggestions:
>
> 1. Make sure "inline" is defined, empty or to something that works.
>     (plain AC_C_INLINE seems to do that)
> 2. Convert current __inline, __inline__ sites to "inline"
>
> and
>
> 3. Remove #ifdefs and duplicate macros, keeping only inline funcs.
>
> There does not seem to be any holding counter-arguments why we should
> not do that, so lets go ahead?

Hi Marko,

Thanks for your comments.

Although I agree that the nicest code would be as you suggest, and
that would work fine with gcc, I hesitate to dismiss so quickly the
counter-arguments:

	i) That would reduce the number of platforms on which the
	backend compiles cleanly with no warnings.  Microsoft C
	platforms would be among those affected.

	ii) Your item #3 would unnecessarily increase the size of
	the postgres executable on platforms where no inline keyword
	is recognized and unused static functions aren't optimized
	away.  The increase would be about 82 KB if this were to
	occur on an x86-32 platform, for example.  Maybe nobody still
	uses such an old compiler, or if they do, maybe the increased
	executable size would be small enough to ignore; but I don't
	know this.  Users who would be affected are not necessarily
	readers of the pgsql-hackers list.

This patch is meant to broaden the number of platforms benefiting
from inlining.  Also it is hoped to facilitate greater use of inline
functions in the future, in preference to macros.  I don't want to
disadvantage the older platforms, unless they are unambiguously not
important anymore.

 > -1.  The PG_INLINE is ugly.

Yes, but not quite as ugly as the existing code.  It is more
portable, eliminating hard coded gcc dependencies in several
places.

 > 1. Make sure "inline" is defined, empty or to something that works.
 >     (plain AC_C_INLINE seems to do that)

Yes, it is already that way in the existing code.  If the compiler
doesn't accept plain "inline", then the "configure" script sets up
pg_config.h to #define inline as a preprocessor symbol that maps to
__inline, __inline__, or empty.  As you say, plain AC_C_INLINE takes
care of that.  My patch doesn't affect this.

 > In actual C code we should see only "inline" keyword, no XX_INLINE,
 > __inline, __inline__, etc.  It's up to configure to make sure "inline"
 > is something that C compiler accepts or simply defined to empty string
 > otherwise.

Yes, that is already how we define inline functions in .c files
such as executor/nodeSetOp.c, optimizer/path/joinpath.c, and
storage/lmgr/lock.c.  It seems to work fine there.

However, there is a difference between .c files and .h files.

A static inline definition in a .c file is certain to be referenced,
and is never instantiated more than once even if not inlined.

Therefore in .c files we need not be concerned about the two issues
mentioned above: superfluous "defined but not used" warnings, and
unnecessary bloating of the executable.

On the other hand, a static inline definition in a .h file is
liable to be #included into (perhaps hundreds of) compilations
where it is not referenced.  Some compilers conscientiously
display a warning every time this happens.  Some emit
out-of-line code every time they see the definition, even if
it is not called (likeliest when inline is not supported at all
and is #defined to empty).

To avoid these problems, historically the PostgreSQL developers
have minimized the use of inline functions in header files, and
instead used macros.  In the two header files where macros were
considered too ugly, inline functions were conditionally
compiled for gcc only.  At that time, __inline__ was a
nonstandard gcc extension.

Nowadays, inline functions are part of standard C99 and C++.
For static inline functions, the C99 and C++ standard behavior
is basically the same as the old gcc extension.  So by now,
most compilers support this somehow, and it makes sense to
remove the gcc dependency.

 > I assume you are playing with force-inline to avoid warnings on some
 > compiler?  Do you a actual compiler that behaves like that?

Yes, Microsoft compilers warn about unreferenced static
functions, but keep quiet when the function is defined with
__forceinline.

Inline functions defined in header files are typically small
enough that compilers will always obey the inline directive,
so it doesn't matter whether plain inline or __forceinline is
used, except for the warnings.  This is certainly the case for
the four inline functions which exist at present in palloc.h
and pg_list.h.

So far in my experience outside PostgreSQL, compilers almost
always inline everything that I have defined as inline.  I've
never had occasion to use __forceinline with a modern compiler,
except now for the MSVC warnings.

 > Unless it is some popular compiler (as "in actual use") it is
 > premature complexity.  Please remove that.

Microsoft's compilers are in actual use, and some might even
say they are popular.  For example, James Mansion posted to
this effect on 2 December.

 > We may want to have force-inline in the future, when we start converting
 > some more complex macros to inline functions, but then it should cover
 > all main compilers, and current patch does not try to do it.

At present I do not know of any reason why we should want
__forceinline, except here for its side-effect of silencing
MSVC's warnings about static functions that are defined but
not used.

Instead of using __forceinline, it would be possible to use
CFLAGS or a #pragma to globally suppress the unreferenced
symbol warnings.  However, the warning codes seem less
portable because they may be specific to one compiler or
change between releases.  Also, useful warnings might be
hidden along with the superfluous ones.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-12-15T21:31:08Z

On 12/15/09, Kurt Harriman <harriman@acm.org> wrote:
> On 12/15/2009 4:05 AM, Marko Kreen wrote:
>  > Unless it is some popular compiler (as "in actual use") it is
>  > premature complexity.  Please remove that.
>
>  Microsoft's compilers are in actual use, and some might even
>  say they are popular.  For example, James Mansion posted to
>  this effect on 2 December.

Erm, AFAIK he simply misunderstood my "(gcc)" comment.

Do you have actual proof that MSVC launches warnings on unused
"static inline" functions?  Not "static", but "static inline".

If yes, indeed we need to fix it.  MSVC is broken then, but it does
not matter as we need to work well on it.  We can fix it with either
force-inline, or equivalent with gcc's __attribute__((unused)).

If no, then we don't need to fix it.  Adding complexity based on
some email miscommunication seems wrong to me...

-- 
marko

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2009-12-15T21:46:48Z

On 12/15/2009 1:31 PM, Marko Kreen wrote:
> Do you have actual proof that MSVC launches warnings on unused
> "static inline" functions?  Not "static", but "static inline".

Yes, I tried it, and that's why I did it this way.

> If yes, indeed we need to fix it.  MSVC is broken then, but it does
> not matter as we need to work well on it.  We can fix it with either
> force-inline, or equivalent with gcc's __attribute__((unused)).

Microsoft doesn't support the gcc __attribute__ syntax, AFAIK.

They have a few other gewgaws, like __declspec__, but I didn't
find one that helps with this problem.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-12-15T22:09:30Z

On 12/15/09, Kurt Harriman <harriman@acm.org> wrote:
> On 12/15/2009 1:31 PM, Marko Kreen wrote:
>
> > Do you have actual proof that MSVC launches warnings on unused
> > "static inline" functions?  Not "static", but "static inline".
>
>  Yes, I tried it, and that's why I did it this way.

Oh.  Ok then.  Force-inline seems better fix as we may want to use
it for other reasons too (converting big macros).

So it seems like a good moment to solve it for gcc too.

Your worry ii) can be ignored, managing to compile on such
compilers is already overachievement.

> > If yes, indeed we need to fix it.  MSVC is broken then, but it does
> > not matter as we need to work well on it.  We can fix it with either
> > force-inline, or equivalent with gcc's __attribute__((unused)).
> >
>
>  Microsoft doesn't support the gcc __attribute__ syntax, AFAIK.
>
>  They have a few other gewgaws, like __declspec__, but I didn't
>  find one that helps with this problem.

The question is now what should we put into configure and what into
headers.

Simplest would be to have plain AC_C_INLINE in configure
and then in header (c.h?):

#ifdef _MSC_VER
#undef inline
#define inline __forceinline
#else
#ifdef __GNUC__
#undef inline
#define inline inline __attribute__((always_inline))
#endif
#endif

(Not compile tested :)

Or should we put that logic also into configure,
so that the config.h already contains proper 'inline'?

Although fitting it into win32 buildsystem seems to be bit more
complex in that case...

-- 
marko

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2009-12-16T02:30:08Z

On 12/15/2009 2:09 PM, Marko Kreen wrote:
>
> Oh.  Ok then.  Force-inline seems better fix as we may want to use
> it for other reasons too (converting big macros).
>
> So it seems like a good moment to solve it for gcc too.

Is ordinary inlining not sufficient?

If deluxe inlining is something that might be wanted in the
future, but isn't needed right now, perhaps we should wait
until then.

> Your worry ii) can be ignored, managing to compile on such
> compilers is already overachievement.

I think so too.  With your opinion added to mine, do we constitute a
consensus of the pg community?  Someone might object that a sample of
two individuals is insufficiently representative of the whole, but
away with the pedants: let us not quibble over trifles.

> The question is now what should we put into configure and what into
> headers.

PostgreSQL seems mostly to follow the GNU

> Simplest would be to have plain AC_C_INLINE in configure
> and then in header (c.h?):
>
> #ifdef _MSC_VER
> #undef inline
> #define inline __forceinline
> #else
> #ifdef __GNUC__
> #undef inline
> #define inline inline __attribute__((always_inline))
> #endif
> #endif
>
> (Not compile tested :)
>
> Or should we put that logic also into configure,
> so that the config.h already contains proper 'inline'?
>
> Although fitting it into win32 buildsystem seems to be bit more
> complex in that case...

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2009-12-16T03:34:40Z

[Please ignore the previous incomplete version of this reply, which I
sent by mistake.  Sorry for the list noise.]

On 12/15/2009 2:09 PM, Marko Kreen wrote:
 >
 > Oh.  Ok then.  Force-inline seems better fix as we may want to use
 > it for other reasons too (converting big macros).
 >
 > So it seems like a good moment to solve it for gcc too.

Is ordinary inlining not sufficient?

If deluxe inlining is something that might be wanted in the
future, but isn't needed right now, perhaps we should wait
until then.

 > Your worry ii) can be ignored, managing to compile on such
 > compilers is already overachievement.

I think so too.  With your opinion added to mine, do we constitute a
consensus of the pg community?  Someone might object that a sample of
two individuals is insufficiently representative of the whole, but
away with the pedants: let us not quibble over trifles.

 > The question is now what should we put into configure and what into
 > headers.

PostgreSQL seems mostly to follow the GNU tradition of using
autoconf rather than thickets of #ifdefs to discover platform
characteristics such as supported features, variant spellings of
keywords and identifiers, and the like.  This often makes it easier
to support new platforms, assuming the autoconf mechanism is
understood.

Autoconf facilitates testing directly for the needed features:
generally a better approach than hard-coding knowledge of the
peculiarities of particular compilers, compiler release levels,
instruction set architectures, etc.  For example, instead of
writing #ifdefs to decide, "if this is gcc, and the version is
high enough, then __funcname__ should work", it's better to let
autoconf actually try to compile a program using __funcname__.
That way PostgreSQL can keep up with the evolution of improved
and new compilers without continually updating an #ifdef-encoded
knowledge base of compiler capabilities.

 > Simplest would be to have plain AC_C_INLINE in configure
 > and then in header (c.h?):
 >
 > #ifdef _MSC_VER
 > #undef inline
 > #define inline __forceinline
 > #else
 > #ifdef __GNUC__
 > #undef inline
 > #define inline inline __attribute__((always_inline))
 > #endif
 > #endif
 >
 > (Not compile tested :)

This would force every inline.  Why do we want that?

For gcc, I think the __attribute__ has to come after the function's
parameter list, rather than before the return type.

 > Or should we put that logic also into configure,
 > so that the config.h already contains proper 'inline'?

That's how it works at present for plain (not forced) inline.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Robert Haas <robertmhaas@gmail.com> — 2009-12-16T05:42:00Z

On Tue, Dec 15, 2009 at 10:34 PM, Kurt Harriman <harriman@acm.org> wrote:
>> Your worry ii) can be ignored, managing to compile on such
>> compilers is already overachievement.
>
> I think so too.  With your opinion added to mine, do we constitute a
> consensus of the pg community?  Someone might object that a sample of
> two individuals is insufficiently representative of the whole, but
> away with the pedants: let us not quibble over trifles.

I haven't completely followed this thread, but I think there has been
some discussion of making changes to inline that would cause
regressions for people using old, crappy compilers, and I think we
should avoid doing that unless there is some compelling benefit.  I'm
not sure what that benefit would be - I don't think "cleaner code" is
enough.

...Robert

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2009-12-16T07:29:33Z

On 12/15/2009 9:42 PM, Robert Haas wrote:
> On Tue, Dec 15, 2009 at 10:34 PM, Kurt Harriman<harriman@acm.org>  wrote:
>>> Your worry ii) can be ignored, managing to compile on such
>>> compilers is already overachievement.
>>
>> I think so too.  With your opinion added to mine, do we constitute a
>> consensus of the pg community?  Someone might object that a sample of
>> two individuals is insufficiently representative of the whole, but
>> away with the pedants: let us not quibble over trifles.
>
> I haven't completely followed this thread, but I think there has been
> some discussion of making changes to inline that would cause
> regressions for people using old, crappy compilers, and I think we
> should avoid doing that unless there is some compelling benefit.  I'm
> not sure what that benefit would be - I don't think "cleaner code" is
> enough.
>
> ...Robert

Hmm, this sample of 3 has a lot of variance now.
When I referred to "consensus", perhaps that was optimism?

In fact I cannot disagree with Robert's comment.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-12-16T14:23:51Z

On 12/16/09, Kurt Harriman <harriman@acm.org> wrote:
> [Please ignore the previous incomplete version of this reply, which I
>  sent by mistake.  Sorry for the list noise.]
>
>  On 12/15/2009 2:09 PM, Marko Kreen wrote:
>  >
>  > Oh.  Ok then.  Force-inline seems better fix as we may want to use
>  > it for other reasons too (converting big macros).
>  >
>  > So it seems like a good moment to solve it for gcc too.
>
>  Is ordinary inlining not sufficient?
>
>  If deluxe inlining is something that might be wanted in the
>  future, but isn't needed right now, perhaps we should wait
>  until then.

It allows the effects the __forceinline has on MSVC also observe
on GCC.  It should not hurt anything, unless we go overboard with
inline usage.

But yeah, it can be omitted.  I was just wondering on code structure,
to make it possible to turn forceinline for gcc too.

>  > Your worry ii) can be ignored, managing to compile on such
>  > compilers is already overachievement.
>
>  I think so too.  With your opinion added to mine, do we constitute a
>  consensus of the pg community?  Someone might object that a sample of
>  two individuals is insufficiently representative of the whole, but
>  away with the pedants: let us not quibble over trifles.

Note that we are not voting here, we are on technical discussion list.
Interested people can voice counter-arguments, if we can overturn them,
we can proceed...

>  > The question is now what should we put into configure and what into
>  > headers.
>
>  PostgreSQL seems mostly to follow the GNU tradition of using
>  autoconf rather than thickets of #ifdefs to discover platform
>  characteristics such as supported features, variant spellings of
>  keywords and identifiers, and the like.  This often makes it easier
>  to support new platforms, assuming the autoconf mechanism is
>  understood.
>
>  Autoconf facilitates testing directly for the needed features:
>  generally a better approach than hard-coding knowledge of the
>  peculiarities of particular compilers, compiler release levels,
>  instruction set architectures, etc.  For example, instead of
>  writing #ifdefs to decide, "if this is gcc, and the version is
>  high enough, then __funcname__ should work", it's better to let
>  autoconf actually try to compile a program using __funcname__.
>  That way PostgreSQL can keep up with the evolution of improved
>  and new compilers without continually updating an #ifdef-encoded
>  knowledge base of compiler capabilities.

Well, yeah, except on one platform we are mostly bypassing the
autoconf logic and have lot of duplicate logic.

But I'm not opposed putting it into configure, assuming the
final keyword is still called "inline".

>  > Simplest would be to have plain AC_C_INLINE in configure
>  > and then in header (c.h?):
>  >
>  > #ifdef _MSC_VER
>  > #undef inline
>  > #define inline __forceinline
>  > #else
>  > #ifdef __GNUC__
>  > #undef inline
>  > #define inline inline __attribute__((always_inline))
>  > #endif
>  > #endif
>  >
>  > (Not compile tested :)
>
>  This would force every inline.  Why do we want that?

The compiler is allowed to uninline a plain "inline" function
if it feels like it.  The gcc uses guesstimate instruction
count (before inlining) to decide on that issue.

ATM it's not a problem, but eg. if we want to turn more complex
macros into inlines (heap_getattr()), it may go over limit and get
uninlined.

>  For gcc, I think the __attribute__ has to come after the function's
>  parameter list, rather than before the return type.

No.


-- 
marko

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-12-16T14:30:19Z

On 12/16/09, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Dec 15, 2009 at 10:34 PM, Kurt Harriman <harriman@acm.org> wrote:
>  >> Your worry ii) can be ignored, managing to compile on such
>  >> compilers is already overachievement.
>  >
>  > I think so too.  With your opinion added to mine, do we constitute a
>  > consensus of the pg community?  Someone might object that a sample of
>  > two individuals is insufficiently representative of the whole, but
>  > away with the pedants: let us not quibble over trifles.
>
>
> I haven't completely followed this thread, but I think there has been
>  some discussion of making changes to inline that would cause
>  regressions for people using old, crappy compilers, and I think we
>  should avoid doing that unless there is some compelling benefit.  I'm
>  not sure what that benefit would be - I don't think "cleaner code" is
>  enough.

Seems you have not followed the thread...

Hypothetical old, crappy compilers would still work, only AC_C_INLINE
would turn "static inline" into plain "static", so hypothetically
they would get some warnings about unused functions.

As this is all hypothetical, I don't see why that should stop us
cleaning our code?

-- 
marko

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-16T14:50:53Z

Marko Kreen <markokr@gmail.com> writes:
> On 12/16/09, Kurt Harriman <harriman@acm.org> wrote:
>> For gcc, I think the __attribute__ has to come after the function's
>> parameter list, rather than before the return type.

> No.

[ squint... ]  That's nowhere documented that I can find: all the
examples in the gcc docs show __attribute__ after the parameters.
It does seem to work, but should we rely on it?

The bigger problem though is that not all versions of gcc understand
always_inline:

$ gcc -Wall check.c
check.c:3: warning: `always_inline' attribute directive ignored

which I think is sufficient reason to put an end to this sub-thread.
We have no particular need for force-inline semantics anyway, as
long as the compiler behaves reasonably for unreferenced inlines,
which gcc always has.

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Robert Haas <robertmhaas@gmail.com> — 2009-12-16T15:02:05Z

On Wed, Dec 16, 2009 at 9:30 AM, Marko Kreen <markokr@gmail.com> wrote:
> On 12/16/09, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Dec 15, 2009 at 10:34 PM, Kurt Harriman <harriman@acm.org> wrote:
>>  >> Your worry ii) can be ignored, managing to compile on such
>>  >> compilers is already overachievement.
>>  >
>>  > I think so too.  With your opinion added to mine, do we constitute a
>>  > consensus of the pg community?  Someone might object that a sample of
>>  > two individuals is insufficiently representative of the whole, but
>>  > away with the pedants: let us not quibble over trifles.
>>
>>
>> I haven't completely followed this thread, but I think there has been
>>  some discussion of making changes to inline that would cause
>>  regressions for people using old, crappy compilers, and I think we
>>  should avoid doing that unless there is some compelling benefit.  I'm
>>  not sure what that benefit would be - I don't think "cleaner code" is
>>  enough.
>
> Seems you have not followed the thread...
>
> Hypothetical old, crappy compilers would still work, only AC_C_INLINE
> would turn "static inline" into plain "static", so hypothetically
> they would get some warnings about unused functions.
>
> As this is all hypothetical, I don't see why that should stop us
> cleaning our code?

I don't think that hypothetical is the right word.  There is no
question that such compilers exist.  What seems to me to be relevant
is whether anyone is still using them.  Maybe with sufficient research
you could demonstrate that there are no platforms we support where
these are still in common use - e.g. the oldest version of AIX
compiler that has this problem is version X, but PostgreSQL is only
supported on versions Y and higher, where Y>X.  But I don't see that
any attempt has been made to do that research, or at least I haven't
seen any attempt to go through our supported platforms and discuss the
situation for each one.

I'm not really sure there's enough benefit in this project to warrant
that effort, but that's up to you to decide.  However, I am pretty
confident that there is going to be opposition to degrading
performance or causing lots of compiler warnings on older platforms
that are still supported.  In fact, such opposition has already been
registered on this thread and you can find it here:

http://archives.postgresql.org/pgsql-hackers/2009-12/msg00239.php

Our standard for accepting patches is that (1) they have a
demonstrated benefit and (2) they don't break anything.  Leaving aside
the question of whether this patch makes anything worse, it would
really improve the case for this patch if someone could enumerate the
supported platforms and compilers where it makes things better.

...Robert

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-16T15:10:30Z

Marko Kreen <markokr@gmail.com> writes:
> Hypothetical old, crappy compilers would still work, only AC_C_INLINE
> would turn "static inline" into plain "static", so hypothetically
> they would get some warnings about unused functions.

> As this is all hypothetical, I don't see why that should stop us
> cleaning our code?

There's nothing "hypothetical" about it --- I still regularly check
that the code builds on an old HP compiler that doesn't have inline.

I remind you that the project policy is to not require any compiler
features not found in C89.  If you can exploit inline on more compilers
than now, fine, but assuming that everything has got it is not OK.

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-12-16T15:18:18Z

On 12/16/09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marko Kreen <markokr@gmail.com> writes:
>  > On 12/16/09, Kurt Harriman <harriman@acm.org> wrote:
>
> >> For gcc, I think the __attribute__ has to come after the function's
>  >> parameter list, rather than before the return type.
>
>  > No.
>
>
> [ squint... ]  That's nowhere documented that I can find: all the
>  examples in the gcc docs show __attribute__ after the parameters.
>  It does seem to work, but should we rely on it?

Heh.  At least in 3.0, 3.1 and 4.2 docs there is:

  __attribute__((noreturn)) void d0 (void),
      __attribute__((format(printf, 1, 2))) d1 (const char *, ...),
      d2 (void);

describing that _att before decl list applies to all declarations,
but _att after decl applies to only that declaration.  That sort of
explains also why all examples have _att afterwards.

  http://gcc.gnu.org/onlinedocs/gcc-3.1.1/gcc/Attribute-Syntax.html

But I dare not to pick out sentences from Function-Attributes.html
that describe that...

>  The bigger problem though is that not all versions of gcc understand
>  always_inline:
>
>  $ gcc -Wall check.c
>  check.c:3: warning: `always_inline' attribute directive ignored

Oh, another argument against logic in headers.

>  which I think is sufficient reason to put an end to this sub-thread.
>  We have no particular need for force-inline semantics anyway, as
>  long as the compiler behaves reasonably for unreferenced inlines,
>  which gcc always has.

Ok.

-- 
marko

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-12-16T15:37:22Z

On 12/16/09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marko Kreen <markokr@gmail.com> writes:
>  > Hypothetical old, crappy compilers would still work, only AC_C_INLINE
>  > would turn "static inline" into plain "static", so hypothetically
>  > they would get some warnings about unused functions.
>
>  > As this is all hypothetical, I don't see why that should stop us
>  > cleaning our code?
>
>  There's nothing "hypothetical" about it --- I still regularly check
>  that the code builds on an old HP compiler that doesn't have inline.

Ok, good.  Thus far only argument was "historically they have existed",
which does not sound good enough worry about them.  If somebody is
actually testing and caring abouth such compilers, they need to be
taken more seriously.

>  I remind you that the project policy is to not require any compiler
>  features not found in C89.  If you can exploit inline on more compilers
>  than now, fine, but assuming that everything has got it is not OK.

Note - my advanced proposal (instead duplicate macros, let 'static inline'
functions fall back to being plain 'static') would still support
non-inline compilers, but with potentially small performance hit.

So the plain-C89 compilers would be downgraded to "second-class"
targets, not worth getting max performance out of them.

Is this OK?

-- 
marko

PostgreSQL project policy compendium

Kurt Harriman <harriman@acm.org> — 2009-12-16T15:44:50Z

On 12/16/2009 7:10 AM, Tom Lane wrote:
> the project policy is to not require any compiler features not found
 > in C89.

Is there somewhere a compendium of such policies which
fledgling hackers should consult to avoid embarrassment?

Regards,
... kurt

Re: PostgreSQL project policy compendium

Peter Eisentraut <peter_e@gmx.net> — 2009-12-16T15:51:27Z

On ons, 2009-12-16 at 07:44 -0800, Kurt Harriman wrote: 
> On 12/16/2009 7:10 AM, Tom Lane wrote:
> > the project policy is to not require any compiler features not found
>  > in C89.
> 
> Is there somewhere a compendium of such policies which
> fledgling hackers should consult to avoid embarrassment?

The installation instructions say that you need a C89 compiler.

Re: PostgreSQL project policy compendium

Robert Haas <robertmhaas@gmail.com> — 2009-12-16T15:53:30Z

On Wed, Dec 16, 2009 at 10:44 AM, Kurt Harriman <harriman@acm.org> wrote:
> On 12/16/2009 7:10 AM, Tom Lane wrote:
>>
>> the project policy is to not require any compiler features not found
>
>> in C89.
>
> Is there somewhere a compendium of such policies which
> fledgling hackers should consult to avoid embarrassment?

Well the best way to avoid embarrassment is to just not be
embarrassed.  I don't think anyone will think ill of you for
suggesting something that turns out not to be where the project wants
to go, as long as you're not too dogmatic about it.

This particular point is mentioned in the documentation:

http://www.postgresql.org/docs/8.4/static/install-requirements.html

The rest of chapter 15 is worth a read, too.

I do think your basic point is well-taken, though.  There are a lot of
details about how we do things that I have had to learn through
osmosis, and maybe if I can figure out what they all are, I'll try to
write them down somewhere, if someone else hasn't done it already.

...Robert

Re: PostgreSQL project policy compendium

Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-16T16:09:13Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Dec 16, 2009 at 10:44 AM, Kurt Harriman <harriman@acm.org> wrote:
>> Is there somewhere a compendium of such policies which
>> fledgling hackers should consult to avoid embarrassment?

> I do think your basic point is well-taken, though.  There are a lot of
> details about how we do things that I have had to learn through
> osmosis, and maybe if I can figure out what they all are, I'll try to
> write them down somewhere, if someone else hasn't done it already.

Yeah, this is something that is difficult for the senior hackers to
undertake because we've forgotten what it was we had to learn the hard
way ;-).  Maybe some of the newer project members could collaborate
on scribbling down such stuff (wiki page?).

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Alvaro Herrera <alvherre@commandprompt.com> — 2009-12-16T16:10:47Z

Tom Lane escribió:

> I think you're way overthinking this.  Where we started was just
> a proposal to try to expand the set of inline-ing compilers beyond
> "gcc only".  I don't see why we need to do anything but that.  The
> code is fine as-is except for the control #ifdefs.

IIRC Kurt was also on about getting rid of some ugly macros that could
instead be coded as inline functions (fastgetattr for example)

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-16T16:21:59Z

Marko Kreen <markokr@gmail.com> writes:
> On 12/15/09, Kurt Harriman <harriman@acm.org> wrote:
>> Attached is a revised patch, offered for the 2010-01 commitfest.
>> It's also available in my git repository in the "submitted" branch:
>> 
>> http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted

> -1.  The PG_INLINE is ugly.

FWIW, I think the patch is largely OK, except for the autoconf hackery
which I'm not the best-qualified person to opine on.  I would only
suggest that the cleanest coding would be

	#ifdef USE_INLINE

	static inline foo(...) ...

	#else

	... non-inline definition of foo

	#endif

ie, go ahead and rely on autoconf's definition (if any) of "inline"
and add a policy symbol USE_INLINE to determine whether to use it.
The proposed PG_INLINE coding conflates the symbol needed in the code
with the policy choice.

Another possibility would be to call the policy symbol HAVE_INLINE,
but that (a) risks collision with a name defined by autoconf built-in
macros, and (b) looks like it merely indicates whether the compiler
*has* inline, not that we have made a choice about how to use it.

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Marko Kreen <markokr@gmail.com> — 2009-12-16T16:24:34Z

On 12/16/09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marko Kreen <markokr@gmail.com> writes:
>
> > So the plain-C89 compilers would be downgraded to "second-class"
>  > targets, not worth getting max performance out of them.
>
>
>  Hm?  Failing to inline is already a performance hit, which is why
>  Kurt got interested in this in the first place.
>
>  I think you're way overthinking this.  Where we started was just
>  a proposal to try to expand the set of inline-ing compilers beyond
>  "gcc only".  I don't see why we need to do anything but that.  The
>  code is fine as-is except for the control #ifdefs.

My proposal is basically about allowing more widespread use of
"static inline".  That is - "static inline" does not need to be
paired with equivalent macro.

But if C89 compilers are still project's primary target, then this
cannot be allowed.

-- 
marko

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-16T16:40:43Z

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane escribi:
>> I think you're way overthinking this.  Where we started was just
>> a proposal to try to expand the set of inline-ing compilers beyond
>> "gcc only".  I don't see why we need to do anything but that.  The
>> code is fine as-is except for the control #ifdefs.

> IIRC Kurt was also on about getting rid of some ugly macros that could
> instead be coded as inline functions (fastgetattr for example)

I'd just bounce that as useless activity.  If they are macros now,
and work, the only possible effects of changing them are negative.

			regards, tom lane

Re: PostgreSQL project policy compendium

Greg Smith <greg@2ndquadrant.com> — 2009-12-16T17:28:23Z

Kurt Harriman wrote:
> On 12/16/2009 7:10 AM, Tom Lane wrote:
>> the project policy is to not require any compiler features not found
> > in C89.
>
> Is there somewhere a compendium of such policies which
> fledgling hackers should consult to avoid embarrassment?

The list of suggestions at http://wiki.postgresql.org/wiki/Developer_FAQ 
and http://wiki.postgresql.org/wiki/Submitting_a_Patch are what we've 
got now.  There's a number of us who try to record any interesting 
discussion bits like this one into one of those, to shorten the future 
learning curve for others.

-- 
Greg Smith    2ndQuadrant   Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com  www.2ndQuadrant.com

Re: Patch: Remove gcc dependency in definition of inline functions

Peter Eisentraut <peter_e@gmx.net> — 2009-12-16T19:36:47Z

On ons, 2009-12-16 at 10:49 -0500, Tom Lane wrote:
> Marko Kreen <markokr@gmail.com> writes:
> > So the plain-C89 compilers would be downgraded to "second-class"
> > targets, not worth getting max performance out of them.
> 
> Hm?  Failing to inline is already a performance hit, which is why
> Kurt got interested in this in the first place.
> 
> I think you're way overthinking this.  Where we started was just
> a proposal to try to expand the set of inline-ing compilers beyond
> "gcc only".  I don't see why we need to do anything but that.  The
> code is fine as-is except for the control #ifdefs.

I think the ifdefs should just be HAVE_INLINE && !MSVC, right?

Re: Patch: Remove gcc dependency in definition of inline functions

Peter Eisentraut <peter_e@gmx.net> — 2010-01-17T19:27:23Z

On ons, 2009-12-16 at 10:49 -0500, Tom Lane wrote:
> I think you're way overthinking this.  Where we started was just
> a proposal to try to expand the set of inline-ing compilers beyond
> "gcc only".  I don't see why we need to do anything but that.  The
> code is fine as-is except for the control #ifdefs.

I have found an Autoconf macro that checks whether the compiler properly
supports C99 inline semantics.  This would allow us to replace the
__GNUC__ conditional with HAVE_C99_INLINE, in this case.  Interestingly,
however, this check results in GCC <=4.2 *not* supporting C99 inline,
apparently because it produces redundant copies of static inline
functions.  But GCC 4.2 is currently Debian stable, for example, so
de-supporting that is probably not yet an option.

So, I'm not sure if this would actually solve anyone's problem, but it
does call into question that exact semantics that we are looking for.
Maybe we just replace __GNUC__ by __GNUC__ || __SOMETHING_ELSE_CC__.

Patch attached for entertainment.

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2010-01-17T23:22:27Z

Peter Eisentraut <peter_e@gmx.net> writes:
> I have found an Autoconf macro that checks whether the compiler properly
> supports C99 inline semantics.  This would allow us to replace the
> __GNUC__ conditional with HAVE_C99_INLINE, in this case.  Interestingly,
> however, this check results in GCC <=4.2 *not* supporting C99 inline,
> apparently because it produces redundant copies of static inline
> functions.  But GCC 4.2 is currently Debian stable, for example, so
> de-supporting that is probably not yet an option.

Some of us are using much older gcc's than that, too ;-).  But actually
I think this test is 100% bogus anyhow.  What it appears to rely on is
the compiler failing to do semantic error testing on an inline function
that it doesn't need to instantiate.  That's a behavior that I'm pretty
sure is gcc-specific rather than required by C99, and in any case has
very little to do with our requirements.

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Peter Eisentraut <peter_e@gmx.net> — 2010-01-18T21:20:32Z

On sön, 2009-12-06 at 02:21 -0800, Kurt Harriman wrote:
> > Which ones does it actually offer any benefit for?
> 
> MSVC is one.

I seem to recall that somewhere else it was said that MSVC produces
warnings on unused static inline functions.  Am I mistaken?

Also, if this is mainly for the benefit of MSVC, we don't really need a
configure check, do we?

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-01-19T00:34:48Z

On 1/18/2010 1:20 PM, Peter Eisentraut wrote:
> I seem to recall that somewhere else it was said that MSVC produces
> warnings on unused static inline functions.  Am I mistaken?

MSVC does warn about unused static inline functions.  The warning
is prevented by using __forceinline instead of __inline.

> Also, if this is mainly for the benefit of MSVC, we don't really need a
> configure check, do we?

A configure check seems worthwhile regardless of MSVC.

No doubt other compilers, yet unknown, will be discovered to spew
alarm upon unreferenced static inline functions.  To address the
difficulty of testing innumerable present and future platforms,
it seems there are three alternatives:	

   a) Wait for complaints, then craft compiler-specific patches and
      backpatches.

   b) Keep the existing compiler-specific logic to enable inlining
      for gcc alone.  Craft additional compiler-specific logic for
      each additional trusted and sufficiently important compiler.

   c) Use configure to automate the testing of each build environment
      in situ.

The third alternative adapts to new or little-known platforms
with little or no manual intervention.  It factors platform
dependencies out of the code, centralizing them into the
build system.

It is true that configure doesn't need to test for MSVC's
__forceinline keyword.  I included that mainly as a placeholder
for the benefit of future hackers:  likely someone will
discover a need for a special keyword to suppress another
compiler's warnings.  With __forceinine as an example,
and the looping logic already in place, it's easy to add an
alternative keyword to the list without having to become an
autoconf expert.  It doesn't make configure slower, since
gcc succeeds on the first iteration without trying the
__forceinline alternative.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2010-01-19T00:42:28Z

Kurt Harriman <harriman@acm.org> writes:
>    c) Use configure to automate the testing of each build environment
>       in situ.

> The third alternative adapts to new or little-known platforms
> with little or no manual intervention.

This argument is bogus unless you can demonstrate a working configure
probe for the property in question.  The question about this patch,
from day one, has been whether we have a working configure test.

> It is true that configure doesn't need to test for MSVC's
> __forceinline keyword.  I included that mainly as a placeholder
> for the benefit of future hackers:  likely someone will
> discover a need for a special keyword to suppress another
> compiler's warnings.

I think including MSVC in the set of compilers targeted by a configure
test is just a waste of code.  It's more likely to confuse people than
help them.

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-01-19T03:40:49Z

On 1/18/2010 4:42 PM, Tom Lane wrote:
 > Kurt Harriman<harriman@acm.org>  writes:
 >>     c) Use configure to automate the testing of each build environment
 >>        in situ.
 >
 >> The third alternative adapts to new or little-known platforms
 >> with little or no manual intervention.
 >
 > This argument is bogus unless you can demonstrate a working configure
 > probe for the property in question.  The question about this patch,
 > from day one, has been whether we have a working configure test.

It does work to detect almost exactly the desired property:
the absence of a compiler or linker warning when a static
inline function is defined but not referenced.

"Almost" exactly, because rather than absence of errors or
warnings, the exact condition tested is: successful exit
code and nothing whatever written to stdout or stderr.
(That is a built-in feature of autoconf.)

Its success is easily demonstrated on any platform by editing the
code within AC_LINK_PROGRAM to induce a compiler or linker warning.

 >> It is true that configure doesn't need to test for MSVC's
 >> __forceinline keyword.  I included that mainly as a placeholder
 >> for the benefit of future hackers:  likely someone will
 >> discover a need for a special keyword to suppress another
 >> compiler's warnings.
 >
 > I think including MSVC in the set of compilers targeted by a configure
 > test is just a waste of code.  It's more likely to confuse people than
 > help them.

There's a loop for trying a series of compiler-specific
formulas.  The list contains two items:  (1) "inline" or
equivalent as determined by AC_C_INLINE; (2) __forceinline.
In a 2-item list, it is easy to see the syntax for adding a
third item: in this case, the language is 'sh' and the list
is space-separated.  The code is:

     for pgac_kw in "$ac_cv_c_inline" "__forceinline" ; do
       AC_LINK_IFELSE([AC_LANG_PROGRAM([static $pgac_kw int fun () {return 0;}],[])],
                      [pgac_cv_c_hushinline=$pgac_kw])
       test "$pgac_cv_c_hushinline" != no && break
     done

MSVC __forceinline could be dropped from the list:

     for pgac_kw in "$ac_cv_c_inline" ; do
       AC_LINK_IFELSE([AC_LANG_PROGRAM([static $pgac_kw int fun () {return 0;}],[])],
                      [pgac_cv_c_hushinline=$pgac_kw])
       test "$pgac_cv_c_hushinline" != no && break
     done

Since the list would then have only one element, the
whole loop could be dropped.

     AC_LINK_IFELSE([AC_LANG_PROGRAM([static $pgac_kw int fun () {return 0;}],[])],
                      [pgac_cv_c_hushinline=$pgac_kw])

Someone could reinstate the loop if it is discovered that MSVC
is not the only compiler which needs something special to
silence its unused-function warning.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Peter Eisentraut <peter_e@gmx.net> — 2010-01-19T07:48:03Z

On mån, 2010-01-18 at 16:34 -0800, Kurt Harriman wrote:
> MSVC does warn about unused static inline functions.  The warning
> is prevented by using __forceinline instead of __inline.

Hmm, but forceinline is not the same as inline.  Are we confident that
forcing inlining is not going to lead to disadvantages?  Has this been
measured?

Is there not a setting to disable this particular warning.  I read that
MSVC has various ways to set that sort of thing.

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-01-19T08:24:08Z

On 1/17/2010 11:27 AM, Peter Eisentraut wrote:
> I have found an Autoconf macro that checks whether the compiler properly
> supports C99 inline semantics.  This would allow us to replace the
> __GNUC__ conditional with HAVE_C99_INLINE, in this case.

At present, PostgreSQL uses only "static inline", where
"inline" might be mapped to empty or to an alternate spelling
via a #define generated by autoconf's AC_C_INLINE macro.

static inline functions seem to work the same (modulo spelling
and warnings) across many extended C89 compilers, including
gcc and MSVC.  The already existing de facto standard has been
adopted into standard C++ and C99.  Consequently, more
implementations will converge toward standard "static inline"
support.

However, C99 introduces its own new rules for functions that
are declared as inline but not static.  There was never a widely
accepted de facto standard for non-static inlines.  Thus, older
non-static inline functions typically require source changes in
order to upgrade to C99.  Because of this source-level upward
incompatibility, compilers might activate the new rules only
when compiling in C99 mode.

Under the C99 non-static inline rules, the programmer provides
an out-of-line definition which the compiler can call anytime
it decides not to use the inline definition.  A compiler might
implicitly generate an out-of-line instantiation of a static
inline function in every compilation unit where the definition
is seen; but by using the C99 non-static inline feature, the
programmer can prevent the implicit generation of out-of-line
copies of the function.  This is essentially an optimization
done manually by the programmer instead of automatically by
the compiler or linker.

Do we have any need for C99 non-static inlines?

I think we'll be alright if we continue to declare all of our
inline functions as static.  Multiple implicit out-of-line
instantiations are unlikely to be a problem because:

- typical inline functions are so simple that they'll always
   be generated inline - e.g. list_head()

- compilers commonly don't generate code for a static
   function unless the compilation unit contains a call
   to the function

- linkers typically delete functions that are not externally
   visible and are not called

- linkers may eliminate duplicate sections

- typical inline functions are so small that any remaining
   extra copies don't matter

At present, IMO, we don't need full C99 inline semantics.
The widely supported de facto standard "static inline"
semantics, checked by the existing AC_C_INLINE test, are
good enough, or nearly so.

To safely expand the use of inline functions in header files
across a wider set of platforms, we merely need to protect
against unwanted warnings from compilation units which don't
actually call all of the static inline functions defined in
their included headers.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-01-19T09:29:11Z

On 1/18/2010 11:48 PM, Peter Eisentraut wrote:
> On mån, 2010-01-18 at 16:34 -0800, Kurt Harriman wrote:
>> MSVC does warn about unused static inline functions.  The warning
>> is prevented by using __forceinline instead of __inline.
>
> Hmm, but forceinline is not the same as inline.  Are we confident that
> forcing inlining is not going to lead to disadvantages?  Has this been
> measured?

My patch uses __forceinline only in header files where it is
needed to avoid warnings.

Inline functions in header files are typically trivial: list_head(),
MemoryContextSwitchTo(); so I expect they'll always be inlined
no matter whether we use __inline or __forceinline.   These are
frequently called on heavily trafficked paths, so I think we
want them to be inlined always.

We have some existing inline functions in .c files.  These can be
more complicated, so it might be ok if the compiler decides to
leave them out-of-line.  And they are never unreferenced, so
suppression of unused-function warnings is not necessary and
perhaps not wanted.  To leave these functions undisturbed, my patch
doesn't redefine the "inline" keyword; instead it adds a new #define
PG_INLINE for use in header files where unused-function warnings
need to be suppressed.

> Is there not a setting to disable this particular warning.  I read that
> MSVC has various ways to set that sort of thing.

Yes, warnings can be turned off by a #pragma specifying the
warning number.

   http://msdn.microsoft.com/en-us/library/2c8f766e%28VS.71%29.aspx
   http://msdn.microsoft.com/en-us/library/cw9x3tcf%28VS.100%29.aspx
   http://msdn.microsoft.com/en-us/library/thxezb7y%28VS.71%29.aspx

To turn off unused-function warnings for just certain inline
functions, one could use

   #ifdef _MSC_VER
   #pragma warning(push)
   #pragma warning(disable:4514)
   #endif
      ... inline function definitions ...
   #ifdef _MSC_VER
   #pragma warning(pop)
   #endif

Or compiler switches could be set to disable all such warnings
globally.  Warning 4514 is specific to inline functions; so
maybe it would be alright to keep it turned off globally.

Note, this warning is disabled by default, but that is typically
overridden by the /Wall option which changes all off-by-default
warnings to become on-by-default.  Probably /Wall could be
combined with /Wd4514 to keep unused-inline-function warnings
turned off while enabling the rest.  If that is acceptable,
we wouldn't need #ifdefs, #pragmas, or __forceinline.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Peter Eisentraut <peter_e@gmx.net> — 2010-01-19T16:01:27Z

On tis, 2010-01-19 at 01:29 -0800, Kurt Harriman wrote:
> On 1/18/2010 11:48 PM, Peter Eisentraut wrote:
> We have some existing inline functions in .c files.  These can be
> more complicated, so it might be ok if the compiler decides to
> leave them out-of-line.  And they are never unreferenced, so
> suppression of unused-function warnings is not necessary and
> perhaps not wanted.  To leave these functions undisturbed, my patch
> doesn't redefine the "inline" keyword; instead it adds a new #define
> PG_INLINE for use in header files where unused-function warnings
> need to be suppressed.

One principle that I suppose should have been made more explicit is that
-- in my mind -- we should avoid littering our code with nonstandard
constructs in place of standard constructs.  Because the next generation
of developers won't know what PG_INLINE is and why we're not using plain
inline, even if we document it somewhere.

That said, ...

> > Is there not a setting to disable this particular warning.  I read that
> > MSVC has various ways to set that sort of thing.
> 
> Yes, warnings can be turned off by a #pragma specifying the
> warning number.

> Or compiler switches could be set to disable all such warnings
> globally.  Warning 4514 is specific to inline functions; so
> maybe it would be alright to keep it turned off globally.

... I think that would exactly be the right solution.

Then just replace in those two locations __GNUC__ by __GNUC__ ||
__MSVC__ (or whatever the symbol is).  Or if you want to make it extra
nice, create a symbol somewhere like in c.h that reads

#define USE_INLINE __GNUC__ || __MSVC__

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2010-01-19T16:43:10Z

Peter Eisentraut <peter_e@gmx.net> writes:
> On tis, 2010-01-19 at 01:29 -0800, Kurt Harriman wrote:
>> Or compiler switches could be set to disable all such warnings
>> globally.  Warning 4514 is specific to inline functions; so
>> maybe it would be alright to keep it turned off globally.

> ... I think that would exactly be the right solution.

I agree that that is a better/safer approach than using __forceinline.

> Then just replace in those two locations __GNUC__ by __GNUC__ ||
> __MSVC__ (or whatever the symbol is).  Or if you want to make it extra
> nice, create a symbol somewhere like in c.h that reads

> #define USE_INLINE __GNUC__ || __MSVC__

Kurt's patch proposes to try to define USE_INLINE via a configure test
rather than hard-coding it like that.  While I'm not entirely convinced
that the configure test will work, I like hard-coding it even less.
Let's try the configure test and see what happens.

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-01-19T21:55:30Z

On 1/19/2010 8:43 AM, Tom Lane wrote:
> Peter Eisentraut<peter_e@gmx.net>  writes:
>> On tis, 2010-01-19 at 01:29 -0800, Kurt Harriman wrote:
>>> Or compiler switches could be set to disable all such warnings
>>> globally.  Warning 4514 is specific to inline functions; so
>>> maybe it would be alright to keep it turned off globally.
>
>> ... I think that would exactly be the right solution.
>
> I agree that that is a better/safer approach than using __forceinline.
>
>> Then just replace in those two locations __GNUC__ by __GNUC__ ||
>> __MSVC__ (or whatever the symbol is).  Or if you want to make it extra
>> nice, create a symbol somewhere like in c.h that reads
>
>> #define USE_INLINE __GNUC__ || __MSVC__
>
> Kurt's patch proposes to try to define USE_INLINE via a configure test
> rather than hard-coding it like that.  While I'm not entirely convinced
> that the configure test will work, I like hard-coding it even less.
> Let's try the configure test and see what happens.
>
> 			regards, tom lane

I'll submit an updated patch.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Robert Haas <robertmhaas@gmail.com> — 2010-01-27T20:12:28Z

On Tue, Jan 19, 2010 at 4:55 PM, Kurt Harriman <harriman@acm.org> wrote:
> I'll submit an updated patch.

We need this patch pretty soon if we're going to include this in 9.0, I think.

...Robert

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-02-10T08:43:18Z

Revised patch is attached (3rd edition).
It's also available in my git repository in the "submitted" branch:
  http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted

With this patch, the "configure" script tests whether a static
inline function can be defined without incurring a warning when
not referenced.  If successful, the preprocessor symbol
inline_quietly is defined in pg_config.h to the appropriate
keyword: inline, __inline, or __inline__.
Otherwise inline_quietly remains undefined.

palloc.h and pg_list.h condition their inline function
definitions on inline_quietly instead of the gcc-specific __GNUC__.
Thus the functions can be inlined on more platforms, not only gcc.

Ordinary out-of-line calls are still used if the compiler doesn't
recognize inline functions, or spews warnings when static inline
functions are defined but not referenced.

Changes since the previous edition of this patch:

- Renamed the new preprocessor symbol to "inline_quietly" instead
   of PG_INLINE.  inline_quietly is more descriptive, and shows up
   when grepping for "inline".

- Removed MSVC-related changes (__forceinline) from the autoconf
   stuff.  Instead, updated the manually-edited pg_config.h.win32
   file to define both "inline" and "inline_quietly" as __inline.

- Removed Windows-only misspelling of __inline__ in instr_time.h.
   This was the only occurrence of __inline__; therefore, deleted
   the no-longer-needed definition of __inline__ from port/win32.h.
   Also deleted the definition of inline from port/win32.h, since
   it is now defined in pg_config.h.win32, consistent with the
   other platforms.

Thanks to all who commented.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-02-10T08:53:48Z

On 1/18/2010 4:42 PM, Tom Lane wrote:
> I think including MSVC in the set of compilers targeted by a configure
> test is just a waste of code.  It's more likely to confuse people than
> help them.

Ok, I have taken that out, and instead put the right stuff for MSVC into
pg_config.h.win32.

Regards, ... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-02-10T09:00:52Z

On 1/18/2010 11:48 PM, Peter Eisentraut wrote:
> On mån, 2010-01-18 at 16:34 -0800, Kurt Harriman wrote:
>> MSVC does warn about unused static inline functions.  The warning
>> is prevented by using __forceinline instead of __inline.
>
> Hmm, but forceinline is not the same as inline.  Are we confident that
> forcing inlining is not going to lead to disadvantages?  Has this been
> measured?
>
> Is there not a setting to disable this particular warning.  I read that
> MSVC has various ways to set that sort of thing.

On further investigation, plain __inline works alright, at least in
Visual Studio 2005.  The warning for unused static inline functions
only comes up when compiling C++, not C.  I've submitted a revised
patch, no longer using __forceinline.

Regards, ... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-02-10T09:26:56Z

On 1/19/2010 8:01 AM, Peter Eisentraut wrote:
> On tis, 2010-01-19 at 01:29 -0800, Kurt Harriman wrote:
>> On 1/18/2010 11:48 PM, Peter Eisentraut wrote:
>> We have some existing inline functions in .c files.  These can be
>> more complicated, so it might be ok if the compiler decides to
>> leave them out-of-line.  And they are never unreferenced, so
>> suppression of unused-function warnings is not necessary and
>> perhaps not wanted.  To leave these functions undisturbed, my patch
>> doesn't redefine the "inline" keyword; instead it adds a new #define
>> PG_INLINE for use in header files where unused-function warnings
>> need to be suppressed.
>
> One principle that I suppose should have been made more explicit is that
> -- in my mind -- we should avoid littering our code with nonstandard
> constructs in place of standard constructs.  Because the next generation
> of developers won't know what PG_INLINE is and why we're not using plain
> inline, even if we document it somewhere.

Unfortunately, to switch to an out-of-line function where inlining is
not supported, a separate preprocessor symbol is needed.  The already
existing "inline" define can't be used to test whether the compiler
supports inlining.  "inline" is defined as empty if configure doesn't
detect an acceptable variant of "inline".  It is left undefined if
the compiler accepts the standard spelling "inline".  But the C
preprocessor offers no way to test whether a symbol is defined as
empty.  #if can compare integers but not strings.

Everyone seems to hate the name PG_INLINE, so I've changed it to
inline_quietly, which is more descriptive.  Anyone who greps for
the definition of inline_quietly will find the comment in pg_config.h
telling what it is and how it should be used, as well as the examples
in pg_list.h and palloc.h.  Also it is explained, I hope clearly, in
the proposed CVS commit comment and in this email thread.

> Then just replace in those two locations __GNUC__ by __GNUC__ ||
> __MSVC__ (or whatever the symbol is).  Or if you want to make it extra
> nice, create a symbol somewhere like in c.h that reads
>
> #define USE_INLINE __GNUC__ || __MSVC__

That would just add to the compiler-specific preprocessor logic
to be duplicated in every header file in which inline functions
are defined.  I'm trying to factor out that compiler dependency
into a central location: pg_config.h.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-02-10T09:43:47Z

On 12/16/2009 8:40 AM, Tom Lane wrote:
> Alvaro Herrera<alvherre@commandprompt.com>  writes:
>> IIRC Kurt was also on about getting rid of some ugly macros that could
>> instead be coded as inline functions (fastgetattr for example)
>
> I'd just bounce that as useless activity.  If they are macros now,
> and work, the only possible effects of changing them are negative.

fastgetattr has just been changed by Robert Haas on 10 Jan 2010:
"Remove partial, broken support for NULL pointers when fetching attributes."

Changing fastgetattr to an inline function would make it

- easier to read, modify, and review for correctness

- debuggable: could set breakpoints, single-step, display the arguments

- profilable

and would make compiler warnings appear at the definition
rather than at every invocation.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-02-10T10:17:32Z

On 12/16/2009 8:21 AM, Tom Lane wrote:
> I would only suggest that the cleanest coding would be
>
> 	#ifdef USE_INLINE
>
> 	static inline foo(...) ...
>
> 	#else
>
> 	... non-inline definition of foo
>
> 	#endif
>
> ie, go ahead and rely on autoconf's definition (if any) of "inline"
> and add a policy symbol USE_INLINE to determine whether to use it.

That would work for gcc and MSVC.  But it wouldn't allow for
configuring an alternative keyword (like __forceinline) or added
magic (like inserting an __attribute__ or __declspec) to silence
warnings for some compiler which we don't know about yet.

> The proposed PG_INLINE coding conflates the symbol needed in the code
> with the policy choice.

Everyone is familiar with this idiom: first test whether a pointer
is NULL, before dereferencing it.  We don't use a separate flag to
say whether the pointer is NULL.

> Another possibility would be to call the policy symbol HAVE_INLINE,
> but that (a) risks collision with a name defined by autoconf built-in
> macros, and (b) looks like it merely indicates whether the compiler
> *has* inline, not that we have made a choice about how to use it.

In the new 3rd edition of the patch, I've changed the name to
inline_quietly.  If not too many people hate this new name, I
can undertake a new career naming tablets for Apple. :)

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-10T15:12:21Z

Kurt Harriman <harriman@acm.org> writes:
> On 1/19/2010 8:01 AM, Peter Eisentraut wrote:
>> One principle that I suppose should have been made more explicit is that
>> -- in my mind -- we should avoid littering our code with nonstandard
>> constructs in place of standard constructs.

> Everyone seems to hate the name PG_INLINE, so I've changed it to
> inline_quietly, which is more descriptive.

Kurt, you seem to be more or less impervious to advice :-(.

Please just make the thing be "inline" and have configure define
USE_INLINE, as per previous discussion.  Cluttering the code with
nonstandard constructs is not good for readability.  More, it is likely
to confuse syntax-aware editors and pgindent, neither of which will read
any of the discussion or commit messages.

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-02-10T22:53:49Z

Revised patch is attached (4th edition).
It's also available in my git repository in the "submitted" branch:
  http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted

With this patch, the "configure" script tests whether a static
inline function can be defined without incurring a warning when
not referenced.  If successful, the preprocessor symbol USE_INLINE
is defined to 1 in pg_config.h.  Otherwise USE_INLINE remains
undefined.

palloc.h and pg_list.h condition their inline function
definitions on USE_INLINE instead of the gcc-specific __GNUC__.
Thus the functions can be inlined on more platforms, not only gcc.

Ordinary out-of-line calls are still used if the compiler doesn't
recognize inline functions, or spews warnings when static inline
functions are defined but not referenced.

 From now on in cross-platform code, plain "inline" should be used
instead of compiler-specific alternate spellings such as __inline__,
because the configure script redefines "inline" to the appropriate
alternate spelling if necessary.  (This patch doesn't affect the
redefinition of "inline": that was already in place.)

Changes since the second and third editions of this patch:

- Renamed the new preprocessor symbol to USE_INLINE, following the
   advice of Tom Lane, instead of my earlier attempts PG_INLINE or
   inline_quietly.  It is used like this:

       /* include/utils/palloc.h */
       #ifdef USE_INLINE
       static inline MemoryContext
       MemoryContextSwitchTo(MemoryContext context)
       {
       	MemoryContext old = CurrentMemoryContext;
       	CurrentMemoryContext = context;
       	return old;
       }
       #else
       extern MemoryContext MemoryContextSwitchTo(MemoryContext context);
       #endif   /* USE_INLINE */

       /* backend/utils/mmgr/mcxt.c */
       #ifndef USE_INLINE
       MemoryContext
       MemoryContextSwitchTo(MemoryContext context)
       {
       	MemoryContext old;
       	AssertArg(MemoryContextIsValid(context));
       	old = CurrentMemoryContext;
       	CurrentMemoryContext = context;
       	return old;
       }
       #endif   /* ! USE_INLINE */

- Simplified the autoconf logic added to config/c-compiler.m4.

- Removed MSVC-related changes (__forceinline) from the autoconf
   stuff.  Instead, updated the manually-edited pg_config.h.win32
   file to define "inline" as __inline, and USE_INLINE as 1.

- Removed Windows-only misspelling of __inline__ in instr_time.h.
   This was the only occurrence of __inline__; therefore, deleted
   the no-longer-needed definition of __inline__ from port/win32.h.
   Also deleted the definition of inline from port/win32.h, since
   it is now defined in pg_config.h.win32, consistent with the
   other platforms.

Thanks again to all who have commented.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> — 2010-02-10T23:04:06Z

On 2/10/2010 7:12 AM, Tom Lane wrote:
> Kurt, you seem to be more or less impervious to advice :-(.

Thank you for reviewing my patch.  It is a rare honor to
have my personal qualities reviewed here as well.

Since this forum is archived for posterity, I suppose I
must point out that I have in fact been responsive to all
the advice that has been offered here.  I have answered
each comment fully and politely.  I have acted upon most
of the suggestions, and have revised my small patch
accordingly and resubmitted it twice (now thrice,
incorporating this comment of yours).

Admittedly I have used my own judgment in how to adopt these
sometimes conflicting suggestions; and have explained the
rationale for my choices.  Everyone may judge whether my
choices and explanations are satisfactory and continue the
dialogue if they are not yet satisfied.  By sincerely
engaging in such a process, consensus may be reached.

All contributors to the pg_hackers list are well advised to
be impervious to brusque and sometimes rude dismissals, which
seem to be de rigueur here.  However, the evidence of this
thread shows that I have been far from impervious to advice.

By the way, suggestions which must be carried out without
question are "orders", not "advice".  When a statement,
meant to be imperative, is phrased softly as advice, it can
easily be mistaken as optional by newcomers who may not have
fully grasped the prevailing reality.  Thus, commands are
best stated in clear language.

> Please just make the thing be "inline" and have configure define
> USE_INLINE, as per previous discussion.

Just now I have resubmitted according to your instruction.

> Cluttering the code with nonstandard constructs is not
 > good for readability.

Agreed.  But any program consists of definitions of new
identifiers, data structures, macros, and conventions or
guidelines for their use.  What, or who, differentiates
ordinary programming practice, such as typedefs, from
"nonstandard constructs"?

> More, it is likely to confuse syntax-aware editors and
 > pgindent, neither of which will read any of the discussion
 > or commit messages.

Good point.  This had not been mentioned before.  It works
alright with the syntax-aware editors that I use, and I
haven't had occasion to run pgindent, so I didn't think of
this earlier.

Does the same problem exist with the PGDLLIMPORT macro
defined in postgres.h?  It, too, is used in the same
syntactic niche where "inline" would be placed.

Regards,
... kurt

Re: Patch: Remove gcc dependency in definition of inline functions

Robert Haas <robertmhaas@gmail.com> — 2010-02-12T04:42:20Z

On Wed, Feb 10, 2010 at 5:53 PM, Kurt Harriman <harriman@acm.org> wrote:
> Revised patch is attached (4th edition).

This looks generally sane to me, though it seems entirely imaginable
that it might break something, somewhere, for somebody... in which
case I suppose we'll adjust as needed.

Peter, are you planning to commit this one?

...Robert

Re: Patch: Remove gcc dependency in definition of inline functions

Robert Haas <robertmhaas@gmail.com> — 2010-02-12T05:17:51Z

On Wed, Feb 10, 2010 at 6:04 PM, Kurt Harriman <harriman@acm.org> wrote:
> By the way, suggestions which must be carried out without
> question are "orders", not "advice".  When a statement,
> meant to be imperative, is phrased softly as advice, it can
> easily be mistaken as optional by newcomers who may not have
> fully grasped the prevailing reality.  Thus, commands are
> best stated in clear language.

I think the reason people tend to phrase things in terms of opinions
or advice is because no single person here is able to speak with
complete authority, and those who attempt to do so tend to draw
irritated responses when a softer statement would have passed
unchallenged.  At the same time, when two or three people all express
an opinion that we should do X, and especially when some of those
people are committers, it's very difficult to get a patch committed
that does not-X.  Maybe this isn't as obvious to newcomers as it could
be, although it's not entirely clear to me how we could make it so
without being heavy-handed about it...   it's not that you can never
win an argument of this type; it's just that you need a darn good
reason and some allies, and it's hard to come up with darn good
reasons when the discussion is basically about style.

Having said all that, I sympathize with your frustration; I've been
there a few times myself.

...Robert

Re: Patch: Remove gcc dependency in definition of inline functions

Tom Lane <tgl@sss.pgh.pa.us> — 2010-02-13T02:35:29Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Feb 10, 2010 at 5:53 PM, Kurt Harriman <harriman@acm.org> wrote:
>> Revised patch is attached (4th edition).

> This looks generally sane to me, though it seems entirely imaginable
> that it might break something, somewhere, for somebody... in which
> case I suppose we'll adjust as needed.

Looks sane to me too -- committed.  We'll soon see what the buildfarm
thinks (though the number of non-gcc buildfarm members is depressingly
small).

			regards, tom lane

Re: Patch: Remove gcc dependency in definition of inline functions

Robert Haas <robertmhaas@gmail.com> — 2010-02-13T02:36:44Z

On Fri, Feb 12, 2010 at 9:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Feb 10, 2010 at 5:53 PM, Kurt Harriman <harriman@acm.org> wrote:
>>> Revised patch is attached (4th edition).
>
>> This looks generally sane to me, though it seems entirely imaginable
>> that it might break something, somewhere, for somebody... in which
>> case I suppose we'll adjust as needed.
>
> Looks sane to me too -- committed.  We'll soon see what the buildfarm
> thinks (though the number of non-gcc buildfarm members is depressingly
> small).

I can't feel bad about the near-ubiquity of gcc...  life would be a
lot harder if it were otherwise.

...Robert