A compilation -Wimplicit-fallthrough warning was enabled by
commit a035d552a93b ("Makefile: Globally enable fall-through warning")
Even though clang 10.0.0 does not currently support this warning
without a patch, clang currently does not support a value for this
option.
Link: https://bugs.llvm.org/show_bug.cgi?id=39382
The gcc default for this warning is 3 so removing the =3 has no
effect for gcc and enables the warning for patched versions of clang.
Also remove the =3 from an existing use in a parisc Makefile:
arch/parisc/math-emu/Makefile
Signed-off-by: Joe Perches <[email protected]>
---
Makefile | 2 +-
arch/parisc/math-emu/Makefile | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 5ee6f6889869..a3985421fd29 100644
--- a/Makefile
+++ b/Makefile
@@ -845,7 +845,7 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
KBUILD_CFLAGS += -Wdeclaration-after-statement
# Warn about unmarked fall-throughs in switch statement.
-KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=3,)
+KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
# Variable Length Arrays (VLAs) should not be used anywhere in the kernel
KBUILD_CFLAGS += -Wvla
diff --git a/arch/parisc/math-emu/Makefile b/arch/parisc/math-emu/Makefile
index 55c1396580a4..3747a0cbd3b8 100644
--- a/arch/parisc/math-emu/Makefile
+++ b/arch/parisc/math-emu/Makefile
@@ -18,4 +18,4 @@ obj-y := frnd.o driver.o decode_exc.o fpudispatch.o denormal.o \
# other very old or stripped-down PA-RISC CPUs -- not currently supported
obj-$(CONFIG_MATH_EMULATION) += unimplemented-math-emulation.o
-CFLAGS_REMOVE_fpudispatch.o = -Wimplicit-fallthrough=3
+CFLAGS_REMOVE_fpudispatch.o = -Wimplicit-fallthrough
On Mon, Aug 05, 2019 at 03:11:15PM -0700, Joe Perches wrote:
> A compilation -Wimplicit-fallthrough warning was enabled by
> commit a035d552a93b ("Makefile: Globally enable fall-through warning")
>
> Even though clang 10.0.0 does not currently support this warning
> without a patch, clang currently does not support a value for this
> option.
>
> Link: https://bugs.llvm.org/show_bug.cgi?id=39382
>
> The gcc default for this warning is 3 so removing the =3 has no
> effect for gcc and enables the warning for patched versions of clang.
>
> Also remove the =3 from an existing use in a parisc Makefile:
> arch/parisc/math-emu/Makefile
>
> Signed-off-by: Joe Perches <[email protected]>
Looks good to me. I can see this warning get added to the command line
even if it does nothing right now.
Reviewed-by: Nathan Chancellor <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>
On Mon, 2019-08-05 at 15:11 -0700, Joe Perches wrote:
> A compilation -Wimplicit-fallthrough warning was enabled by
> commit a035d552a93b ("Makefile: Globally enable fall-through warning")
>
> Even though clang 10.0.0 does not currently support this warning
> without a patch, clang currently does not support a value for this
> option.
>
> Link: https://bugs.llvm.org/show_bug.cgi?id=39382
>
> The gcc default for this warning is 3 so removing the =3 has no
> effect for gcc and enables the warning for patched versions of clang.
>
> Also remove the =3 from an existing use in a parisc Makefile:
> arch/parisc/math-emu/Makefile
Hey Linus,
What does it take for this sort of patch to be applied by you?
> Signed-off-by: Joe Perches <[email protected]>
> ---
> Makefile | 2 +-
> arch/parisc/math-emu/Makefile | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 5ee6f6889869..a3985421fd29 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -845,7 +845,7 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> KBUILD_CFLAGS += -Wdeclaration-after-statement
>
> # Warn about unmarked fall-throughs in switch statement.
> -KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=3,)
> +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
>
> # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> KBUILD_CFLAGS += -Wvla
> diff --git a/arch/parisc/math-emu/Makefile b/arch/parisc/math-emu/Makefile
> index 55c1396580a4..3747a0cbd3b8 100644
> --- a/arch/parisc/math-emu/Makefile
> +++ b/arch/parisc/math-emu/Makefile
> @@ -18,4 +18,4 @@ obj-y := frnd.o driver.o decode_exc.o fpudispatch.o denormal.o \
> # other very old or stripped-down PA-RISC CPUs -- not currently supported
>
> obj-$(CONFIG_MATH_EMULATION) += unimplemented-math-emulation.o
> -CFLAGS_REMOVE_fpudispatch.o = -Wimplicit-fallthrough=3
> +CFLAGS_REMOVE_fpudispatch.o = -Wimplicit-fallthrough
>
On Sat, Aug 10, 2019 at 12:32 PM Joe Perches <[email protected]> wrote:
>
> What does it take for this sort of patch to be applied by you?
The basic rule tends to be: "normal channels".
For example, in the case of most of your patches, that tends to be
through Andrew, since most of them tend to be about the scripts.
In this case, I would have expected the patch to come in the same way
that the original Makefile change came in and follow-up fallthrough
fixups have come, ie though Gustavo's tree.
I certainly do take patches directly too, but tend to do so only if I
feel there's some problem with the process.
I pulled from Gustavo earlier today to add a few more expected switch
fall-through's, I guess I can take this Makefile change directly.
Linus
On Sat, 2019-08-10 at 12:44 -0700, Linus Torvalds wrote:
> On Sat, Aug 10, 2019 at 12:32 PM Joe Perches <[email protected]> wrote:
> > What does it take for this sort of patch to be applied by you?
>
> The basic rule tends to be: "normal channels".
[]
> I pulled from Gustavo earlier today to add a few more expected switch
> fall-through's, I guess I can take this Makefile change directly.
Thanks. It's simple enough.
There are classes of patches generated by scripts that have
no real mechanism to be applied today.
For instance: global coccinelle scripted changes to use stracpy
https://lore.kernel.org/lkml/alpine.DEB.2.21.1907251747560.2494@hadrien/
and trivial scripted changes to MAINTAINERS
https://lore.kernel.org/lkml/[email protected]/
that are basically impossible to be applied by anyone but you.
Otherwise there are hundreds of little micro patches most of
which would not otherwise be applied.
There should be some process available to get these treewide
or difficult to keep up-to-date and apply patches handled.
I believe these sorts of scripted patches should ideally
be handled immediately before an RC1 so other trees can be
synchronized in the simplest way possible.
On Sat, 2019-08-10 at 13:18 -0700, Joe Perches wrote:
> On Sat, 2019-08-10 at 12:44 -0700, Linus Torvalds wrote:
> > On Sat, Aug 10, 2019 at 12:32 PM Joe Perches <[email protected]> wrote:
> > > What does it take for this sort of patch to be applied by you?
> >
> > The basic rule tends to be: "normal channels".
> []
> > I pulled from Gustavo earlier today to add a few more expected switch
> > fall-through's, I guess I can take this Makefile change directly.
>
> Thanks. It's simple enough.
>
> There are classes of patches generated by scripts that have
> no real mechanism to be applied today.
>
> For instance: global coccinelle scripted changes to use stracpy
> https://lore.kernel.org/lkml/alpine.DEB.2.21.1907251747560.2494@hadrien/
>
> and trivial scripted changes to MAINTAINERS
> https://lore.kernel.org/lkml/[email protected]/
>
> that are basically impossible to be applied by anyone but you.
>
> Otherwise there are hundreds of little micro patches most of
> which would not otherwise be applied.
>
> There should be some process available to get these treewide
> or difficult to keep up-to-date and apply patches handled.
>
> I believe these sorts of scripted patches should ideally
> be handled immediately before an RC1 so other trees can be
> synchronized in the simplest way possible.
Hey Stephen
Question for you about a possible -next process change.
Would it be reasonable to have some mechanism to script
treewide patches to generate and apply after Andrew Morton's
mmotm patches are applied to -next?
This could allow treewide scripted patches to have
compilation and test coverage before possibly being
applied to Linus' tree.
What would be necessary to allow this?
On Sat, Aug 10, 2019 at 01:18:22PM -0700, Joe Perches wrote:
> On Sat, 2019-08-10 at 12:44 -0700, Linus Torvalds wrote:
> > On Sat, Aug 10, 2019 at 12:32 PM Joe Perches <[email protected]> wrote:
> > > What does it take for this sort of patch to be applied by you?
> >
> > The basic rule tends to be: "normal channels".
> []
> > I pulled from Gustavo earlier today to add a few more expected switch
> > fall-through's, I guess I can take this Makefile change directly.
>
> Thanks. It's simple enough.
>
> There are classes of patches generated by scripts that have
> no real mechanism to be applied today.
>
> For instance: global coccinelle scripted changes to use stracpy
> https://lore.kernel.org/lkml/alpine.DEB.2.21.1907251747560.2494@hadrien/
>
> and trivial scripted changes to MAINTAINERS
> https://lore.kernel.org/lkml/[email protected]/
>
> that are basically impossible to be applied by anyone but you.
>
> Otherwise there are hundreds of little micro patches most of
> which would not otherwise be applied.
>
> There should be some process available to get these treewide
> or difficult to keep up-to-date and apply patches handled.
>
> I believe these sorts of scripted patches should ideally
> be handled immediately before an RC1 so other trees can be
> synchronized in the simplest way possible.
>
Hi Joe,
On a tangential note, how are you planning on doing the fallthrough
comment to attribute conversion? The reason I ask is clang does not
support the comment annotations, meaning that when Nathan Huckleberry's
patch is applied to clang (which has been accepted [1]), we are going
to get slammed by the warnings. I just ran an x86 defconfig build at
296d05cb0d3c with his patch applied and I see 27673 instances of this
warning... (mostly coming from some header files so nothing crazy but it
will be super noisy).
If you have something to share like a script or patch, I'd be happy to
test it locally.
[1]: https://reviews.llvm.org/D64838
Cheers,
Nathan
On Sat, 2019-08-10 at 19:04 -0700, Nathan Chancellor wrote:
> On a tangential note, how are you planning on doing the fallthrough
> comment to attribute conversion? The reason I ask is clang does not
> support the comment annotations, meaning that when Nathan Huckleberry's
> patch is applied to clang (which has been accepted [1]), we are going
> to get slammed by the warnings. I just ran an x86 defconfig build at
> 296d05cb0d3c with his patch applied and I see 27673 instances of this
> warning... (mostly coming from some header files so nothing crazy but it
> will be super noisy).
>
> If you have something to share like a script or patch, I'd be happy to
> test it locally.
>
> [1]: https://reviews.llvm.org/D64838
Something like this patch:
https://lore.kernel.org/patchwork/patch/1108577/
Maybe use:
#define fallthrough [[fallthrough]]
if the compiler supports that notation
On Sat, Aug 10, 2019 at 08:06:05PM -0700, Joe Perches wrote:
> On Sat, 2019-08-10 at 19:04 -0700, Nathan Chancellor wrote:
> > On a tangential note, how are you planning on doing the fallthrough
> > comment to attribute conversion? The reason I ask is clang does not
> > support the comment annotations, meaning that when Nathan Huckleberry's
> > patch is applied to clang (which has been accepted [1]), we are going
> > to get slammed by the warnings. I just ran an x86 defconfig build at
> > 296d05cb0d3c with his patch applied and I see 27673 instances of this
> > warning... (mostly coming from some header files so nothing crazy but it
> > will be super noisy).
> >
> > If you have something to share like a script or patch, I'd be happy to
> > test it locally.
> >
> > [1]: https://reviews.llvm.org/D64838
>
> Something like this patch:
>
> https://lore.kernel.org/patchwork/patch/1108577/
>
> Maybe use:
>
> #define fallthrough [[fallthrough]]
>
> if the compiler supports that notation
>
That patch as it stands will work with D64838, as it is adding support
for the GNU fallthrough attribute.
However, I assume that all of the /* fall through */ comments will need
to be converted to the attribute macro, was that going to be done with
Coccinelle or something else?
On Sat, 2019-08-10 at 20:17 -0700, Nathan Chancellor wrote:
> On Sat, Aug 10, 2019 at 08:06:05PM -0700, Joe Perches wrote:
> > On Sat, 2019-08-10 at 19:04 -0700, Nathan Chancellor wrote:
> > > On a tangential note, how are you planning on doing the fallthrough
> > > comment to attribute conversion? The reason I ask is clang does not
> > > support the comment annotations, meaning that when Nathan Huckleberry's
> > > patch is applied to clang (which has been accepted [1]), we are going
> > > to get slammed by the warnings. I just ran an x86 defconfig build at
> > > 296d05cb0d3c with his patch applied and I see 27673 instances of this
> > > warning... (mostly coming from some header files so nothing crazy but it
> > > will be super noisy).
> > >
> > > If you have something to share like a script or patch, I'd be happy to
> > > test it locally.
> > >
> > > [1]: https://reviews.llvm.org/D64838
> >
> > Something like this patch:
> >
> > https://lore.kernel.org/patchwork/patch/1108577/
> >
> > Maybe use:
> >
> > #define fallthrough [[fallthrough]]
> >
> > if the compiler supports that notation
> >
>
> That patch as it stands will work with D64838, as it is adding support
> for the GNU fallthrough attribute.
>
> However, I assume that all of the /* fall through */ comments will need
> to be converted to the attribute macro, was that going to be done with
> Coccinelle or something else?
Coccinelle doesn't support transforming comments
so I am using a perl script for those transforms
that I will post when I'm happy enough with it.
On 11/08/2019 05:17, Nathan Chancellor wrote:
> On Sat, Aug 10, 2019 at 08:06:05PM -0700, Joe Perches wrote:
>> On Sat, 2019-08-10 at 19:04 -0700, Nathan Chancellor wrote:
>>> On a tangential note, how are you planning on doing the fallthrough
>>> comment to attribute conversion? The reason I ask is clang does not
>>> support the comment annotations, meaning that when Nathan Huckleberry's
>>> patch is applied to clang (which has been accepted [1]), we are going
>>> to get slammed by the warnings. I just ran an x86 defconfig build at
>>> 296d05cb0d3c with his patch applied and I see 27673 instances of this
>>> warning... (mostly coming from some header files so nothing crazy but it
>>> will be super noisy).
>>>
>>> If you have something to share like a script or patch, I'd be happy to
>>> test it locally.
>>>
>>> [1]: https://reviews.llvm.org/D64838
>>
>> Something like this patch:
>>
>> https://lore.kernel.org/patchwork/patch/1108577/
>>
>> Maybe use:
>>
>> #define fallthrough [[fallthrough]]
>>
>> if the compiler supports that notation
>>
>
> That patch as it stands will work with D64838, as it is adding support
> for the GNU fallthrough attribute.
>
> However, I assume that all of the /* fall through */ comments will need
> to be converted to the attribute macro, was that going to be done with
> Coccinelle or something else?
clang has not problem with the comment - it's just a comment;-)
The #define above works BTW.
MfG,
Bernd
--
Bernd Petrovitsch Email : [email protected]
LUGA : http://www.luga.at
On Sat, 2019-08-10 at 20:54 -0700, Joe Perches wrote:
> On Sat, 2019-08-10 at 20:17 -0700, Nathan Chancellor wrote:
> > On Sat, Aug 10, 2019 at 08:06:05PM -0700, Joe Perches wrote:
> > > On Sat, 2019-08-10 at 19:04 -0700, Nathan Chancellor wrote:
> > > > On a tangential note, how are you planning on doing the fallthrough
> > > > comment to attribute conversion? The reason I ask is clang does not
> > > > support the comment annotations, meaning that when Nathan Huckleberry's
> > > > patch is applied to clang (which has been accepted [1]), we are going
> > > > to get slammed by the warnings. I just ran an x86 defconfig build at
> > > > 296d05cb0d3c with his patch applied and I see 27673 instances of this
> > > > warning... (mostly coming from some header files so nothing crazy but it
> > > > will be super noisy).
> > > >
> > > > If you have something to share like a script or patch, I'd be happy to
> > > > test it locally.
[]
> Coccinelle doesn't support transforming comments
> so I am using a perl script for those transforms
> that I will post when I'm happy enough with it.
Well, it kinda works well enough.
Several things still need improvement but
try this with command lines like:
$ git ls-files <some_path> | \
xargs perl cvt_style.pl -o --convert=fallthrough
For instance:
$ git ls-files crypto | xargs perl cvt_style.pl -o --convert=fallthrough
Converted crypto/drbg.c
1 fallthrough
Converted crypto/tcrypt.c
57 fallthrough
Warning: these changes may not be correct.
These changes should be carefully reviewed manually and not combined with
any functional changes.
Compile, build and test your changes.
You should understand and be responsible for all object changes.
Make sure you read Documentation/SubmittingPatches before sending
any changes to reviewers, maintainers or mailing lists.
---
That command line creates:
---
$ git diff --stat -p crypto/
crypto/drbg.c | 3 +-
crypto/tcrypt.c | 114 ++++++++++++++++++++++++++++----------------------------
2 files changed, 58 insertions(+), 59 deletions(-)
diff --git a/crypto/drbg.c b/crypto/drbg.c
index b6929eb5f565..cd7eef227ff8 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1505,8 +1505,7 @@ static int drbg_prepare_hrng(struct drbg_state *drbg)
case -EALREADY:
err = 0;
- /* fall through */
-
+ fallthrough;
default:
drbg->random_ready.func = NULL;
return err;
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index c578ccd92c57..f236c057dd9f 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -2339,121 +2339,121 @@ static int do_test(const char *alg, u32 type, u32 mask, int m, u32 num_mb)
test_hash_speed(alg, sec, generic_hash_speed_template);
break;
}
- /* fall through */
+ fallthrough;
case 301:
test_hash_speed("md4", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 302:
test_hash_speed("md5", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 303:
test_hash_speed("sha1", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 304:
test_hash_speed("sha256", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 305:
test_hash_speed("sha384", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 306:
test_hash_speed("sha512", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 307:
test_hash_speed("wp256", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 308:
test_hash_speed("wp384", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 309:
test_hash_speed("wp512", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 310:
test_hash_speed("tgr128", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 311:
test_hash_speed("tgr160", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 312:
test_hash_speed("tgr192", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 313:
test_hash_speed("sha224", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 314:
test_hash_speed("rmd128", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 315:
test_hash_speed("rmd160", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 316:
test_hash_speed("rmd256", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 317:
test_hash_speed("rmd320", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 318:
test_hash_speed("ghash-generic", sec, hash_speed_template_16);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 319:
test_hash_speed("crc32c", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 320:
test_hash_speed("crct10dif", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 321:
test_hash_speed("poly1305", sec, poly1305_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 322:
test_hash_speed("sha3-224", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 323:
test_hash_speed("sha3-256", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 324:
test_hash_speed("sha3-384", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 325:
test_hash_speed("sha3-512", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 326:
test_hash_speed("sm3", sec, generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 327:
test_hash_speed("streebog256", sec,
generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 328:
test_hash_speed("streebog512", sec,
generic_hash_speed_template);
if (mode > 300 && mode < 400) break;
- /* fall through */
+ fallthrough;
case 399:
break;
@@ -2462,121 +2462,121 @@ static int do_test(const char *alg, u32 type, u32 mask, int m, u32 num_mb)
test_ahash_speed(alg, sec, generic_hash_speed_template);
break;
}
- /* fall through */
+ fallthrough;
case 401:
test_ahash_speed("md4", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 402:
test_ahash_speed("md5", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 403:
test_ahash_speed("sha1", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 404:
test_ahash_speed("sha256", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 405:
test_ahash_speed("sha384", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 406:
test_ahash_speed("sha512", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 407:
test_ahash_speed("wp256", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 408:
test_ahash_speed("wp384", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 409:
test_ahash_speed("wp512", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 410:
test_ahash_speed("tgr128", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 411:
test_ahash_speed("tgr160", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 412:
test_ahash_speed("tgr192", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 413:
test_ahash_speed("sha224", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 414:
test_ahash_speed("rmd128", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 415:
test_ahash_speed("rmd160", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 416:
test_ahash_speed("rmd256", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 417:
test_ahash_speed("rmd320", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 418:
test_ahash_speed("sha3-224", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 419:
test_ahash_speed("sha3-256", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 420:
test_ahash_speed("sha3-384", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 421:
test_ahash_speed("sha3-512", sec, generic_hash_speed_template);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 422:
test_mb_ahash_speed("sha1", sec, generic_hash_speed_template,
num_mb);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 423:
test_mb_ahash_speed("sha256", sec, generic_hash_speed_template,
num_mb);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 424:
test_mb_ahash_speed("sha512", sec, generic_hash_speed_template,
num_mb);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 425:
test_mb_ahash_speed("sm3", sec, generic_hash_speed_template,
num_mb);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 426:
test_mb_ahash_speed("streebog256", sec,
generic_hash_speed_template, num_mb);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 427:
test_mb_ahash_speed("streebog512", sec,
generic_hash_speed_template, num_mb);
if (mode > 400 && mode < 500) break;
- /* fall through */
+ fallthrough;
case 499:
break;
On Sat, Aug 10, 2019 at 8:06 PM Joe Perches <[email protected]> wrote:
>
> On Sat, 2019-08-10 at 19:04 -0700, Nathan Chancellor wrote:
> > On a tangential note, how are you planning on doing the fallthrough
> > comment to attribute conversion? The reason I ask is clang does not
> > support the comment annotations, meaning that when Nathan Huckleberry's
> > patch is applied to clang (which has been accepted [1]), we are going
> > to get slammed by the warnings. I just ran an x86 defconfig build at
> > 296d05cb0d3c with his patch applied and I see 27673 instances of this
> > warning... (mostly coming from some header files so nothing crazy but it
> > will be super noisy).
> >
> > If you have something to share like a script or patch, I'd be happy to
> > test it locally.
> >
> > [1]: https://reviews.llvm.org/D64838
>
> Something like this patch:
>
> https://lore.kernel.org/patchwork/patch/1108577/
>
> Maybe use:
>
> #define fallthrough [[fallthrough]]
Isn't [[fallthrough]] the C++ style attribute? **eek** Seems to be a
waste for Clang to implement __attribute__((fallthrough)) just as we
switch the kernel to not use it. Also, I'd recommend making the
preprocessor define all caps to help folks recognize it's a
preprocessor define.
--
Thanks,
~Nick Desaulniers
On Mon, 2019-08-12 at 09:28 -0700, Nick Desaulniers wrote:
> Isn't [[fallthrough]] the C++ style attribute?
double brackets will likely at some point become the default
attribute style for c as well. It is not now though and
linux will continue to support gcc 7+ and the __attribute__
style for quite a while.
The minimum gcc version just moved to 4.6 which was
released in 2013 so likely linux won't move to something
that requires [[<attribute>]] for a decade or more.
> **eek** Seems to be a
> waste for Clang to implement __attribute__((fallthrough)) just as we
> switch the kernel to not use it.
clang already supports the __attribute__((<foo>)) style for
gcc compatibility.
This is just clang supporting a new <foo> type which would
nominally be required for gcc compatibility anyway.
> Also, I'd recommend making the
> preprocessor define all caps to help folks recognize it's a
> preprocessor define.
It's more a matching styles thing.
I rather suspect that the c committees would choose to add
fallthrough as a keyword if it was possible, but it is not
possible without risking breaking existing code.
linux source code is not constrained by this requirement.
In my opinion, case statement blocks should always use a
terminating keyword. I think the best option is to add
fallthrough as a keyword, but there are other options:
IMO the best option is:
break;; goto; return; fallthrough;
or (slightly worse)
break; goto; return; __fallthrough;
or (even worse)
break; goto; return; FALLTHROUGH;
Generic arguments pro/con for each style:
----------------------------------------
fallthrough looks like normal code but could not be
used in uapi headers.
__fallthrough is underscore prefixed, so reserved and
generic, and could be used in uapi headers. __fallthrough
is rather unnatural looking when used to terminate a case
statement block.
FALLTHROUGH looks like a macro, but could not be used in
uapi headers. It is also rather unnatural looking when
used to terminate a case statement block.
----------------------------------------
There are no existing uses of fallthrough in uapi headers.
On Mon, Aug 12, 2019 at 6:29 PM Nick Desaulniers
<[email protected]> wrote:
>
> On Sat, Aug 10, 2019 at 8:06 PM Joe Perches <[email protected]> wrote:
> >
> > On Sat, 2019-08-10 at 19:04 -0700, Nathan Chancellor wrote:
> > > On a tangential note, how are you planning on doing the fallthrough
> > > comment to attribute conversion? The reason I ask is clang does not
> > > support the comment annotations, meaning that when Nathan Huckleberry's
> > > patch is applied to clang (which has been accepted [1]), we are going
> > > to get slammed by the warnings. I just ran an x86 defconfig build at
> > > 296d05cb0d3c with his patch applied and I see 27673 instances of this
> > > warning... (mostly coming from some header files so nothing crazy but it
> > > will be super noisy).
> > >
> > > If you have something to share like a script or patch, I'd be happy to
> > > test it locally.
> > >
> > > [1]: https://reviews.llvm.org/D64838
> >
> > Something like this patch:
> >
> > https://lore.kernel.org/patchwork/patch/1108577/
> >
> > Maybe use:
> >
> > #define fallthrough [[fallthrough]]
>
> Isn't [[fallthrough]] the C++ style attribute? **eek** Seems to be a
It is going to be very likely also the C spelling too, i.e. C2x will
likely be released with attributes. [[fallthrough]] in particular is
still on discussion but it may be included too (thanks Aaron!).
> waste for Clang to implement __attribute__((fallthrough)) just as we
> switch the kernel to not use it. Also, I'd recommend making the
> preprocessor define all caps to help folks recognize it's a
> preprocessor define.
Hm... I would go for either __fallthrough as the rest of attributes,
or simply fallthrough -- FALLTHROUGH seems wrong. If you want it that
way for visibility, then I would choose __fallthrough, since the
underscores are quite prominent and anyway IDEs typically highlight
macros in a different color than keywords (return etc.).
Cheers,
Miguel
On Tue, 2019-08-13 at 14:44 +0200, Miguel Ojeda wrote:
> Hm... I would go for either __fallthrough as the rest of attributes,
> or simply fallthrough -- FALLTHROUGH seems wrong. If you want it that
> way for visibility, then I would choose __fallthrough, since the
> underscores are quite prominent and anyway IDEs typically highlight
> macros in a different color than keywords (return etc.).
Just fyi:
I added this line to my .emacs and "fallthrough" is now
syntax highlighted like every other keyword.
(font-lock-add-keywords 'c-mode
'(("\\<\\(fallthrough\\)\\>" . font-lock-keyword-face)))
So now my linux-c-mode block is:
(defun linux-c-mode ()
"C mode with adjusted defaults for use with the Linux kernel."
(interactive)
(font-lock-add-keywords 'c-mode
'(("\\<\\(fallthrough\\)\\>" . font-lock-keyword-face)))
(c-mode)
(c-set-style "K&R")
(setq c-basic-offset 8)
(setq c-indent-level 8)
(setq c-brace-imaginary-offset 0)
(setq c-brace-offset -8)
(setq c-argdecl-indent 8)
(setq c-label-offset -8)
(setq c-continued-statement-offset 8)
(setq indent-tabs-mode t)
(setq tab-width 8)
(setq show-trailing-whitespace t)
)
I don't know to do that for vim nor any other ide,
but I trust someone will know and show how it's done.
(adding Vivien Didelot for vim)
On Wed, 2019-08-14 at 19:44 -0700, Joe Perches wrote:
> On Tue, 2019-08-13 at 14:44 +0200, Miguel Ojeda wrote:
> > Hm... I would go for either __fallthrough as the rest of attributes,
> > or simply fallthrough -- FALLTHROUGH seems wrong. If you want it that
> > way for visibility, then I would choose __fallthrough, since the
> > underscores are quite prominent and anyway IDEs typically highlight
> > macros in a different color than keywords (return etc.).
>
> Just fyi:
>
> I added this line to my .emacs and "fallthrough" is now
> syntax highlighted like every other keyword.
>
> (font-lock-add-keywords 'c-mode
> '(("\\<\\(fallthrough\\)\\>" . font-lock-keyword-face)))
>
> So now my linux-c-mode block is:
>
> (defun linux-c-mode ()
> "C mode with adjusted defaults for use with the Linux kernel."
> (interactive)
> (font-lock-add-keywords 'c-mode
> '(("\\<\\(fallthrough\\)\\>" . font-lock-keyword-face)))
> (c-mode)
> (c-set-style "K&R")
> (setq c-basic-offset 8)
> (setq c-indent-level 8)
> (setq c-brace-imaginary-offset 0)
> (setq c-brace-offset -8)
> (setq c-argdecl-indent 8)
> (setq c-label-offset -8)
> (setq c-continued-statement-offset 8)
> (setq indent-tabs-mode t)
> (setq tab-width 8)
> (setq show-trailing-whitespace t)
> )
>
> I don't know to do that for vim nor any other ide,
> but I trust someone will know and show how it's done.
I investigated vim a bit as I am not a vim user.
If this is the most commonly used vim style,
perhaps a reference to this style could be added
to Documentation/process/coding-style.rst
https://github.com/vivien/vim-linux-coding-style
I had thought that
syn keyword cKeyword fallthrough
would work, but it does not add syntax coloring
syn keyword cStatement fallthrough
does through.
If ~.vim/plugin/linuxsty.vim is commonly used, maybe
---
plugin/linuxsty.vim | 1 +
1 file changed, 1 insertion(+)
diff --git a/plugin/linuxsty.vim b/plugin/linuxsty.vim
index 18b2867..44824b8 100644
--- a/plugin/linuxsty.vim
+++ b/plugin/linuxsty.vim
@@ -69,6 +69,7 @@ function s:LinuxFormatting()
endfunction
function s:LinuxKeywords()
+ syn keyword cStatement fallthrough
syn keyword cOperator likely unlikely
syn keyword cType u8 u16 u32 u64 s8 s16 s32 s64
syn keyword cType __u8 __u16 __u32 __u64 __s8 __s16 __s32 __s64
On Sat, 2019-08-10 at 13:33 -0700, Joe Perches wrote:
> On Sat, 2019-08-10 at 13:18 -0700, Joe Perches wrote:
[]
> > There are classes of patches generated by scripts that have
> > no real mechanism to be applied today.
> >
> > For instance: global coccinelle scripted changes to use stracpy
> > https://lore.kernel.org/lkml/alpine.DEB.2.21.1907251747560.2494@hadrien/
> >
> > and trivial scripted changes to MAINTAINERS
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > that are basically impossible to be applied by anyone but you.
> >
> > Otherwise there are hundreds of little micro patches most of
> > which would not otherwise be applied.
> >
> > There should be some process available to get these treewide
> > or difficult to keep up-to-date and apply patches handled.
> >
> > I believe these sorts of scripted patches should ideally
> > be handled immediately before an RC1 so other trees can be
> > synchronized in the simplest way possible.
>
> Hey Stephen
>
> Question for you about a possible -next process change.
>
> Would it be reasonable to have some mechanism to script
> treewide patches to generate and apply after Andrew Morton's
> mmotm patches are applied to -next?
>
> This could allow treewide scripted patches to have
> compilation and test coverage before possibly being
> applied to Linus' tree.
>
> What would be necessary to allow this?
Ping?
Hi Joe,
Sorry for the slow response.
On Fri, 16 Aug 2019 12:58:27 -0700 Joe Perches <[email protected]> wrote:
>
> On Sat, 2019-08-10 at 13:33 -0700, Joe Perches wrote:
> > On Sat, 2019-08-10 at 13:18 -0700, Joe Perches wrote:
> []
> > > There are classes of patches generated by scripts that have
> > > no real mechanism to be applied today.
> > >
> > > For instance: global coccinelle scripted changes to use stracpy
> > > https://lore.kernel.org/lkml/alpine.DEB.2.21.1907251747560.2494@hadrien/
> > >
> > > and trivial scripted changes to MAINTAINERS
> > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > that are basically impossible to be applied by anyone but you.
> > >
> > > Otherwise there are hundreds of little micro patches most of
> > > which would not otherwise be applied.
> > >
> > > There should be some process available to get these treewide
> > > or difficult to keep up-to-date and apply patches handled.
> > >
> > > I believe these sorts of scripted patches should ideally
> > > be handled immediately before an RC1 so other trees can be
> > > synchronized in the simplest way possible.
> >
> > Hey Stephen
> >
> > Question for you about a possible -next process change.
> >
> > Would it be reasonable to have some mechanism to script
> > treewide patches to generate and apply after Andrew Morton's
> > mmotm patches are applied to -next?
I don't see why not (its all just software, right? :-)). I would have
to refresh my understanding of how Andrew constructs his mmot{s,m} quilt
series, but I should be able to sort that out. The only other issue is
the time it takes to apply these changes and test them. The total time
it takes to construct linux-next each day increases towards the opening
of the merge window (we are currently at -rc5 and I am already taking
about 12 hours each day).
> > This could allow treewide scripted patches to have
> > compilation and test coverage before possibly being
> > applied to Linus' tree.
Always a good thing :-)
So, do we have a pending example, or can you give my some idea of what
they would look like?
--
Cheers,
Stephen Rothwell
On Tue, 2019-08-20 at 09:24 +1000, Stephen Rothwell wrote:
> Hi Joe,
Hi Stephen
> Sorry for the slow response.
No worries. thanks for picking up the thread.
> On Fri, 16 Aug 2019 12:58:27 -0700 Joe Perches <[email protected]> wrote:
> > On Sat, 2019-08-10 at 13:33 -0700, Joe Perches wrote:
> > > On Sat, 2019-08-10 at 13:18 -0700, Joe Perches wrote:
> > []
> > > > There are classes of patches generated by scripts that have
> > > > no real mechanism to be applied today.
> > > >
> > > > For instance: global coccinelle scripted changes to use stracpy
> > > > https://lore.kernel.org/lkml/alpine.DEB.2.21.1907251747560.2494@hadrien/
> > > >
> > > > and trivial scripted changes to MAINTAINERS
> > > > https://lore.kernel.org/lkml/[email protected]/
> > > >
> > > > that are basically impossible to be applied by anyone but you.
> > > >
> > > > Otherwise there are hundreds of little micro patches most of
> > > > which would not otherwise be applied.
> > > >
> > > > There should be some process available to get these treewide
> > > > or difficult to keep up-to-date and apply patches handled.
> > > >
> > > > I believe these sorts of scripted patches should ideally
> > > > be handled immediately before an RC1 so other trees can be
> > > > synchronized in the simplest way possible.
> > >
> > > Hey Stephen
> > >
> > > Question for you about a possible -next process change.
> > >
> > > Would it be reasonable to have some mechanism to script
> > > treewide patches to generate and apply after Andrew Morton's
> > > mmotm patches are applied to -next?
>
> I don't see why not (its all just software, right? :-)). I would have
> to refresh my understanding of how Andrew constructs his mmot{s,m} quilt
> series, but I should be able to sort that out. The only other issue is
> the time it takes to apply these changes and test them. The total time
> it takes to construct linux-next each day increases towards the opening
> of the merge window (we are currently at -rc5 and I am already taking
> about 12 hours each day).
>
> > > This could allow treewide scripted patches to have
> > > compilation and test coverage before possibly being
> > > applied to Linus' tree.
>
> Always a good thing :-)
>
> So, do we have a pending example, or can you give my some idea of what
> they would look like?
A few examples:
1: a patch just to MAINTAINERS done via bash script:
https://lore.kernel.org/lkml/[email protected]/
$ git grep -h "^[FX]:" MAINTAINERS | \
cut -f2- | grep -vP '/$|\*|\?|\[' | \
while read file ; do \
if [ -d $file ]; then \
sed -i -e "s@${file}\$@${file}/@" MAINTAINERS ; \
fi ; \
done
This one is trivial and takes almost no time.
2: would be Julia Lawall's stracpy change done
with coccinelle: (attached)
This one takes quite a bit longer as it has to do a
cocci --all-includes scan of each source file and each
of its #include files.
The 1st MAINTAINERS change is an annoyance because it
either is individual patches for each of 50 subsystems
or a single patch that changes constantly. Either
tends to get elements dropped on the floor.
The 2nd is treewide and quite a large patch which
spans nearly every subsystem. These types of patches
generally are not always acceptable to one party or
another but do allow whatever exceptional uses of
strlcpy or strncpy that remain to be analyzed for
defects.
3: might be the /* fallthrough */ to fallthrough;
script attached to this email:
https://lore.kernel.org/lkml/[email protected]/
$ git ls-files -- '*.[ch]' | \
xargs perl cvt_style.pl -o --convert=fallthrough
but this depends on the acceptance of another currently
rfc patch:
https://lore.kernel.org/lkml/1d2830aadbe9d8151728a7df5b88528fc72a0095.1564549413.git.joe@perches.com/
This script takes around 15 minutes on my 3 year old
laptop with an ssd.
cheers, Joe
On Mon, Aug 19, 2019 at 5:08 PM Joe Perches <[email protected]> wrote:
>
> 2: would be Julia Lawall's stracpy change done
> with coccinelle: (attached)
I'm not actually convinced about stracpy() and friends.
It seems to be yet another badly thought out string interface, and
there are now so many of them that no human being can keep track of
them.
The "badly thought out" part is that it (like the original strlcpy
garbage from BSD) thinks that there is only one size that matters -
the destination.
Yes, we fixed part of the "source is also limited" with strscpy(). It
didn't fix the problem with different size limits, but at least it
fixed the fundamentally broken assumption that the source has no size
limit at all.
Honestly, I really really REALLY don't want yet another broken string
handling function, when we still have a lot of the old strlcpy() stuff
in the tree from previous broken garbage.
The fact is, when you copy strings, both the destination *AND* the
source may have size limits. They may be the same. Or they may not be.
This is particularly noticeable in the "str*_pad()" versions. It's
simply absolutely and purely wrong. I will note that we currently have
not a single user or strscpy_pad() in the whole kernel outside of the
testing code.
And yes, we actually *do* have real and present cases of "source and
destination have different sizes". They aren't common, but they do
exist.
So I'm putting my foot down on yet another broken string copy
interface from people who do not understand this fundamental issue.
Linus
On Tue, 2019-08-20 at 16:28 -0700, Linus Torvalds wrote:
> On Mon, Aug 19, 2019 at 5:08 PM Joe Perches <[email protected]> wrote:
> > 2: would be Julia Lawall's stracpy change done
> > with coccinelle: (attached)
>
> I'm not actually convinced about stracpy() and friends.
>
> It seems to be yet another badly thought out string interface, and
> there are now so many of them that no human being can keep track of
> them.
>
> The "badly thought out" part is that it (like the original strlcpy
> garbage from BSD) thinks that there is only one size that matters -
> the destination.
>
> Yes, we fixed part of the "source is also limited" with strscpy(). It
> didn't fix the problem with different size limits, but at least it
> fixed the fundamentally broken assumption that the source has no size
> limit at all.
>
> Honestly, I really really REALLY don't want yet another broken string
> handling function, when we still have a lot of the old strlcpy() stuff
> in the tree from previous broken garbage.
>
> The fact is, when you copy strings, both the destination *AND* the
> source may have size limits. They may be the same. Or they may not be.
>
> This is particularly noticeable in the "str*_pad()" versions. It's
> simply absolutely and purely wrong. I will note that we currently have
> not a single user or strscpy_pad() in the whole kernel outside of the
> testing code.
>
> And yes, we actually *do* have real and present cases of "source and
> destination have different sizes". They aren't common, but they do
> exist.
>
> So I'm putting my foot down on yet another broken string copy
> interface from people who do not understand this fundamental issue.
I think you are mistaken about the stracpy limits as
the only limit is not the source size but the dest.
Why should the source be size limited?
btw: I also think str.cpy_pad is horrible.
On Tue, 2019-08-20 at 16:28 -0700, Linus Torvalds wrote:
> On Mon, Aug 19, 2019 at 5:08 PM Joe Perches <[email protected]> wrote:
> > 2: would be Julia Lawall's stracpy change done
> > with coccinelle: (attached)
>
> I'm not actually convinced about stracpy() and friends.
>
> It seems to be yet another badly thought out string interface, and
> there are now so many of them that no human being can keep track of
> them.
>
> The "badly thought out" part is that it (like the original strlcpy
> garbage from BSD) thinks that there is only one size that matters -
> the destination.
>
> Yes, we fixed part of the "source is also limited" with strscpy(). It
> didn't fix the problem with different size limits, but at least it
> fixed the fundamentally broken assumption that the source has no size
> limit at all.
Umm, btw: have you actually looked at stracpy?
It's just a convenience wrapper around strscpy
and dest must be a char array and its size
does not need to be specified as a somewhat
useless argument otherwise prone to misuse.
#define stracpy(dest, src) \
({ \
size_t count = ARRAY_SIZE(dest); \
BUILD_BUG_ON(!(__same_type(dest, char[]) || \
__same_type(dest, unsigned char[]) || \
__same_type(dest, signed char[]))); \
\
strscpy(dest, src, count); \
})
I sent several patches for those misuses.
Hi Joe,
On Mon, 19 Aug 2019 17:08:00 -0700 Joe Perches <[email protected]> wrote:
>
> A few examples:
>
> 1: a patch just to MAINTAINERS done via bash script:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> $ git grep -h "^[FX]:" MAINTAINERS | \
> cut -f2- | grep -vP '/$|\*|\?|\[' | \
> while read file ; do \
> if [ -d $file ]; then \
> sed -i -e "s@${file}\$@${file}/@" MAINTAINERS ; \
> fi ; \
> done
>
> This one is trivial and takes almost no time.
That one seems ok (except you need "s around the $file in [ -d $file ]).
In this case, I guess the plan is that I run the script and commit the
result using the commit message and authorship from the above mail ...
(I would also replace the first three commands with
sed -En 's/^[FX]:[[:space:]]*([^[*?]*[^[*?/])$/\1/p' MAINTAINERS
/me puts away his yak razor :-))
> 2: would be Julia Lawall's stracpy change done
> with coccinelle: (attached)
>
> This one takes quite a bit longer as it has to do a
> cocci --all-includes scan of each source file and each
> of its #include files.
What do I need to apply that "patch"?
--
Cheers,
Stephen Rothwell
On Tue, Aug 20, 2019 at 4:37 PM Joe Perches <[email protected]> wrote:
>
> > So I'm putting my foot down on yet another broken string copy
> > interface from people who do not understand this fundamental issue.
>
> I think you are mistaken about the stracpy limits as
> the only limit is not the source size but the dest.
>
> Why should the source be size limited?
You just proved my point. You don't understand that sources can also
be limited, and the limit on a source can be *smaller* than the limit
of a destination.
Did we learn *NOTHING* from the complete and utter disaster that was strlcpy()?
Do you not understand why strlcpy() was unacceptably bad, and why the
people who converted strncpy() to it introduced real bugs?
The fact is, it's not just the destination that has a size limit. The
source often has one too.
And no, the source is not always guaranteed to be NUL-terminated, nor
is the source buffer guaranteed to be larger than the destination
buffer.
Now, if you *know* that the source is smaller than the destination
size, you can do:
len = strnlen(src, srclen);
memcpy(dst, len);
dst[len] = 0;
and that's not wrong, but that works only when
(a) you actually do the above
(b) you have no data races on src (or you at least only require that
'dst' is NUL-terminated, not that 'len' is necessarily the correct
length of the result
(c) you actually know as the programmer that yes, the source is
definitely smaller than the destination.
and honestly, people don't get _any_ of that right.
For one thing, the buffer sizes of the source and destination may be
two different things and some #define. It's hard to tell that one is
always smaller than the other (or that they are always the same size).
So then to get it right in the *general* case, you may need to do
something like
if (srcsize < dstsize) {
.. do the above ..
} else {
strlcpy(dst,src,dstsize);
}
do you see the reason?
Do you see why strlcpy() is only safe to do when the allocation size
of the source buffer is at least as big as the allocation size of the
destination buffer?
For example, I just grepped for some patterns, and I can find code
like this in the kernel
name_len = strnlen(fileName, PATH_MAX);
name_len++; /* trailing null */
strncpy(pSMB->fileName, fileName, name_len);
where pretty much everything is wrong. The comment is fundamentally
wrong, and even spells "nul" wrong. Christ.
Here's another one:
/* will be less than a page size */
len = strnlen(link, ocfs2_fast_symlink_chars(inode->i_sb));
kaddr = kmap_atomic(page);
memcpy(kaddr, link, len + 1);
and notice how this time at least the comment is (hopefully) correct.
But the code is wrong once again, because it doesn't actually do the
correct pattern I showed above, it does a "memcpy(len+1)" instead.
Bzzt. WRONG!
What I think the code *wants* to do is
kaddr = kmap_atomic(page);
copy_string(
// destination and destination size limit
kaddr, PAGE_SIZE,
// source and source size limit
link, ocfs2_fast_symlink_chars(inode->i_sb)
);
ie the destination has one size, and the source has another size, and
the source may or may not be NUL-terminated.
And then the programmer wouldn't have needed the comment, and wouldn't
have needed to make sure that yes, ocfs2_fast_symlink_chars() is
guaranteed to be less than PAGE_SIZE.
Again, the code we actually _have_ in the kernel is not sensible. It
doesn't actually nul-terminate the destination, despite clearly
_trying_ to (note that "len+1" in the memcpy).
Now, it's possible that it doesn't care about properly nul-terminating
things. And it's possible; that the source is always nul-terminated to
begin with unless the filesystem is corrupted. But the code clearly
_tries_ to do something, and fails.
Because copying a string is complicated, particularly when the
allocations for source and destination may be entirely different.
On a happier note, I actually found a correct code case too. Our
"kstrndup()" function seems to actually be at a first glance entirely
bug-free, and actually takes a limited source string size, and gives
you back a nul-terminated destination string of a different size. Of
course, that's a simple case, because the size of the destination is
something that that function actually controls, so getting it right is
actually somewhat simpler.
Linus
On Tue, Aug 20, 2019 at 5:20 PM Joe Perches <[email protected]> wrote:
>
> Umm, btw: have you actually looked at stracpy?
Yes, Joe, I have.
What part of "there are now so many of them that no human being can
keep track of them" didn't you see as a problem?
How many broken string functions are we going to do, adding yet
another one when you notice that the _last_ one wasn't great?
We never seem to remove the broken ones. We just add yet another one,
and have a never-ending jumble of random letters.
I would seriously suggest doing something like
copy_string( dst, dstsize, src, srcsize, FLAGS );
where FLAGS migth be "pad" or whatever. Make it return the size of the
resulting string, because while it can be convenient to pass 'dst" on,
it's not useful.
And then maybe just add the helper macro that turns an array into a
"pointer, size" combination, rather than yet another letter jumble.
Linus
On Tue, 2019-08-20 at 17:43 -0700, Linus Torvalds wrote:
> On Tue, Aug 20, 2019 at 5:20 PM Joe Perches <[email protected]> wrote:
> > Umm, btw: have you actually looked at stracpy?
>
> Yes, Joe, I have.
>
> What part of "there are now so many of them that no human being can
> keep track of them" didn't you see as a problem?
Well, the actual post-conversion uses to stracpy make the old ones
(strcpy/strlcpy/strncpy) exceptional uses that can be analyzed
quite a bit more easily.
btw: I really don't care what any convenience macro is named.
Most all of the strlcpy and strscpy uses actually _do_ copy to
a char array and strscpy is a simple interface that is somewhat
frequently misused.
> How many broken string functions are we going to do, adding yet
> another one when you notice that the _last_ one wasn't great?
>
> We never seem to remove the broken ones. We just add yet another one,
> and have a never-ending jumble of random letters.
<shrug> Intermediate problems.
> I would seriously suggest doing something like
> and
> copy_string( dst, dstsize, src, srcsize, FLAGS );
>
> where FLAGS migth be "pad" or whatever. Make it return the size of the
> resulting string, because while it can be convenient to pass 'dst" on,
> it's not useful.
That's simply not convenient for pointers.
Auditing the kernel for those unsized uses is a
large scope problem. Even anything that uses
PAGE_SIZE sized allocations does sprintf
instead of snprintf. Any show_<foo> for instance.
> And then maybe just add the helper macro that turns an array into a
> "pointer, size" combination, rather than yet another letter jumble.
Good luck with that when an unsized char pointer is the thing
passed to a function.
There are _way_ too many of those already in the kernel.
Simple strcpy is already used > 2000 times.
On Tue, Aug 20, 2019 at 05:43:27PM -0700, Linus Torvalds wrote:
> I would seriously suggest doing something like
>
> copy_string( dst, dstsize, src, srcsize, FLAGS );
>
> where FLAGS migth be "pad" or whatever. Make it return the size of the
> resulting string, because while it can be convenient to pass 'dst" on,
> it's not useful.
I actually like this a lot. FLAGS could also indicate whether or not a
zero before srcsize ends the copy or not, allowing to copy substrings
of known length or known valid strings of unknown length by passing ~0
in srcsize. And it could also indicate whether the returned value should
indicate how much was copied or how much would have been needed for the
copy to work (so that testing (result <= dstsize) indicates truncation).
> And then maybe just add the helper macro that turns an array into a
> "pointer, size" combination, rather than yet another letter jumble.
I did exactly this in some of my projects including haproxy, I called
the lib "ist" for "indirect string", and found it extremely convenient
to use because many functions now return an ist or take an ist as an
argument. Passing a structure of only two elements results in passing
only two registers, and that's the same for the return value. Moreover,
the compiler is smart enough to eliminate a *lot* of manipulations, and
to replace pointer dereferences with direct register manipulations. I
do have a lot of ist("foo") spread everywhere in the code, which makes
a struct ist from the string and its length, and when you look at the
code, the compiler used immediate values for both the string and its
length. It's also extremely convenient for string comparisons and
lookups because you start by checking the length and can eliminate
lookups and dereferences, making keyword parsers very efficient. It
also allows us to have an istcat() function doing like strncat() but
with the output size always known so that there's no risk of appending
past the end when the starting point doesn't match the beginning of a
string.
I must confess that I became quite addict to using this because it's
so much convenient not to have to care about string length nor zero
termination anymore, without the overhead of calling strlen() on
resulting values!
For illustration of the simplicity the code is here :
http://git.haproxy.org/?p=haproxy.git;a=blob_plain;f=include/common/ist.h
And here are a few examples of usage:
- declaration in arrays:
http://git.haproxy.org/?p=haproxy.git;a=blob;f=contrib/prometheus-exporter/service-prometheus.c;h=9b9ef2ea8e2e8ee0cc63364500d39fc08009fb8d;hb=HEAD#l644
- appending to a string:
http://git.haproxy.org/?p=haproxy.git;a=blob;f=contrib/prometheus-exporter/service-prometheus.c;h=9b9ef2ea8e2e8ee0cc63364500d39fc08009fb8d;hb=HEAD#l1112
- passing as function arguments:
http://git.haproxy.org/?p=haproxy.git;a=blob;f=src/http_ana.c;h=b2069e3ead59e7bcde45ac76a1c6b0b6b5fb3882;hb=HEAD#l2468
http://git.haproxy.org/?p=haproxy.git;a=blob;f=src/http_ana.c;h=b2069e3ead59e7bcde45ac76a1c6b0b6b5fb3882;hb=HEAD#l2602
- checking for known values:
http://git.haproxy.org/?p=haproxy.git;a=blob;f=src/h2.c;h=c41da8e5ee116e75e4719709527511c299a3657c;hb=HEAD#l295
I'm personally totally convinced by this approach and am slowly improving
this interface to progressively use it everywhere, and quite frankly I
can only strongly recommend going into the same direction for ease of
use, safety, and efficiency.
Willy
Hi Linus,
On Wed, Aug 21, 2019 at 2:41 AM Linus Torvalds
<[email protected]> wrote:
> On Tue, Aug 20, 2019 at 4:37 PM Joe Perches <[email protected]> wrote:
> > > So I'm putting my foot down on yet another broken string copy
> > > interface from people who do not understand this fundamental issue.
> >
> > I think you are mistaken about the stracpy limits as
> > the only limit is not the source size but the dest.
> >
> > Why should the source be size limited?
>
> You just proved my point. You don't understand that sources can also
> be limited, and the limit on a source can be *smaller* than the limit
> of a destination.
>
> Did we learn *NOTHING* from the complete and utter disaster that was strlcpy()?
>
> Do you not understand why strlcpy() was unacceptably bad, and why the
> people who converted strncpy() to it introduced real bugs?
>
> The fact is, it's not just the destination that has a size limit. The
> source often has one too.
>
> And no, the source is not always guaranteed to be NUL-terminated, nor
> is the source buffer guaranteed to be larger than the destination
> buffer.
>
> Now, if you *know* that the source is smaller than the destination
> size, you can do:
>
> len = strnlen(src, srclen);
> memcpy(dst, len);
> dst[len] = 0;
>
> and that's not wrong, but that works only when
>
> (a) you actually do the above
>
> (b) you have no data races on src (or you at least only require that
> 'dst' is NUL-terminated, not that 'len' is necessarily the correct
> length of the result
>
> (c) you actually know as the programmer that yes, the source is
> definitely smaller than the destination.
>
> and honestly, people don't get _any_ of that right.
(d) you know the untouched trailing end of dst[] does not leak data.
Anything else we're missing?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds