Re: [PATCH] don't declare header-defined functions both static and inline

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Jan 31, 2023 at 9:43 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Jan 31, 2023 at 08:05:15AM +0100, Richard Biener via Gcc-patches wrote:
> > On Tue, Jan 31, 2023 at 4:39 AM Patrick Palka via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > Many functions defined in our headers are declared 'static inline' which
> > > is a vestige from when GCC's implementation language was C.  But in C++
> > > the inline keyword is more than just a compiler hint, and is sufficient
> > > to give the function the intended semantics.  In fact declaring a
> > > (namespace-scope) function both static and inline is a pessimization
> > > since static effectively disables the intended definition merging
> > > behavior that inline provides, and is also a source of (harmless) ODR
> > > violations when a static inline function gets called from a non-static
> > > inline one (such as tree_operand_length being called from
> > > tree_operand_check).
> > >
> > > This patch mechanically fixes the vast majority of occurrences of this
> > > anti-pattern throughout the compiler's headers via the command line
> > >
> > >   echo gcc/*.h gcc/*/*.h | xargs sed -i 's/^static inline/inline/g'
> > >
> > > Besides fixing the ODR violations, this speeds up stage1 cc1plus by
> > > about 2% and reduces the size of its text segment by 1.5MB.
> > >
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, would this be OK to
> > > push now or wait for stage1?
> >
> > Speeding up and reducing the size of cc1plus improves on continued
> > regression in this area.  So I'd vote +1 to do this now, let's see what others
> > think.
>
> I lean towards +1 too, but
> 1) we should make sure we don't do that for installed headers with the
>    exception of headers meant for GCC plugins; in quick skimming I don't
>    see ginclude/ headers among the changed ones, but perhaps doing all
>    languages make install, then applying the patch, make and make install
>    again into another tree and compare all the headers in those tree
>    with the exception of paths with /plugin/include/ in it?
> 2) we should make sure we don't introduce ODR violations through this, if
>    say 2 headers would define different static inline functions with the
>    same name (or even one static inline and another without that).
>    Don't know what would be best to catch that, get from the patch
>    list of changed functions and then search for it in readelf -wi output
>    using some script, or would LTO bootstrap detect that, or libabigail?
>    What I'm worried about is 2 different headers defining the same
>    function perhaps with different arguments/return values or content and
>    that we happen to never include the two headers in the same TU

Ah, didn't think of that - yes, I suppose it might be that LTO bootstrap
would warn about those?

> 3) we have also gcc/ada/gcc-interface/*.h with
> ada.h:#define INLINE static inline
> gigi.h:static inline unsigned HOST_WIDE_INT
> gigi.h:static inline bool
> gigi.h:static inline bool
> gigi.h:static inline bool
> gigi.h:static inline tree
> gigi.h:static inline tree
> gigi.h:static inline tree
> gigi.h:static inline tree
> gigi.h:static inline tree
> gigi.h:static inline tree
> gigi.h:static inline tree
> gigi.h:static inline tree
> gigi.h:static inline tree
> gigi.h:static inline tree
>    I think we can defer that to Ada maintaners but we should tell them
>    about it
> 4) there are some static inline also in
>    gcc/config/*/*.h (and some in gcc/common/*/*.h - though in that case
>    it is solely in installed header that shouldn't be changed)
> avr/avr-protos.h:static inline unsigned
> pru/pru-protos.h:static inline bool
> rs6000/rs6000-internal.h:static inline bool
> rs6000/rs6000-protos.h:static inline bool
> s390/s390-builtins.h:static inline unsigned int
> s390/s390-builtins.h:static inline unsigned int
> s390/vecintrin.h:static inline int
>    The last one is an installed header, so I think we shouldn't touch
>    it, the rest should be considered.

The above probably asks to split the patch up and separate gcc/*/*.h
into individual patches?

Richard.

>
>         Jakub
>