Re: rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)QUILT
Hi Linus, On Wed, Aug 21, 2019 at 2:41 AM Linus Torvalds wrote: > On Tue, Aug 20, 2019 at 4:37 PM Joe Perches 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 -- ge...@linux-m68k.org 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
Re: rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)QUILT
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
Re: rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)QUILT
On Tue, Aug 20, 2019 at 5:20 PM Joe Perches 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
Re: rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)QUILT
On Tue, Aug 20, 2019 at 4:37 PM Joe Perches 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
Re: rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)QUILT
Hi Joe, On Mon, 19 Aug 2019 17:08:00 -0700 Joe Perches wrote: > > A few examples: > > 1: a patch just to MAINTAINERS done via bash script: > > https://lore.kernel.org/lkml/904551f1f198ffac9a0f9c3c99aa966b0a7c76c1.ca...@perches.com/ > > $ 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 pgppSDuQZCMsc.pgp Description: OpenPGP digital signature
Re: rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)QUILT
On Tue, 2019-08-20 at 16:28 -0700, Linus Torvalds wrote: > On Mon, Aug 19, 2019 at 5:08 PM Joe Perches 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.
Re: rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)QUILT
On Tue, 2019-08-20 at 16:28 -0700, Linus Torvalds wrote: > On Mon, Aug 19, 2019 at 5:08 PM Joe Perches 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.
Re: rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)QUILT
On Mon, Aug 19, 2019 at 5:08 PM Joe Perches 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
Re: rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)QUILT
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 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/6482e6546dc328ec47b07dba9a78a9573ebb3e56.ca...@perches.com/ > > > > > > > > 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/904551f1f198ffac9a0f9c3c99aa966b0a7c76c1.ca...@perches.com/ $ 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/61ddbb86d5e68a15e24ccb06d9b399bbf5ce2da7.ca...@perches.com/ $ 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@perches.com/ This script takes around 15 minutes on my 3 year old laptop with an ssd. cheers, Joe // spatch.opt -j 44 ~/linux-next stracpy.cocci --recursive-includes --very-quiet > stracpy.out @r@ identifier f,i1,i2; struct i1 e1; expression e2; position p; @@ \(strscpy\|strlcpy\)(e1.f, e2, i2)@p @ok@ identifier r.i1,r.i2,r.f; type T; @@ struct i1 { ... T f[i2]; ... } @depends on ok@ identifier f,i2,i1; struct i1 e1; expression e2; local idexpression x; position r.p; assignment operator aop; @@ ( -x aop strlcpy +stracpy (e1.f, e2 -, i2 )@p; ... when != x | -strlcpy +stracpy (e1.f, e2 -, i2 )@p; | -strscpy +stracpy (e1.f, e2 -, i2 )@p ... when any ) diff -u -p a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c --- a/drivers/gpu/drm/drm_client.c +++ b/drivers/gpu/drm/drm_client.c @@ -347,7 +347,7 @@ static int drm_client_buffer_addfb(struc /* drop the reference we picked up in framebuffer lookup */ drm_framebuffer_put(buffer->fb); - strscpy(buffer->fb->comm, client->name, TASK_COMM_LEN); + stracpy(buffer->fb->comm, client->name); return 0; } diff -u -p a/drivers/hwmon/max6639.c
Re: rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)QUILT
Hi Joe, Sorry for the slow response. On Fri, 16 Aug 2019 12:58:27 -0700 Joe Perches 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/6482e6546dc328ec47b07dba9a78a9573ebb3e56.ca...@perches.com/ > > > > > > 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 pgpRN1fZfC9rb.pgp Description: OpenPGP digital signature
rfc: treewide scripted patch mechanism? (was: Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang)
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/6482e6546dc328ec47b07dba9a78a9573ebb3e56.ca...@perches.com/ > > > > 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?
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
(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") > (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
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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") (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.
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
On Mon, Aug 12, 2019 at 6:29 PM Nick Desaulniers wrote: > > On Sat, Aug 10, 2019 at 8:06 PM 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]] > > 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
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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 [[]] 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__(()) style for gcc compatibility. This is just clang supporting a new 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.
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
On Sat, Aug 10, 2019 at 8:06 PM 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]] 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
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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 | \ 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;
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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 : be...@petrovitsch.priv.at LUGA : http://www.luga.at
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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.
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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?
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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 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/6482e6546dc328ec47b07dba9a78a9573ebb3e56.ca...@perches.com/ > > 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
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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 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/6482e6546dc328ec47b07dba9a78a9573ebb3e56.ca...@perches.com/ > > 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?
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
On Sat, 2019-08-10 at 12:44 -0700, Linus Torvalds wrote: > On Sat, Aug 10, 2019 at 12:32 PM Joe Perches 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/6482e6546dc328ec47b07dba9a78a9573ebb3e56.ca...@perches.com/ 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.
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
On Sat, Aug 10, 2019 at 12:32 PM Joe Perches 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
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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 > --- > 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 >
Re: [PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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 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 Tested-by: Nathan Chancellor
[PATCH] Makefile: Convert -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang
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 --- 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