* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
@ 2020-05-09 15:12 Heinrich Schuchardt
2020-05-10 13:12 ` Masahiro Yamada
2020-05-15 20:54 ` Tom Rini
0 siblings, 2 replies; 12+ messages in thread
From: Heinrich Schuchardt @ 2020-05-09 15:12 UTC (permalink / raw)
To: u-boot
GCC recognizes /* fallthrough */ if -Wimplicit-fallthrough=3 is enabled.
Let's use it consistently.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-09 15:12 [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed Heinrich Schuchardt
@ 2020-05-10 13:12 ` Masahiro Yamada
2020-05-11 18:40 ` Tom Rini
2020-05-15 20:54 ` Tom Rini
1 sibling, 1 reply; 12+ messages in thread
From: Masahiro Yamada @ 2020-05-10 13:12 UTC (permalink / raw)
To: u-boot
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.
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@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
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-10 13:12 ` Masahiro Yamada
@ 2020-05-11 18:40 ` Tom Rini
2020-05-11 19:08 ` Heinrich Schuchardt
0 siblings, 1 reply; 12+ messages in thread
From: Tom Rini @ 2020-05-11 18:40 UTC (permalink / raw)
To: u-boot
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.
>
>
> 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.
--
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/20200511/a0877143/attachment-0001.sig>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-11 18:40 ` Tom Rini
@ 2020-05-11 19:08 ` Heinrich Schuchardt
2020-05-13 3:04 ` Tom Rini
0 siblings, 1 reply; 12+ messages in thread
From: Heinrich Schuchardt @ 2020-05-11 19:08 UTC (permalink / raw)
To: u-boot
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.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-11 19:08 ` Heinrich Schuchardt
@ 2020-05-13 3:04 ` Tom Rini
2020-05-13 14:42 ` Tom Rini
0 siblings, 1 reply; 12+ messages in thread
From: Tom Rini @ 2020-05-13 3:04 UTC (permalink / raw)
To: u-boot
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.
--
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/20200512/1f59ea71/attachment.sig>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-13 3:04 ` Tom Rini
@ 2020-05-13 14:42 ` Tom Rini
2020-05-13 16:05 ` Masahiro Yamada
0 siblings, 1 reply; 12+ messages in thread
From: Tom Rini @ 2020-05-13 14:42 UTC (permalink / raw)
To: u-boot
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.
--
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/2e906188/attachment.sig>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-13 14:42 ` Tom Rini
@ 2020-05-13 16:05 ` Masahiro Yamada
2020-05-13 16:13 ` Tom Rini
0 siblings, 1 reply; 12+ messages in thread
From: Masahiro Yamada @ 2020-05-13 16:05 UTC (permalink / raw)
To: u-boot
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?
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-13 16:05 ` Masahiro Yamada
@ 2020-05-13 16:13 ` Tom Rini
2020-05-13 17:27 ` Masahiro Yamada
0 siblings, 1 reply; 12+ messages in thread
From: Tom Rini @ 2020-05-13 16:13 UTC (permalink / raw)
To: u-boot
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
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.
--
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/ad19d04f/attachment.sig>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-13 16:13 ` Tom Rini
@ 2020-05-13 17:27 ` Masahiro Yamada
2020-05-13 18:41 ` Tom Rini
0 siblings, 1 reply; 12+ messages in thread
From: Masahiro Yamada @ 2020-05-13 17:27 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-13 17:27 ` Masahiro Yamada
@ 2020-05-13 18:41 ` Tom Rini
2020-05-13 21:52 ` Tom Rini
0 siblings, 1 reply; 12+ messages in thread
From: Tom Rini @ 2020-05-13 18:41 UTC (permalink / raw)
To: u-boot
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!
--
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/256e56e2/attachment.sig>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-13 18:41 ` Tom Rini
@ 2020-05-13 21:52 ` Tom Rini
0 siblings, 0 replies; 12+ messages in thread
From: Tom Rini @ 2020-05-13 21:52 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed
2020-05-09 15:12 [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed Heinrich Schuchardt
2020-05-10 13:12 ` Masahiro Yamada
@ 2020-05-15 20:54 ` Tom Rini
1 sibling, 0 replies; 12+ messages in thread
From: Tom Rini @ 2020-05-15 20:54 UTC (permalink / raw)
To: u-boot
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@gmx.de>
Applied to u-boot/master, thanks!
--
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/20200515/e0825bd2/attachment.sig>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-05-15 20:54 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-09 15:12 [PATCH 1/1] tools: ftdgrep: use /* fallthrough */ as needed Heinrich Schuchardt
2020-05-10 13:12 ` Masahiro Yamada
2020-05-11 18:40 ` Tom Rini
2020-05-11 19:08 ` Heinrich Schuchardt
2020-05-13 3:04 ` Tom Rini
2020-05-13 14:42 ` Tom Rini
2020-05-13 16:05 ` Masahiro Yamada
2020-05-13 16:13 ` Tom Rini
2020-05-13 17:27 ` Masahiro Yamada
2020-05-13 18:41 ` Tom Rini
2020-05-13 21:52 ` Tom Rini
2020-05-15 20:54 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.