| Message ID | 20200509151242.68082-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Commit | b606a6cfa6d3d606f4cdbd1585f74cec409af661 |
| Headers | show |
| Series | [1/1] tools: ftdgrep: use /* fallthrough */ as needed | expand |
On Sun, May 10, 2020 at 12:12 AM Heinrich Schuchardt <xypron.glpk at 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. 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 ?) > Let's use it consistently. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de> > --- > tools/fdtgrep.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c > index 7e168a1e6b..8b4d2765ad 100644 > --- a/tools/fdtgrep.c > +++ b/tools/fdtgrep.c > @@ -923,7 +923,9 @@ static const char usage_synopsis[] = > /* Helper for getopt case statements */ > #define case_USAGE_COMMON_FLAGS \ > case 'h': usage(NULL); \ > + /* fallthrough */ \ > case 'V': util_version(); \ > + /* fallthrough */ \ > case '?': usage("unknown option"); > > static const char usage_short_opts[] = > @@ -1085,6 +1087,7 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) > > switch (opt) { > case_USAGE_COMMON_FLAGS > + /* fallthrough */ > case 'a': > disp->show_addr = 1; > break; > @@ -1096,7 +1099,7 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) > break; > case 'C': > inc = 0; > - /* no break */ > + /* fallthrough */ > case 'c': > type = FDT_IS_COMPAT; > break; > @@ -1111,7 +1114,7 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) > break; > case 'G': > inc = 0; > - /* no break */ > + /* fallthrough */ > case 'g': > type = FDT_ANY_GLOBAL; > break; > @@ -1129,7 +1132,7 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) > break; > case 'N': > inc = 0; > - /* no break */ > + /* fallthrough */ > case 'n': > type = FDT_IS_NODE; > break; > @@ -1148,7 +1151,7 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) > break; > case 'P': > inc = 0; > - /* no break */ > + /* fallthrough */ > case 'p': > type = FDT_IS_PROP; > break; > -- > 2.26.2 >
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 at 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. > > > 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.
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 at 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. Best regards Heinrich
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 at 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.
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 at 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.
Hi Tom, On Wed, May 13, 2020 at 11:42 PM Tom Rini <trini at 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 at 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? -- Best Regards Masahiro Yamada
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 at 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 at 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 clk_get_by_indexed_prop - 80 +80 fdt_read_number - 76 +76 do_fdt 3984 4060 +76 usb_kbd_poll_for_event - 72 +72 rpc_lookup_req - 72 +72 menu_item_key_match - 72 +72 bin2hex - 68 +68 asix_get_phy_addr - 68 +68 fdt_setprop_u64 - 64 +64 fdt_set_size_dt_strings - 64 +64 fdt_set_off_dt_strings - 64 +64 zalloc - 60 +60 static.strlcat - 60 +60 dwmci_wait_reset - 60 +60 menu_item_print - 56 +56 is_zero_ethaddr - 56 +56 asix_eth_start 228 284 +56 set_sctlr - 52 +52 menu_item_destroy - 52 +52 get_sctlr - 48 +48 fdt_setprop_u32 - 48 +48 is_valid_ethaddr - 44 +44 is_hex_prefix - 44 +44 fdt_data_size_ - 44 +44 list_del - 40 +40 fdt_mem_rsv_ - 40 +40 dev_read_u32_default - 40 +40 __tolower - 40 +40 le32_to_int - 36 +36 fdt_open_into 392 428 +36 _debug_uart_putc - 36 +36 usb_gadget_disconnect - 32 +32 usb_gadget_connect - 32 +32 fdt_set_size_dt_struct - 32 +32 fdt_set_off_dt_struct - 32 +32 fdt_packblocks_ 176 208 +32 fdt_blocks_misordered_ 96 128 +32 __get_unaligned_le32 - 32 +32 fdtdec_get_number 48 76 +28 fdt_ro_probe_ 128 156 +28 env_flags_validate 632 660 +28 clk_valid - 28 +28 clk_set_rate 52 80 +28 clk_set_parent 52 80 +28 clk_get_rate 52 80 +28 clk_free 44 72 +28 clk_enable 52 80 +28 clk_disable 52 80 +28 bootstage_error - 28 +28 asix_set_sw_mii - 28 +28 asix_set_hw_mii - 28 +28 uuid_str_to_bin 396 420 +24 ofnode_read_u64 92 116 +24 fdt_mem_rsv 60 84 +24 fdt_get_property_namelen 44 68 +24 static.usb_ep_queue - 20 +20 static.image_get_size - 20 +20 is_extended - 20 +20 flush_dcache_all - 20 +20 fdt_splice_mem_rsv_ 96 116 +20 fdt_offset_ptr 104 124 +20 fdt_check_header 276 296 +20 eth_validate_ethaddr_str 152 172 +20 android_image_get_kcomp 84 104 +20 usb_ep_free_request - 16 +16 static.image_get_magic - 16 +16 static.image_get_load - 16 +16 nfs_read_req 228 244 +16 malloc_cache_aligned - 16 +16 image_print_contents 448 464 +16 fit_get_end 16 32 +16 fdt_add_property_ 388 404 +16 dwc3_flush_cache - 16 +16 do_bootm_states 2376 2392 +16 dev_read_bool - 16 +16 static.image_get_ep - 12 +12 static.dev_read_u32_default - 12 +12 nfs_readlink_reply 336 348 +12 nfs_read_reply 404 416 +12 fit_image_get_address 132 144 +12 fdtdec_parse_phandle_with_args 336 348 +12 fdt_shrink_to_minimum 220 232 +12 fdt_header_size 12 24 +12 fdt_del_node 96 108 +12 fdt_add_mem_rsv 124 136 +12 boot_get_fdt 1028 1040 +12 android_image_get_kernel 572 584 +12 update_package_list 280 288 +8 nfs_lookup_reply 292 300 +8 net_copy_u32 - 8 +8 net_copy_ip - 8 +8 image_multi_getimg 132 140 +8 image_check_dcrc 68 76 +8 guidcmp - 8 +8 genimg_get_format 92 100 +8 free_packagelist 68 76 +8 fit_image_load 1608 1616 +8 fdt_valid 204 212 +8 fdt_splice_struct_ 96 104 +8 fdt_rw_probe_ 108 116 +8 fdt_num_mem_rsv 60 68 +8 fdt_next_tag 256 264 +8 fdt_getprop_namelen 100 108 +8 dhcp_extended 616 624 +8 boot_get_ramdisk 964 972 +8 xhci_submit_control_msg 2732 2736 +4 static.image_get_hcrc - 4 +4 static.image_get_dcrc - 4 +4 static.__swab32p - 4 +4 sd_get_capabilities 432 436 +4 rpc_req 288 292 +4 rpc_lookup_reply 180 184 +4 print_data 444 448 +4 nfs_umountall_reply 136 140 +4 nfs_readlink_req 192 196 +4 nfs_mount_req 180 184 +4 nfs_mount_reply 148 152 +4 nfs_lookup_req 324 328 +4 image_setup_libfdt 284 288 +4 image_check_hcrc 80 84 +4 fdtdec_get_int_array 96 100 +4 fdt_setprop_placeholder 216 220 +4 fdt_getprop_by_offset 184 188 +4 fdt_get_string 288 292 +4 fdt_get_property_namelen_ 212 216 +4 fdt_get_property_by_offset_ 100 104 +4 fdt_get_phandle 120 124 +4 fdt_get_mem_rsv 108 112 +4 boot_relocate_fdt 424 428 +4 spi_slave_ofdata_to_platdata 432 428 -4 sdhci_send_command 1660 1656 -4 i2c_chip_ofdata_to_platdata 100 96 -4 file_fat_write_at 652 648 -4 fdt_get_name 164 160 -4 fat_size 204 200 -4 fat_opendir 172 168 -4 fat_exists 128 124 -4 efi_search_protocol 148 144 -4 efi_install_multiple_protocol_interfaces 552 548 -4 dwmci_send_cmd 1556 1552 -4 remove_strings_package 124 116 -8 fdt_splice_ 148 140 -8 fdt_pack_reg 216 208 -8 fdt_add_subnode_namelen 284 276 -8 fastboot_start_ep 124 116 -8 efi_signal_event 200 192 -8 efi_process_event_queue 144 136 -8 efi_install_fdt 868 860 -8 efi_install_configuration_table 456 448 -8 efi_disconnect_all_drivers 404 396 -8 clk_set_defaults 516 508 -8 ustrtoull 144 132 -12 ustrtoul 144 132 -12 rx_handler_dl_image 204 192 -12 rx_handler_command 368 356 -12 remove_keyboard_package 76 64 -12 regulator_common_ofdata_to_platdata 188 176 -12 read_bootsectandvi 308 296 -12 free_keyboard_layouts 80 68 -12 file_fat_read_at 864 852 -12 fat_mkdir 928 916 -12 fat_itr_root 444 432 -12 fastboot_tx_write_str 152 140 -12 fastboot_set_alt 316 304 -12 efi_uninstall_protocol_interface 112 100 -12 efi_uninstall_protocol 216 204 -12 efi_uninstall_multiple_protocol_interfaces 464 452 -12 efi_remove_protocol 100 88 -12 efi_locate_handle 380 368 -12 efi_exit_boot_services 336 324 -12 efi_delete_handle 60 48 -12 efi_close_protocol 220 208 -12 dwc3_prepare_trbs 772 760 -12 dwc3_gadget_uboot_handle_interrupt 1552 1540 -12 dwc3_gadget_giveback 240 228 -12 g_dnl_bind 376 360 -16 fastboot_disable 140 124 -16 ext4fs_iterate_dir 656 640 -16 efi_locate_protocol 280 264 -16 efi_add_protocol 320 304 -16 dhcp_process_options 512 496 -16 fat_unlink 668 648 -20 ext4fs_read_inode 392 372 -20 ext4fs_mount 236 216 -20 dhcp_handler 972 952 -20 _parse_integer_fixup_radix 248 228 -20 g_dnl_unbind 52 28 -24 fdt_initrd 436 412 -24 efi_hii_package_len 24 - -24 dcache_status 52 24 -28 usb_kbd_testc 144 112 -32 usb_kbd_getc 116 84 -32 ext4fs_find_file1 660 628 -32 asix_eth_probe 700 668 -32 dwc3_gadget_ep_enable 264 228 -36 printch 80 40 -40 eth_write_hwaddr 220 180 -40 efi_close_event 268 224 -44 asix_mdio_write 140 96 -44 add_packages 928 884 -44 of_bus_default_map 168 120 -48 menu_destroy 128 80 -48 of_bus_default_translate 184 132 -52 mmc_init 2724 2668 -56 static.dwmci_wait_reset 60 - -60 __of_translate_address 624 564 -60 remove_guid_package 64 - -64 menu_item_add 240 176 -64 menu_default_set 148 76 -72 icache_disable 100 24 -76 static.clk_get_by_indexed_prop 80 - -80 dcache_disable 132 48 -84 icache_enable 116 28 -88 nfs_send 260 168 -92 xhci_microframes_to_exponent 104 - -104 skip_num 104 - -104 spi_nor_scan 2196 2088 -108 part_get_info_extended 716 604 -112 static.dwmci_fifo_ready 124 - -124 static.sdhci_reset 136 - -136 print_partition_extended 648 512 -136 asix_mdio_read 140 - -140 static.parse_attr 468 308 -160 efi_set_variable_common 1440 1276 -164 efi_get_variable_common 548 384 -164 eth_post_probe 580 404 -176 dcache_enable 272 44 -228 read_allocated_block 2120 1832 -288 hsearch_r 1080 728 -352 menu_get_choice 480 84 -396 spl-u-boot-spl: add: 23/-5, grow: 25/-12 bytes: 1700/-768 (932) function old new delta sdhci_reset - 136 +136 __fswab64 - 132 +132 dwmci_fifo_ready - 124 +124 fdt_offset_ptr_ - 120 +120 __fswab32 - 104 +104 fdt_mem_rsv_ - 80 +80 clk_get_by_indexed_prop - 80 +80 fdt_set_size_dt_strings - 64 +64 fdt_set_off_dt_strings - 64 +64 dwmci_wait_reset - 60 +60 set_sctlr - 52 +52 get_sctlr - 48 +48 fdt_setprop_u32 - 48 +48 fdt_data_size_ - 44 +44 __tolower - 40 +40 _debug_uart_putc - 36 +36 fdt_set_size_dt_struct - 32 +32 fdt_set_off_dt_struct - 32 +32 fdtdec_get_number 48 76 +28 fdt_offset_ptr 52 80 +28 clk_valid - 28 +28 clk_set_rate 52 80 +28 clk_set_parent 52 80 +28 clk_get_rate 52 80 +28 ofnode_read_u64 92 116 +24 flush_dcache_all - 20 +20 fdt_splice_mem_rsv_ 96 116 +20 fdt_check_header 28 44 +16 dev_read_u32_default - 16 +16 dev_read_bool - 16 +16 fit_image_get_address 132 144 +12 fdtdec_parse_phandle_with_args 336 348 +12 fdt_shrink_to_minimum 220 232 +12 fdt_get_property_by_offset_ 36 48 +12 fdt_add_property_ 340 352 +12 fdt_splice_struct_ 96 104 +8 fdt_get_string 64 72 +8 fdt_add_mem_rsv 112 120 +8 static.__swab32p - 4 +4 sd_get_capabilities 432 436 +4 fdtdec_get_int_array 96 100 +4 fdt_setprop_placeholder 196 200 +4 fdt_num_mem_rsv 64 68 +4 fdt_next_tag 192 196 +4 fdt_getprop_by_offset 80 84 +4 fdt_get_property_namelen_ 200 204 +4 fdt_get_phandle 120 124 +4 fdt_get_mem_rsv 52 56 +4 sdhci_send_command 1660 1656 -4 fdt_del_mem_rsv 104 100 -4 dwmci_send_cmd 1556 1552 -4 fdt_splice_ 148 140 -8 fdt_add_subnode_namelen 260 252 -8 fdt_get_name 68 56 -12 clk_set_defaults 488 472 -16 fdt_mem_rsv 20 - -20 _parse_integer_fixup_radix 248 228 -20 fdt_record_loadable 372 336 -36 printch 80 40 -40 static.dwmci_wait_reset 60 - -60 static.clk_get_by_indexed_prop 80 - -80 dcache_disable 132 48 -84 mmc_init 4576 4464 -112 static.dwmci_fifo_ready 124 - -124 static.sdhci_reset 136 - -136 Which is not good, and I need to dig in to a bit more to see what changes cause this.
On Thu, May 14, 2020 at 1:13 AM Tom Rini <trini at 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 at 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 at 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
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 at 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 at 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 at 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!
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 at 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 at 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 at 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.
On Sat, May 09, 2020 at 05:12:42PM +0200, Heinrich Schuchardt wrote: > GCC recognizes /* fallthrough */ if -Wimplicit-fallthrough=3 is enabled. > Let's use it consistently. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de> Applied to u-boot/master, thanks!
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index 7e168a1e6b..8b4d2765ad 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -923,7 +923,9 @@ static const char usage_synopsis[] = /* Helper for getopt case statements */ #define case_USAGE_COMMON_FLAGS \ case 'h': usage(NULL); \ + /* fallthrough */ \ case 'V': util_version(); \ + /* fallthrough */ \ case '?': usage("unknown option"); static const char usage_short_opts[] = @@ -1085,6 +1087,7 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) switch (opt) { case_USAGE_COMMON_FLAGS + /* fallthrough */ case 'a': disp->show_addr = 1; break; @@ -1096,7 +1099,7 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) break; case 'C': inc = 0; - /* no break */ + /* fallthrough */ case 'c': type = FDT_IS_COMPAT; break; @@ -1111,7 +1114,7 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) break; case 'G': inc = 0; - /* no break */ + /* fallthrough */ case 'g': type = FDT_ANY_GLOBAL; break; @@ -1129,7 +1132,7 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) break; case 'N': inc = 0; - /* no break */ + /* fallthrough */ case 'n': type = FDT_IS_NODE; break; @@ -1148,7 +1151,7 @@ static void scan_args(struct display_info *disp, int argc, char *argv[]) break; case 'P': inc = 0; - /* no break */ + /* fallthrough */ case 'p': type = FDT_IS_PROP; break;
GCC recognizes /* fallthrough */ if -Wimplicit-fallthrough=3 is enabled. Let's use it consistently. Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de> --- tools/fdtgrep.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) -- 2.26.2