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

On 12/15/2009 4:05 AM, Marko Kreen wrote:
> On 12/15/09, Kurt Harriman<harriman(at)acm(dot)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