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.
-- Best Regards Masahiro Yamada