Prev: [HACKERS] Git out of sync vs. CVS
Next: Git out of sync vs. CVS
From: Kurt Harriman on 19 Jan 2010 03:24
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

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

From: Kurt Harriman on 19 Jan 2010 04:29
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

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

From: Peter Eisentraut on 19 Jan 2010 11:01
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__


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

From: Kurt Harriman on 19 Jan 2010 16:55
On 1/19/2010 8:43 AM, Tom Lane wrote:
> Peter Eisentraut<peter_e(a)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


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

From: Kurt Harriman on 10 Feb 2010 03:43
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
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: [HACKERS] Git out of sync vs. CVS
Next: Git out of sync vs. CVS