From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tom Rini <trini@konsulko.com>
Date: Wed, 13 May 2020 17:52:03 -0400
Subject: [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
In-Reply-To: <20200513184151.GJ5091@bill-the-cat>
References: <20200509151242.68082-1-xypron.glpk@gmx.de>
 <CAK7LNAQ6TDt5314aNo4p1ptAjggtEvaDz0ZFndEZ=OkVP_ec4A@mail.gmail.com>
 <20200511184045.GP12564@bill-the-cat>
 <87e8b987-dcd6-b2ae-96d4-6e3cfbb76662@gmx.de>
 <20200513030438.GE5091@bill-the-cat>
 <20200513144200.GF5091@bill-the-cat>
 <CAK7LNAQB25DGArttz510b94=_MEV89kCAr0+hv6CNH-axRFoKQ@mail.gmail.com>
 <20200513161303.GI5091@bill-the-cat>
 <CAK7LNATeOepwRJbdSsZOY43s7vDPR+dwbQr5JoNGf3_+mcJ-rg@mail.gmail.com>
 <20200513184151.GJ5091@bill-the-cat>
Message-ID: <20200513215203.GK5091@bill-the-cat>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Wed, May 13, 2020 at 02:41:51PM -0400, Tom Rini wrote:
> On Thu, May 14, 2020 at 02:27:20AM +0900, Masahiro Yamada wrote:
> > On Thu, May 14, 2020 at 1:13 AM Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, May 14, 2020 at 01:05:37AM +0900, Masahiro Yamada wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, May 13, 2020 at 11:42 PM Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, May 12, 2020 at 11:04:38PM -0400, Tom Rini wrote:
> > > > > > On Mon, May 11, 2020 at 09:08:03PM +0200, Heinrich Schuchardt wrote:
> > > > > > > On 5/11/20 8:40 PM, Tom Rini wrote:
> > > > > > > > On Sun, May 10, 2020 at 10:12:07PM +0900, Masahiro Yamada wrote:
> > > > > > > >> On Sun, May 10, 2020 at 12:12 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > >>>
> > > > > > > >>> GCC recognizes /* fallthrough */ if -Wimplicit-fallthrough=3 is enabled.
> > > > > > > >>
> > > > > > > >> FYI.
> > > > > > > >>
> > > > > > > >> Linux decided to not use /* fallthrough */ any more
> > > > > > > >> because Clang does not recognize it.
> > > > > > > >>
> > > > > > > >> __attribute__((__fallthrough__)) is supported
> > > > > > > >> by both Clang and recent GCC.
> > > > > > > In fact Linux has a define:
> > > > > > >
> > > > > > > include/linux/compiler_attributes.h:200:# define fallthrough
> > > > > > >         __attribute__((__fallthrough__))
> > > > > > >
> > > > > > > And in the code you would use
> > > > > > >
> > > > > > >     case foo:
> > > > > > >             fallthrough;
> > > > > > >     case bar:
> > > > > > >
> > > > > > > But the Linux kernel still has a lot of lines with
> > > > > > >
> > > > > > > /* fallthrough */
> > > > > > >
> > > > > > > Documentation/process/deprecated.rst:
> > > > > > >
> > > > > > > <cite>
> > > > > > > As there have been a long list of flaws `due to missing "break"
> > > > > > > statements <https://cwe.mitre.org/data/definitions/484.html>`_, we no
> > > > > > > longer allow implicit fall-through. In order to identify intentional
> > > > > > > fall-through cases, we have adopted a pseudo-keyword macro "fallthrough"
> > > > > > > which expands to gcc's extension `__attribute__((__fallthrough__))
> > > > > > > <https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html>`_. (When
> > > > > > > the C17/C18  `[[fallthrough]]` syntax is more commonly supported by C
> > > > > > > compilers, static analyzers, and IDEs, we can switch to using that
> > > > > > > syntax for the macro pseudo-keyword.)
> > > > > > > </cite>
> > > > > > >
> > > > > > > Using the attribute is not standard C and not any better than using the
> > > > > > > comment. The real target is the C17 syntax.
> > > > > > >
> > > > > > > >>
> > > > > > > >>
> > > > > > > >> Linux is now doing treewide conversion
> > > > > > > >> from /* fallthrough */ to 'fallthrough;'.
> > > > > > > >>
> > > > > > > >> See include/linux/compiler_attributes.h in Linux.
> > > > > > > >>
> > > > > > > >> I do not know if U-Boot wants to align with it.
> > > > > > > >> (up to Tom ?)
> > > > > > > >
> > > > > > > > A re-sync on the compiler headers again and making use of this sounds
> > > > > > > > like a good idea, yes.
> > > > > > > >
> > > > > > >
> > > > > > > We should enable -Wimplicit-fallthrough like the kernel does. This
> > > > > > > defaults to -Wimplicit-fallthrough=3 and is happy with both the comment
> > > > > > > as well as with the attribute.
> > > > > > >
> > > > > > > @Tom:
> > > > > > > Will you update the compiler headers within this release cycle?
> > > > > > > Otherwise we should take the patch as is to get us closer to the
> > > > > > > -Wimplicit-fallthrough target.
> > > > > >
> > > > > > I'm not going to update it for this release cycle.  I've done the
> > > > > > initial import and build and there's some fairly large changes related
> > > > > > to inlining that I want to look at harder to see if we can/should do
> > > > > > something about (I don't want to derail this thread, I'll start
> > > > > > another).  But it's very far from zero size change and given the inline
> > > > > > changes I think it'll need real testing.
> > > > > >
> > > > > > And since the kernel isn't making a huge use yet of fallthrough; we can
> > > > > > afford to look a little harder at things.
> > > > >
> > > > > I think I've figured out the inline issue which is that we need
> > > > > scripts/Kconfig.include from the kernel, CC_HAS_ASM_INLINE Kconfig
> > > > > option, and re-sync with Kconfiglib, but that's still going to be enough
> > > > > stuff that I don't think it's good to pull in at -rc2.
> > > > >
> > > >
> > > >
> > > > I do not get how 'asm inline' support is related
> > > > to this topic.
> > > >
> > > > GCC 9 started to support 'asm inline' for the better inlining heuristic.
> > > > The kernel uses a bunch of inline assembly
> > > > that is not as expensive as it looks.
> > > >
> > > > As GCC is agnostic about the real cost of inline assembly,
> > > > 'asm inline' is a good hint if people know the real cost is quite small.
> > > > Then, GCC will be able to inline more functions.
> > > >
> > > > I do not know how important it is for U-Boot, though.
> > > >
> > > > What is causing you a trouble?
> > >
> > > So, it turns out that while we do want to grab the changes so that we
> > > can have CC_HAS_ASM_INLINE via Kconfig, it's not "it".  What I see for
> > > virtually every board (with gcc-9.3 from kernel.org) is something like:
> > >             rock960-rk3399 : all -8 rodata -4 spl/u-boot-spl:all +992 spl/u-boot-spl:text +992 text -4
> > >                u-boot: add: 67/-9, grow: 74/-92 bytes: 5072/-4928 (144)
> > >                  function                                   old     new   delta
> > >                  static._compare_and_overwrite_entry          -     348    +348
> > >                  menu_interactive_choice                      -     288    +288
> > >                  hex2bin                                      -     200    +200
> > >                  __fswab64                                    -     176    +176
> > >                  __fswab32                                    -     144    +144
> > >                  sdhci_reset                                  -     136    +136
> > >                  dwmci_fifo_ready                             -     124    +124
> > >                  fdt_offset_ptr_                              -     120    +120
> > >                  menu_items_iter                              -     108    +108
> > >                  generic_fls                                  -     100    +100
> > >                  fdt_set_totalsize                            -      96     +96
> > >                  static.generic_fls                           -      84     +84
> > 
> > 
> > OK, these functions previously disappeared because all
> > of the function call-sites were inlined.
> > 
> > If you resync <linux/compier*.h> with latest Linux,
> > they are not necessarily inlined.
> > 
> > 
> > 
> > 
> > In current U-Boot, 'static inline' is actually replaced with
> > __attribute__((always_inline)).
> > So, inlining is forcible.
> > 
> > See the code.
> > 
> > 
> > include/linux/compiler-gcc.h
> > 
> > #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) ||                \
> >     !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4)
> > #define inline          inline          __attribute__((always_inline)) notrace
> > #define __inline__      __inline__      __attribute__((always_inline)) notrace
> > #define __inline        __inline        __attribute__((always_inline)) notrace
> > 
> > 
> > 
> > 
> > In Linux, the following commits stopped doing that.
> > (both my commits)
> > 
> > ac7c3e4ff401b304489a031938dbeaab585bfe0a
> > 889b3c1245de48ed0cacf7aebb25c489d3e4a3e9
> > 
> > 
> > Now, 'inline' is just a compiler hint.
> > The compiler does the best judge
> > whether to inline the function or not.
> 
> Ah, OK.  And the kernel is uninterested in bringing back forcing
> inlineing for size reasons as it's gone for LTO instead of
> ffunction-sections/fdata-sections and discarding sections (and we're in
> turn a ways off from that as we need to move to gcc linking) I assume.
> Thanks!

Digging more still, it's not a universal choice.  On full U-Boot, it's
around 90% a size win to let the compiler make the inline choice.  On
SPL it's more like 50%.  I think I'm leaning towards making it a choice
especially until we can figure out what would lead to better SPL results
for everyone.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200513/5b525e4c/attachment.sig>