clk: samsung: s3c2443: Mark expected switch fall-through
Subsystems (3)
PATCH 1/1 clk: samsung: s3c2443: Mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
This patch fixes the following warnings:
drivers/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
samsung_clk_register_alias(ctx, s3c2450_aliases, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ARRAY_SIZE(s3c2450_aliases));
~~~~~~~~~~~~~~~~~~~~~~~~~~~~drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
case S3C2416:
^~~~
Warning level 3 was used: -Wimplicit-fallthrough=3
Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find.
This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.
--- drivers/clk/samsung/clk-s3c2443.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/clk/samsung/clk-s3c2443.c +1 -1
@@ -389,7 +389,7 @@ void __init s3c2443_common_clk_init(struct device_node *np, unsigned long xti_f, ARRAY_SIZE(s3c2450_gates));samsung_clk_register_alias (ctx, s3c2450_aliases,ARRAY_SIZE (s3c2450_aliases)); -/* fall through, as s3c2450 extends the s3c2416 clocks */ +/* fall through - as s3c2450 extends the s3c2416 clocks */ case S3C2416 :samsung_clk_register_div (ctx, s3c2416_dividers,ARRAY_SIZE (s3c2416_dividers));
Signature · 7 lines
2.20.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Chanwoo Choi
maintainerHi Gustavo,
Quoted 15 lines · Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
This patch fixes the following warnings:
drivers/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
samsung_clk_register_alias(ctx,s3c2450_aliases, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ARRAY_SIZE(s3c2450_aliases)); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
case S3C2416:
^~~~
Quoted 7 lines
Warning level 3 was used: -Wimplicit-fallthrough=3
Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find.
This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.
Except for level 5 of -Wimplicit-fallthrough,
level 4 is more strict to show the warnings.
Why don't you support level 4 for -Wimplicit-fallthrough? I think that you want to fix for -Wimplicit-fallthrough warning, you better to support level 4. What do you think?
Quoted 20 lines
Signed-off-by Gustavo A. R. Silva <[email protected]>--- drivers/clk/samsung/clk-s3c2443.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/clk/samsung/clk-s3c2443.c +1
-1@@ -389,7 +389,7 @@ void __init s3c2443_common_clk_init(struct device_node *np, unsigned long xti_f, ARRAY_SIZE(s3c2450_gates));samsung_clk_register_alias (ctx, s3c2450_aliases,ARRAY_SIZE (s3c2450_aliases)); -/* fall through, as s3c2450 extends the s3c2416 clocks */ +/* fall through - as s3c2450 extends the s3c2416 clocks */ case S3C2416 :samsung_clk_register_div (ctx, s3c2416_dividers,ARRAY_SIZE (s3c2416_dividers));
Signature · 3 lines
Best Regards, Chanwoo Choi Samsung Electronics
Krzysztof Kozlowski
maintainerQuoted 16 lines · Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
This patch fixes the following warnings:
drivers/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
samsung_clk_register_alias(ctx,s3c2450_aliases, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ARRAY_SIZE(s3c2450_aliases)); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
case S3C2416:
^~~~
Quoted 7 lines
Warning level 3 was used: -Wimplicit-fallthrough=3
Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find.
This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.
I saw this in multiple places already and I think fix is wrong. The point is that the code is correct - the fall through is marked properly.
It is just the GCC which has to be fixed not the code. You want to adjust the code for specific version of GCC and what if GCC changes its warning? For example GCC might require "fall through: "... or any other syntax. Another point - what about clang's syntax?
Best regards,
Krzysztof
Quoted 27 lines · Krzysztof Kozlowski
Quoted 16 lines · Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
This patch fixes the following warnings:
drivers/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
samsung_clk_register_alias(ctx,s3c2450_aliases, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ARRAY_SIZE(s3c2450_aliases)); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
case S3C2416:
^~~~Quoted 7 lines
Warning level 3 was used: -Wimplicit-fallthrough=3
Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find.
This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.
Quoted 8 lines
I saw this in multiple places already and I think fix is wrong. The point is that the code is correct - the fall through is marked properly.
It is just the GCC which has to be fixed not the code. You want to adjust the code for specific version of GCC and what if GCC changes its warning? For example GCC might require "fall through: "... or any other syntax. Another point - what about clang's syntax?
-Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its choice. GCC's levels were chosen based on the existing linters, static analyzers, etc. The patterns are unlikely to change (see the gcc man-page).
Clang doesn't recognize anything in C mode (hopefully this will be fixed in the future[1]).
As long as one of the compilers is able to check this, we'll avoid the bugs associated with this mis-pattern. Gustavo's efforts here have found kind of a lot of bugs, so I think it's worth a little churn to add these (and make minor adjustments to existing comments).
[1] https://bugs.llvm.org/show_bug.cgi?id=37135
Signature · 1 line
Kees Cook
Stephen Boyd
maintainerQuoting Kees Cook (2019-02-12 10:57:05)
Quoted 12 lines · Krzysztof Kozlowski
Quoted 5 lines
It is just the GCC which has to be fixed not the code. You want to adjust the code for specific version of GCC and what if GCC changes its warning? For example GCC might require "fall through: "... or any other syntax. Another point - what about clang's syntax?
-Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its choice. GCC's levels were chosen based on the existing linters, static analyzers, etc. The patterns are unlikely to change (see the gcc man-page).
Quoted 7 lines
Clang doesn't recognize anything in C mode (hopefully this will be fixed in the future[1]).
As long as one of the compilers is able to check this, we'll avoid the bugs associated with this mis-pattern. Gustavo's efforts here have found kind of a lot of bugs, so I think it's worth a little churn to add these (and make minor adjustments to existing comments).
Just curious, what compilation phase does this check run in? Could we gain a macro like FALLTHRU or even lowercase 'fallthru' that expanded to whatever the compiler wants to see and then there would only be "one way" to do this? It would alleviate the above concerns, but maybe I'm rehashing something that's already been proposed and rejected.
Of course, I'm happy to merge any of these patches that tweak things so no worries either way.
Quoted 21 lines · Stephen Boyd
Quoting Kees Cook (2019-02-12 10:57:05)
Quoted 12 lines · Krzysztof Kozlowski
Quoted 5 lines
It is just the GCC which has to be fixed not the code. You want to adjust the code for specific version of GCC and what if GCC changes its warning? For example GCC might require "fall through: "... or any other syntax. Another point - what about clang's syntax?
-Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its choice. GCC's levels were chosen based on the existing linters, static analyzers, etc. The patterns are unlikely to change (see the gcc man-page).
Quoted 7 lines
Clang doesn't recognize anything in C mode (hopefully this will be fixed in the future[1]).
As long as one of the compilers is able to check this, we'll avoid the bugs associated with this mis-pattern. Gustavo's efforts here have found kind of a lot of bugs, so I think it's worth a little churn to add these (and make minor adjustments to existing comments).
Quoted 5 lines
Just curious, what compilation phase does this check run in? Could we gain a macro like FALLTHRU or even lowercase 'fallthru' that expanded to whatever the compiler wants to see and then there would only be "one way" to do this? It would alleviate the above concerns, but maybe I'm rehashing something that's already been proposed and rejected.
When this got discussed a while back, the thinking was that since we're also dealing with static analyzers (e.g. Coverity) and IDEs that literally parse comments in the code, it was most sensible (at least for now, prior to there being a formal C "fall through" statement -- there is for C++ but not yet for C), we'd stick to explicit comments. In theory, we will be able to do a tree-wide change to add the C statement once it exists.
Quoted 2 lines
Of course, I'm happy to merge any of these patches that tweak things so no worries either way.
Thanks! :)
Signature · 1 line
Kees Cook
Stephen Boyd
maintainerQuoting Kees Cook (2019-02-20 14:26:06)
Quoted 28 lines · Stephen Boyd
Quoted 21 lines
Quoting Kees Cook (2019-02-12 10:57:05)
Quoted 12 lines · Krzysztof Kozlowski
Quoted 5 lines
It is just the GCC which has to be fixed not the code. You want to adjust the code for specific version of GCC and what if GCC changes its warning? For example GCC might require "fall through: "... or any other syntax. Another point - what about clang's syntax?
-Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its choice. GCC's levels were chosen based on the existing linters, static analyzers, etc. The patterns are unlikely to change (see the gcc man-page).
Quoted 7 lines
Clang doesn't recognize anything in C mode (hopefully this will be fixed in the future[1]).
As long as one of the compilers is able to check this, we'll avoid the bugs associated with this mis-pattern. Gustavo's efforts here have found kind of a lot of bugs, so I think it's worth a little churn to add these (and make minor adjustments to existing comments).
Quoted 5 lines
Just curious, what compilation phase does this check run in? Could we gain a macro like FALLTHRU or even lowercase 'fallthru' that expanded to whatever the compiler wants to see and then there would only be "one way" to do this? It would alleviate the above concerns, but maybe I'm rehashing something that's already been proposed and rejected.
Quoted 7 lines
When this got discussed a while back, the thinking was that since we're also dealing with static analyzers (e.g. Coverity) and IDEs that literally parse comments in the code, it was most sensible (at least for now, prior to there being a formal C "fall through" statement -- there is for C++ but not yet for C), we'd stick to explicit comments. In theory, we will be able to do a tree-wide change to add the C statement once it exists.
Ok, thanks for the background. Looks like the perf tool already introduced the #define __fallthrough that they use for this purpose. Maybe they're hoping that it will be formalized.
Quoted 3 lines · Stephen Boyd
Ok, thanks for the background. Looks like the perf tool already introduced the #define
__fallthroughthat they use for this purpose. Maybe they're hoping that it will be formalized.
Yeah, GCC has an extension for it (but it's not yet part of the C standard -- though there are open bugs against Clang to support it). Our adoption of the markings, thanks to Gustavo's work, has rapidly increased lately too (we started with something like 2400 instances and we were gaining about as many each cycle as we removed). 5.1, though, is closing in on _0_ instances. My thinking is that once we're to the point where we can globally enable -Wimplicit-fallthrough, then we can depend on the compiler to enforce this (instead continuing to depend on external tracking in Coverity and the like). It was a bit of a chicken-and-egg and I was afraid we were going to be left with this "partial adoption" for a long time. But we're nearly to the point where I would be happy doing a tree-wide replacement to __fallthrough (which should be mechanically easy).
Signature · 1 line
Kees Cook
Quoting Gustavo A. R. Silva (2019-02-11 10:15:31)
Quoted 17 lines
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
This patch fixes the following warnings:
drivers/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
samsung_clk_register_alias(ctx,s3c2450_aliases, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ARRAY_SIZE(s3c2450_aliases)); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
case S3C2416:
^~~~Warning level 3 was used: -Wimplicit-fallthrough=3
Quoted 8 lines
Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find.
This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.
Signed-off-by Gustavo A. R. Silva <[email protected]>---
Applied to clk-next