| Message ID | 20200704135747.87752-3-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | teach sparse about arch specific asm constraints | expand |
Hi Luc, All of the patches in this series look good to me! ;-) (I have been following all patches on the list, I just haven't had anything to say about them - which is a good thing!) On 04/07/2020 14:57, Luc Van Oostenryck wrote: > The 'Z' asm constraint is used for doing IO accessors on PPC but > isn't part of the 'common constraints'. It's responsible for > more than half of all warnings (with defconfig + allyesconfig). Not a problem, but this made me think 'half of which warnings'. :-D I assume, but it's just a guess, this means 'half of all asm-constraints warnings on the kernel PPC build'. How many warnings is that? What percentage is that of _all_ sparse warnings on a typical kernel build? Thanks! [BTW, I also noticed the (long running) 'luc/options' branch, which looks like it could prove to be a nice cleanup - I've only read the commit messages, not the actual commits.] ATB, Ramsay Jones > > Fix this by handling this constraint in a specific method for PPC. > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > --- > target-ppc.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/target-ppc.c b/target-ppc.c > index b138635b9103..c0d6068f436a 100644 > --- a/target-ppc.c > +++ b/target-ppc.c > @@ -1,6 +1,7 @@ > #include "symbol.h" > #include "target.h" > #include "machine.h" > +#include "expression.h" > > > static void predefine_ppc(const struct target *self) > @@ -15,6 +16,16 @@ static void predefine_ppc(const struct target *self) > predefine("_BIG_ENDIAN", 1, "1"); > } > > +static const char *asm_constraint_ppc(struct asm_operand *op, int c, const char *str) > +{ > + switch (c) { > + case 'Z': > + op->is_memory = true; > + break; > + } > + return str; > +} > + > > static void predefine_ppc32(const struct target *self) > { > @@ -32,6 +43,7 @@ const struct target target_ppc32 = { > .target_64bit = &target_ppc64, > > .predefine = predefine_ppc32, > + .asm_constraint = asm_constraint_ppc, > }; > > > @@ -55,4 +67,5 @@ const struct target target_ppc64 = { > .target_32bit = &target_ppc32, > > .predefine = predefine_ppc64, > + .asm_constraint = asm_constraint_ppc, > }; >
On Sat, Jul 04, 2020 at 06:44:53PM +0100, Ramsay Jones wrote: > > Hi Luc, > > All of the patches in this series look good to me! ;-) > > (I have been following all patches on the list, I just > haven't had anything to say about them - which is a good thing!) Hi, Yes, indeed. Thank you! > On 04/07/2020 14:57, Luc Van Oostenryck wrote: > > The 'Z' asm constraint is used for doing IO accessors on PPC but > > isn't part of the 'common constraints'. It's responsible for > > more than half of all warnings (with defconfig + allyesconfig). > > Not a problem, but this made me think 'half of which warnings'. :-D > I assume, but it's just a guess, this means 'half of all asm-constraints > warnings on the kernel PPC build'. > > How many warnings is that? What percentage is that of _all_ sparse > warnings on a typical kernel build? It's literally more than half of all warnings issued by sparse when doing a build of the kernel with 'defconfig' and another one with 'allyesconfig' (all my tests on kernel builds are done like so) on a ppc64 machine: $ grep ': \(error\|warning\):' log-master-master | wc -l 138581 $ grep ': \(error\|warning\):' log-arch-asm-mem | wc -l 50006 So, this series eliminates about 64% of all warnings, a nice improvement of the S/N ratio. Now, most of these warnings are exactly the same, again and again: $ diff -U1 log-master-master log-arch-asm-mem --- log-master-master 2020-07-04 14:40:56.733110303 +0200 +++ log-arch-asm-mem 2020-07-04 15:46:47.972595182 +0200 @@ -1 +1 @@ -SPARSE=fa1539620 +SPARSE=69fedae3e @@ -24,8 +23,0 @@ -./arch/powerpc/include/asm/io.h:143:1: warning: dereference of noderef expression -./arch/powerpc/include/asm/io.h:144:1: warning: dereference of noderef expression -./arch/powerpc/include/asm/io.h:148:1: warning: dereference of noderef expression -./arch/powerpc/include/asm/io.h:149:1: warning: dereference of noderef expression -./arch/powerpc/include/asm/io.h:143:1: warning: dereference of noderef expression -./arch/powerpc/include/asm/io.h:144:1: warning: dereference of noderef expression -./arch/powerpc/include/asm/io.h:148:1: warning: dereference of noderef expression -./arch/powerpc/include/asm/io.h:149:1: warning: dereference of noderef expression @@ -41 +32,0 @@ -./arch/powerpc/include/asm/io.h:144:1: warning: dereference of noderef expression @@ -56,4 +46,0 @@ -./arch/powerpc/include/asm/io.h:143:1: warning: dereference of noderef expression -./arch/powerpc/include/asm/io.h:144:1: warning: dereference of noderef expression -./arch/powerpc/include/asm/io.h:148:1: warning: dereference of noderef expression -./arch/powerpc/include/asm/io.h:149:1: warning: dereference of noderef expression ... If you count only unique warnings, the result is quite different: @gcc203:~/dev $ grep ': \(error\|warning\):' log-master-master | sort -u | wc -l 19071 @gcc203:~/dev $ grep ': \(error\|warning\):' log-arch-asm-mem | sort -u | wc -l 19067 Only 4 unique warnings are eliminated. For comparison, on x86_64 the warnings are: $ grep ': \(error\|warning\):' log-master-master | wc -l 29806 $ grep ': \(error\|warning\):' log-master-master | sort -u | wc -l 19234 > Thanks! > > [BTW, I also noticed the (long running) 'luc/options' branch, which > looks like it could prove to be a nice cleanup - I've only read the > commit messages, not the actual commits.] Yes, I think it's a nice cleanup because this code is quite messy but, OTOH, moving around all this code break all its history (via 'git blame' or 'git log -L') is is guaranteed to create really nasty conflicts with anything touching the code for the options. This is really a downside. Best regards, -- Luc
On 04/07/2020 20:32, Luc Van Oostenryck wrote: > On Sat, Jul 04, 2020 at 06:44:53PM +0100, Ramsay Jones wrote: >> On 04/07/2020 14:57, Luc Van Oostenryck wrote: >>> The 'Z' asm constraint is used for doing IO accessors on PPC but >>> isn't part of the 'common constraints'. It's responsible for >>> more than half of all warnings (with defconfig + allyesconfig). >> >> Not a problem, but this made me think 'half of which warnings'. :-D >> I assume, but it's just a guess, this means 'half of all asm-constraints >> warnings on the kernel PPC build'. >> >> How many warnings is that? What percentage is that of _all_ sparse >> warnings on a typical kernel build? > > It's literally more than half of all warnings issued by sparse when doing > a build of the kernel with 'defconfig' and another one with 'allyesconfig' > (all my tests on kernel builds are done like so) on a ppc64 machine: > $ grep ': \(error\|warning\):' log-master-master | wc -l > 138581 > $ grep ': \(error\|warning\):' log-arch-asm-mem | wc -l > 50006 > So, this series eliminates about 64% of all warnings, a nice > improvement of the S/N ratio. Oh, nice! It's a _long_ time since I last built the kernel (about when reading Greg's 'Linux Kernel in a Nutshell' book, so about 2006), and I don't even recall if I ran sparse over it. (Hmm, did you have to specify a C parameter to make or something?). Anyway, that is still a very large number of error/warnings - has it always been that bad? >> [BTW, I also noticed the (long running) 'luc/options' branch, which >> looks like it could prove to be a nice cleanup - I've only read the >> commit messages, not the actual commits.] > > Yes, I think it's a nice cleanup because this code is quite messy but, > OTOH, moving around all this code break all its history (via 'git blame' > or 'git log -L') is is guaranteed to create really nasty conflicts with > anything touching the code for the options. This is really a downside. Yep, I know what you mean. However, I don't think you should shy away from clean-ups for too long - it will be counter-productive in the end. ATB, Ramsay Jones
On Sat, Jul 04, 2020 at 10:07:15PM +0100, Ramsay Jones wrote: > > > On 04/07/2020 20:32, Luc Van Oostenryck wrote: > > On Sat, Jul 04, 2020 at 06:44:53PM +0100, Ramsay Jones wrote: > > >> On 04/07/2020 14:57, Luc Van Oostenryck wrote: > >>> The 'Z' asm constraint is used for doing IO accessors on PPC but > >>> isn't part of the 'common constraints'. It's responsible for > >>> more than half of all warnings (with defconfig + allyesconfig). > >> > >> Not a problem, but this made me think 'half of which warnings'. :-D > >> I assume, but it's just a guess, this means 'half of all asm-constraints > >> warnings on the kernel PPC build'. > >> > >> How many warnings is that? What percentage is that of _all_ sparse > >> warnings on a typical kernel build? > > > > It's literally more than half of all warnings issued by sparse when doing > > a build of the kernel with 'defconfig' and another one with 'allyesconfig' > > (all my tests on kernel builds are done like so) on a ppc64 machine: > > $ grep ': \(error\|warning\):' log-master-master | wc -l > > 138581 > > $ grep ': \(error\|warning\):' log-arch-asm-mem | wc -l > > 50006 > > So, this series eliminates about 64% of all warnings, a nice > > improvement of the S/N ratio. > > Oh, nice! > > It's a _long_ time since I last built the kernel (about when reading > Greg's 'Linux Kernel in a Nutshell' book, so about 2006), and I don't > even recall if I ran sparse over it. (Hmm, did you have to specify > a C parameter to make or something?). Yes, the simplest is to use 'make C=2'. > Anyway, that is still a very large number of error/warnings - has it > always been that bad? Well, I like to think that it has improved considerably the last years. But yes, it's still very much. The problem is not very simple and has, I think, multiple causes, like (in random order): * not all developers use sparse * some don't really know about it * some don't want to use it anymore * support for architectures other than i386/x86_64 was very weak * sparse used to (and still) issue a lot of false negatives * documentation for sparse is very ... sparse * sparse's warnings are not always easy to understand * developers doesn't always seem to know/understand well the kind of checks sparse is doing, what's it is possible, what it is capable (I'm thinking of the address spaces, the context/locking and bitwise). * it's not always obvious what is the value of sparse warnings * it's not always obvious what is the value of fixing sparse warnings * fixing sparse warnings in the kernel is not very gratifying * devs are usually paid (and like) to add features, not to touch to code that seems to work. * very strict typing, like created with __bitwise, sometimes requires a lot of interfaces variant (one for each type/arguments/return value) otherwise functionally identical and C has essentially no support for generic programming (everything must be done with macros & typeof). * for several reasons, it can very quickly become very frustrating to make (series of) patches that fix sparse warnings in the kernel. OTOH, on x86, the vast majority of warnings I see are totally valid ones. By far, the most common ones are those involving __bitwise, then missing static, then problems with address space (especially with __rcu), then with contexts. The remaining ones are drops of water in the ocean. Also, the kernel has about 30 millions lines of code, thousands of devs, hundreds of new devs every releases, ... So, maybe it's not so bad. Finally, CI/bot testing/building has progressed a lot lately and have a very positive effect on sparse warnings (it can also be quite frustrating because every day you see automatic reports, take a quick look, think "oh, it shouldn't be hard to fix it" but then it's "oh, well ..." because you have absolutely not the bandwidth for it). > Yep, I know what you mean. However, I don't think you should shy > away from clean-ups for too long - it will be counter-productive > in the end. Yes. I generally do this on a kind of "on demand basis". When I need to touch some code and it's too hard to change or follow, I first reorganize or simplify it. Sometimes it's really needed, sometimes it's only a kind of nice side-effect, like here (I have 3 incoming series on top of it). -- Luc
diff --git a/target-ppc.c b/target-ppc.c index b138635b9103..c0d6068f436a 100644 --- a/target-ppc.c +++ b/target-ppc.c @@ -1,6 +1,7 @@ #include "symbol.h" #include "target.h" #include "machine.h" +#include "expression.h" static void predefine_ppc(const struct target *self) @@ -15,6 +16,16 @@ static void predefine_ppc(const struct target *self) predefine("_BIG_ENDIAN", 1, "1"); } +static const char *asm_constraint_ppc(struct asm_operand *op, int c, const char *str) +{ + switch (c) { + case 'Z': + op->is_memory = true; + break; + } + return str; +} + static void predefine_ppc32(const struct target *self) { @@ -32,6 +43,7 @@ const struct target target_ppc32 = { .target_64bit = &target_ppc64, .predefine = predefine_ppc32, + .asm_constraint = asm_constraint_ppc, }; @@ -55,4 +67,5 @@ const struct target target_ppc64 = { .target_32bit = &target_ppc32, .predefine = predefine_ppc64, + .asm_constraint = asm_constraint_ppc, };
The 'Z' asm constraint is used for doing IO accessors on PPC but isn't part of the 'common constraints'. It's responsible for more than half of all warnings (with defconfig + allyesconfig). Fix this by handling this constraint in a specific method for PPC. Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- target-ppc.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)