print address space number for cast-from-AS warnings

Commit Message

Luc Van Oostenryck Sept. 6, 2018, 10:30 p.m. UTC

From: Vincenzo Frascino <[e-mail]>

This patch prints the address space number when a warning
"cast removes address space of expression" is triggered.

This makes easier to discriminate in between different address
spaces.

Signed-off-by: Vincenzo Frascino <[e-mail]>
Signed-off-by: Luc Van Oostenryck <[e-mail]>
---
 evaluate.c                           |  2 +-
 validation/Waddress-space-all-attr.c | 84 ++++++++++++++++++++++++++++
 validation/Waddress-space-strict.c   | 10 ++--
 3 files changed, 90 insertions(+), 6 deletions(-)
 create mode 100644 validation/Waddress-space-all-attr.c

Comments

Linus Torvalds Sept. 6, 2018, 10:38 p.m. UTC | #1

On Thu, Sep 6, 2018 at 3:30 PM Luc Van Oostenryck
<[e-mail]> wrote:
>
> This makes easier to discriminate in between different address
> spaces.

I think it also makes it obvious how much better it would be if the
address space number was a string, not a number.

Oh well.

                Linus

Luc Van Oostenryck Sept. 6, 2018, 10:52 p.m. UTC | #2

On Thu, Sep 06, 2018 at 03:38:20PM -0700, Linus Torvalds wrote:
> On Thu, Sep 6, 2018 at 3:30 PM Luc Van Oostenryck
> <[e-mail]> wrote:
> >
> > This makes easier to discriminate in between different address
> > spaces.
> 
> I think it also makes it obvious how much better it would be if the
> address space number was a string, not a number.

Oh yes. I said essentially the same to Vincenzo a few hours ago
in the KHWASAN thread.

I suppose that hard-coding these names wouldn't be too bad.
Otherwise, I only see something like:
	#pragma sparse address_space_name(1, "user")
to add to "linux/compiler_types.h". It would be more flexible
but is a bit ugly.

-- Luc

Linus Torvalds Sept. 6, 2018, 11:01 p.m. UTC | #3

On Thu, Sep 6, 2018 at 3:52 PM Luc Van Oostenryck
<[e-mail]> wrote:
>
> I suppose that hard-coding these names wouldn't be too bad.
> Otherwise, I only see something like:

I was thinking that we could do something like actually change sparse
to track a token instead of a number for the address space, and then
we'd just make the kernel eventually switch to

  #define __user  __attribute__((noderef, address_space(user)))

and the address space would just magically change from '1' to 'user'.

The sparse validation tests would then just use a token name, and you
coulkd just have a warning like

   warning: cast removes 'user' address space of expression

and yeah, the kernel warnings would look odd for a while ("cast
removes '1' address space of expression"), but we'd know that they
eventually would become more legible as we start relying on a newer
version of sparse.

Maybe it's not a big deal, and maybe it's too painful to change the
asn from an integer to a token pointer. I didn't even look at that.

                     Linus

Luc Van Oostenryck Sept. 7, 2018, 12:15 a.m. UTC | #4

On Thu, Sep 06, 2018 at 04:01:28PM -0700, Linus Torvalds wrote:
> On Thu, Sep 6, 2018 at 3:52 PM Luc Van Oostenryck
> <[e-mail]> wrote:
> >
> > I suppose that hard-coding these names wouldn't be too bad.
> > Otherwise, I only see something like:
> 
> I was thinking that we could do something like actually change sparse
> to track a token instead of a number for the address space, and then
> we'd just make the kernel eventually switch to
> 
>   #define __user  __attribute__((noderef, address_space(user)))
> 
> and the address space would just magically change from '1' to 'user'.
> 
> The sparse validation tests would then just use a token name, and you
> coulkd just have a warning like
> 
>    warning: cast removes 'user' address space of expression
> 
> and yeah, the kernel warnings would look odd for a while ("cast
> removes '1' address space of expression"), but we'd know that they
> eventually would become more legible as we start relying on a newer
> version of sparse.
> 
> Maybe it's not a big deal, and maybe it's too painful to change the
> asn from an integer to a token pointer. I didn't even look at that.

Yes, that would also be possible, of course.
I think it would be a good thing to have a name in the message.

It shouldn't be a big problem to deal with a pointer (most uses would 
be unchanged, a few places use things like 'as1 |= sym->ctype.as;'
which will need a bit more care).

OTOH, this would destroy one of my patch to shrink the size of
struct ctype (worth maybe 0.5% of perf), though. I'm also a bit
annoyed about this transition period. I'll think about it.

-- Luc

Patch

diff --git a/evaluate.c b/evaluate.c
index 6d5d4793f..9c2304a7e 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -3018,7 +3018,7 @@  static struct symbol *evaluate_cast(struct expression *expr)
 	}
 
 	if (!tas && sas > 0)
-		warning(expr->pos, "cast removes address space of expression");
+		warning(expr->pos, "cast removes address space of expression (<asn:%d>)", sas);
 	if (tas > 0 && sas > 0 && tas != sas)
 		warning(expr->pos, "cast between address spaces (<asn:%d>-><asn:%d>)", sas, tas);
 	if (tas > 0 && !sas &&
diff --git a/validation/Waddress-space-all-attr.c b/validation/Waddress-space-all-attr.c
new file mode 100644
index 000000000..be60451a1
--- /dev/null
+++ b/validation/Waddress-space-all-attr.c
@@ -0,0 +1,84 @@ 
+/* Resembles include/linux/compiler_types.h */
+#define __kernel __attribute__((address_space(0)))
+#define __user __attribute__((address_space(1)))
+#define __iomem __attribute__((address_space(2)))
+#define __percpu __attribute__((address_space(3)))
+#define __rcu __attribute__((address_space(4)))
+
+
+typedef unsigned long ulong;
+typedef long long llong;
+typedef struct s obj_t;
+
+static void expl(obj_t __kernel *k, obj_t __iomem *o,
+		 obj_t __user *p, obj_t __percpu *pc,
+		 obj_t __rcu *r)
+{
+	(int)(k);
+	(ulong)(k);
+	(llong)(k);
+	(void *)(k);
+	(obj_t*)(k);
+	(obj_t __kernel*)(k);
+
+	(int)(o);
+	(ulong)(o);
+	(llong)(o);
+	(void *)(o);
+	(obj_t*)(o);
+	(obj_t __iomem*)(o);
+
+	(int)(p);
+	(ulong)(p);
+	(llong)(p);
+	(void *)(p);
+	(obj_t*)(p);
+	(obj_t __user*)(p);
+
+	(int)(pc);
+	(ulong)(pc);
+	(llong)(pc);
+	(void *)(pc);
+	(obj_t*)(pc);
+	(obj_t __percpu*)(pc);
+
+	(int)(r);
+	(ulong)(r);
+	(llong)(r);
+	(void *)(r);
+	(obj_t*)(r);
+	(obj_t __rcu*)(r);
+}
+
+/*
+ * check-name: Waddress-space-all-attr
+ * check-command: sparse -Wcast-from-as -Wcast-to-as $file
+ *
+ * check-error-start
+Waddress-space-all-attr.c:24:10: warning: cast removes address space of expression (<asn:2>)
+Waddress-space-all-attr.c:25:10: warning: cast removes address space of expression (<asn:2>)
+Waddress-space-all-attr.c:26:10: warning: cast removes address space of expression (<asn:2>)
+Waddress-space-all-attr.c:27:10: warning: cast removes address space of expression (<asn:2>)
+Waddress-space-all-attr.c:28:10: warning: cast removes address space of expression (<asn:2>)
+Waddress-space-all-attr.c:31:10: warning: cast removes address space of expression (<asn:1>)
+Waddress-space-all-attr.c:32:10: warning: cast removes address space of expression (<asn:1>)
+Waddress-space-all-attr.c:33:10: warning: cast removes address space of expression (<asn:1>)
+Waddress-space-all-attr.c:34:10: warning: cast removes address space of expression (<asn:1>)
+Waddress-space-all-attr.c:35:10: warning: cast removes address space of expression (<asn:1>)
+Waddress-space-all-attr.c:38:10: warning: cast removes address space of expression (<asn:3>)
+Waddress-space-all-attr.c:39:10: warning: cast removes address space of expression (<asn:3>)
+Waddress-space-all-attr.c:40:10: warning: cast removes address space of expression (<asn:3>)
+Waddress-space-all-attr.c:41:10: warning: cast removes address space of expression (<asn:3>)
+Waddress-space-all-attr.c:42:10: warning: cast removes address space of expression (<asn:3>)
+Waddress-space-all-attr.c:45:10: warning: cast removes address space of expression (<asn:4>)
+Waddress-space-all-attr.c:46:10: warning: cast removes address space of expression (<asn:4>)
+Waddress-space-all-attr.c:47:10: warning: cast removes address space of expression (<asn:4>)
+Waddress-space-all-attr.c:48:10: warning: cast removes address space of expression (<asn:4>)
+Waddress-space-all-attr.c:49:10: warning: cast removes address space of expression (<asn:4>)
+Waddress-space-all-attr.c:17:15: warning: non size-preserving pointer to integer cast
+Waddress-space-all-attr.c:24:15: warning: non size-preserving pointer to integer cast
+Waddress-space-all-attr.c:31:15: warning: non size-preserving pointer to integer cast
+Waddress-space-all-attr.c:38:15: warning: non size-preserving pointer to integer cast
+Waddress-space-all-attr.c:45:15: warning: non size-preserving pointer to integer cast
+ * check-error-end
+ */
diff --git a/validation/Waddress-space-strict.c b/validation/Waddress-space-strict.c
index 5071aab26..4ea20fae6 100644
--- a/validation/Waddress-space-strict.c
+++ b/validation/Waddress-space-strict.c
@@ -43,11 +43,11 @@  Waddress-space-strict.c:13:10: warning: cast adds address space to expression (<
 Waddress-space-strict.c:16:10: warning: cast adds address space to expression (<asn:1>)
 Waddress-space-strict.c:19:10: warning: cast adds address space to expression (<asn:1>)
 Waddress-space-strict.c:26:10: warning: cast adds address space to expression (<asn:1>)
-Waddress-space-strict.c:28:10: warning: cast removes address space of expression
-Waddress-space-strict.c:29:10: warning: cast removes address space of expression
-Waddress-space-strict.c:30:10: warning: cast removes address space of expression
-Waddress-space-strict.c:31:10: warning: cast removes address space of expression
-Waddress-space-strict.c:32:10: warning: cast removes address space of expression
+Waddress-space-strict.c:28:10: warning: cast removes address space of expression (<asn:1>)
+Waddress-space-strict.c:29:10: warning: cast removes address space of expression (<asn:1>)
+Waddress-space-strict.c:30:10: warning: cast removes address space of expression (<asn:1>)
+Waddress-space-strict.c:31:10: warning: cast removes address space of expression (<asn:1>)
+Waddress-space-strict.c:32:10: warning: cast removes address space of expression (<asn:1>)
 Waddress-space-strict.c:9:18: warning: non size-preserving integer to pointer cast
 Waddress-space-strict.c:10:25: warning: non size-preserving integer to pointer cast
 Waddress-space-strict.c:21:15: warning: non size-preserving pointer to integer cast