[PATCH 0/3] teach sparse about arch specific asm constraints

[PATCH 0/3] teach sparse about arch specific asm constraints

[Luc Van Oostenryck]

When evaluating asm operands it must be known if they correspond
to a memory operand or not. This is done by looking at the constraint.
In most case it's enough to look at the common constraints but
PPC & S390 have some specific constraints for memory operands.
Other kinds of operands may also need arch-specific constraints.
    
This series adds support for processing arch-specific constraints
and let sparse know about PPC & S390 constraints for memory operands.


Luc Van Oostenryck (3):
  add support for arch specific asm constraints
  add memory asm constraint for PPC
  add memory asm constraint for S390

 evaluate.c    |  5 ++++-
 target-ppc.c  | 13 +++++++++++++
 target-s390.c | 13 +++++++++++++
 target.h      |  3 ++-
 4 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.27.0

[PATCH 1/3] add support for arch specific asm constraints

[Luc Van Oostenryck]

When evaluating asm operands it must be known if they correspond
to a memory operand or not in order to process/ignore the 'noderef'
attribute.

This is done for operands specified with the common constraints
but not for the machine specific constraints.

So, add support for processing machine specific constraints.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c | 5 ++++-
 target.h   | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 3b9aec3c13c0..f515ce6f2de6 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -3642,7 +3642,10 @@ static void parse_asm_constraint(struct asm_operand *op)
 			return;
 
 		default:
-			// FIXME: arch-specific (and multi-letter) constraints
+			if (arch_target->asm_constraint)
+				str = arch_target->asm_constraint(op, c, str);
+
+			// FIXME: multi-letter constraints
 			break;
 		}
 	}
diff --git a/target.h b/target.h
index 8640026cc6d4..3ef0d520f83c 100644
--- a/target.h
+++ b/target.h
@@ -53,7 +53,7 @@ extern int pointer_alignment;
 extern int bits_in_enum;
 extern int enum_alignment;
 

[PATCH 1/3] add support for arch specific asm constraints

[Luc Van Oostenryck]

When evaluating asm operands it must be known if they correspond
to a memory operand or not in order to process/ignore the 'noderef'
attribute.

This is done for operands specified with the common constraints
but not for the machine specific constraints.

So, add support for processing machine specific constraints.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c | 5 ++++-
 target.h   | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 3b9aec3c13c0..f515ce6f2de6 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -3642,7 +3642,10 @@ static void parse_asm_constraint(struct asm_operand *op)
 			return;
 
 		default:
-			// FIXME: arch-specific (and multi-letter) constraints
+			if (arch_target->asm_constraint)
+				str = arch_target->asm_constraint(op, c, str);
+
+			// FIXME: multi-letter constraints
 			break;
 		}
 	}
diff --git a/target.h b/target.h
index 8640026cc6d4..3ef0d520f83c 100644
--- a/target.h
+++ b/target.h
@@ -53,7 +53,7 @@ extern int pointer_alignment;
 extern int bits_in_enum;
 extern int enum_alignment;
 
-
+struct asm_operand;
 struct builtin_fn;
 
 struct target {
@@ -77,6 +77,7 @@ struct target {
 
 	void (*init)(const struct target *self);
 	void (*predefine)(const struct target *self);
+	const char *(*asm_constraint)(struct asm_operand *op, int c, const char *str);
 };
 
 extern const struct target target_default;
-- 
2.27.0

[PATCH 2/3] add memory asm constraint for PPC

[Luc Van Oostenryck]

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(+)

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,
 };
-- 
2.27.0

Re: [PATCH 2/3] add memory asm constraint for PPC

[Ramsay Jones]

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,
>  };
> 

Re: [PATCH 2/3] add memory asm constraint for PPC

[Luc Van Oostenryck]

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

Re: [PATCH 2/3] add memory asm constraint for PPC

[Ramsay Jones]

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

Re: [PATCH 2/3] add memory asm constraint for PPC

[Luc Van Oostenryck]

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

[PATCH 3/3] add memory asm constraint for S390

[Luc Van Oostenryck]

The 'Q', 'R', 'S' & 'T'' asm constraint are used for for memory
operands on S390 but only 'Q' belong to the 'common constraints'.

Fix this by handling the 3 others with an arch-specific method.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 target-s390.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/target-s390.c b/target-s390.c
index 8fe7d93677b2..9dbc810e507c 100644
--- a/target-s390.c
+++ b/target-s390.c
@@ -1,6 +1,7 @@
 #include "symbol.h"
 #include "target.h"
 #include "machine.h"
+#include "expression.h"
 
 
 static void predefine_s390(const struct target *self)
@@ -8,6 +9,16 @@ static void predefine_s390(const struct target *self)
 	predefine("__s390__", 1, "1");
 }
 
+static const char *asm_constraint_s390(struct asm_operand *op, int c, const char *str)
+{
+	switch (c) {
+	case 'R': case 'S': case 'T':
+		op->is_memory = true;
+		break;
+	}
+	return str;
+}
+
 const struct target target_s390 = {
 	.mach = MACH_S390,
 	.bitness = ARCH_LP32,
@@ -21,6 +32,7 @@ const struct target target_s390 = {
 	.target_64bit = &target_s390x,
 
 	.predefine = predefine_s390,
+	.asm_constraint = asm_constraint_s390,
 };
 
 
@@ -45,4 +57,5 @@ const struct target target_s390x = {
 	.target_32bit = &target_s390,
 
 	.predefine = predefine_s390x,
+	.asm_constraint = asm_constraint_s390,
 };
-- 
2.27.0