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

[Jakub Jelinek <jakub@redhat.com>]

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
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.

	Jakub