Re: Re: typeof and operands in named address spaces

Re: Re: typeof and operands in named address spaces

[Peter Zijlstra]

( restoring at least linux-toolchains@vger.kernel.org, since that seems
  to have gone missing )

On Mon, Nov 16, 2020 at 10:11:50AM +0100, Richard Biener wrote:
> On Sun, Nov 15, 2020 at 11:53 AM Uecker, Martin
> <Martin.Uecker@med.uni-goettingen.de> wrote:
> > > On Wed, Nov 04, 2020 at 07:31:42PM +0100, Uros Bizjak wrote:
> > > > Hello!
> > > >
> > > > I was looking at the recent linux patch series [1] where segment
> > > > qualifiers (named address spaces) were introduced to handle percpu
> > > > variables. In the patch [2], the author mentions that:
> > > >
> > > > --q--
> > > > Unfortunately, gcc does not provide a way to remove segment
> > > > qualifiers, which is needed to use typeof() to create local instances
> > > > of the per-cpu variable. For this reason, do not use the segment
> > > > qualifier for per-cpu variables, and do casting using the segment
> > > > qualifier instead.
> > > > --/q--
> > >
> > > C in general does not provide means to strip qualifiers. We recently had
> > > a _lot_ of 'fun' trying to strip volatile from a type, see here:
> > >
> > >   https://lore.kernel.org/lkml/875zimp0ay.fsf@mpe.ellerman.id.au
> > >
> > > which resulted in the current __unqual_scalar_typeof() hack.
> > >
> > > If we're going to do compiler extentions here, can we pretty please have
> > > a sane means of modifying qualifiers in general?
> >
> > Another way to drop qualifiers is using a cast. So you
> > can use typeof twice:
> >
> > typeof((typeof(_var))_var) tmp__;
> >
> > This also works for non-scalars but this is a GCC extension.
> >
> >
> > WG14 plans to standardize typeof. I would like to hear opinion
> > whether we should have typeof drop qualifiers or not.
> >
> > Currently, it does not do this on all compilers I tested
> > (except _Atomic on GCC) and there are also use cases for
> > keeping qualifiers. This is an argument for keeping qualifiers
> > should we standardize it, but then we need a way to drop
> > qualifiers.
> >
> >
> > lvalue conversion drops qualifers in C.  In GCC, this is not
> > implemented correctly as it is unobvervable in standard C
> > (but it using typeof).
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97702
> >
> > A have a working patch in preparation to change this. Then you
> > could use
> >
> > typeof( ((void)0, x) )

Neat, that actually already works with clang. And I suppose we can use
the above GCC extention until such time as that GCC is fixed.

See below..

> > to drop qualifiers. But this would then
> > also do array-to-pointer conversion. I am not sure
> > whether this is a problem.

I don't _think_ so, but..

> > Of course, we could also introduce a new feature for
> > dropping qualifiers. Thoughts?

> Just add a new qualifier that un-qualifies?
> 
> _Unqual volatile T x;
> 
> is T with volatile (evenually) removed.  Or just a way to drop
> all using _Unqual?
> 
> _Unqual T x;
> 
> removing all qualifiers from T.  Or add a special _Unqual_all
> to achieve that.  I think removing a specific qualification is
> useful.  Leaves cases like
> 
> _Unqual volatile volatile T x;
> 
> to be specified (that is ordering and cancellation of the
> unqual and qual variants of qualifiers).

I rather like this, however I think I'd prefer the syntax be something
like:

	_Unqual T x;

for removing all qualifiers, and:

	_Unqual(volatile) volatile T X;

for stripping specific qualifiers. The syntax as proposed above seems
very error prone to me.


---
Subject: compiler: Improve __unqual_typeof()

Improve our __unqual_scalar_typeof() implementation by relying on C
dropping qualifiers for lvalue convesions. There is one small catch in
that GCC is currently known broken in this respect, however it happens
to have a C language extention that achieves the very same, it drops
qualifiers on casts.

This gets rid of the _Generic() usage and should improve compile times
(less preprocessor output) as well as increases the capabilities of the
macros.

XXX: I've only verified the below actually compiles, I've not verified
     the generated code is actually 'correct'.

Suggested-by: "Uecker, Martin" <Martin.Uecker@med.uni-goettingen.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 74c6c0486eed..3c5cb52c12f9 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -156,3 +156,11 @@
 #else
 #define __diag_GCC_8(s)
 #endif
+
+/*
+ * GCC has a bug where lvalue conversion doesn't drop qualifiers, use a GCC
+ * extention instead. GCC drops qualifiers on a cast, so use a double typeof().
+ *
+ *  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97702
+ */
+#define __unqual_typeof(type)	typeof( (typeof(type))type )
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index ac3fa37a84f9..4a6e2caab17b 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -250,27 +250,14 @@ struct ftrace_likely_data {
 /* Are two types/vars the same type (ignoring qualifiers)? */
 #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
 
+#ifndef __unqual_typeof
 /*
- * __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
- *			       non-scalar types unchanged.
+ * lvalue conversion drops qualifiers
  */
-/*
- * Prefer C11 _Generic for better compile-times and simpler code. Note: 'char'
- * is not type-compatible with 'signed char', and we define a separate case.
- */
-#define __scalar_type_to_expr_cases(type)				\
-		unsigned type:	(unsigned type)0,			\
-		signed type:	(signed type)0
-
-#define __unqual_scalar_typeof(x) typeof(				\
-		_Generic((x),						\
-			 char:	(char)0,				\
-			 __scalar_type_to_expr_cases(char),		\
-			 __scalar_type_to_expr_cases(short),		\
-			 __scalar_type_to_expr_cases(int),		\
-			 __scalar_type_to_expr_cases(long),		\
-			 __scalar_type_to_expr_cases(long long),	\
-			 default: (x)))
+#define __unqual_typeof(type)	typeof( ((void)0, type) )
+#endif
+
+#define __unqual_scalar_typeof(type) __unqual_typeof(type)
 
 /* Is this type a native word size -- useful for atomic operations */
 #define __native_word(t) \

Re: Re: typeof and operands in named address spaces

[Peter Zijlstra]

On Mon, Nov 16, 2020 at 12:10:56PM +0100, Peter Zijlstra wrote:

> > > Another way to drop qualifiers is using a cast. So you
> > > can use typeof twice:
> > >
> > > typeof((typeof(_var))_var) tmp__;
> > >
> > > This also works for non-scalars but this is a GCC extension.

FWIW, clang seems to support this extension as well..

Re: Re: typeof and operands in named address spaces

[Uecker, Martin]

Am Montag, den 16.11.2020, 12:10 +0100 schrieb Peter Zijlstra:
> ( restoring at least linux-toolchains@vger.kernel.org, since that
> seems
>   to have gone missing )
> 
> On Mon, Nov 16, 2020 at 10:11:50AM +0100, Richard Biener wrote:
> > On Sun, Nov 15, 2020 at 11:53 AM Uecker, Martin
> > <Martin.Uecker@med.uni-goettingen.de> wrote:
> > > > On Wed, Nov 04, 2020 at 07:31:42PM +0100, Uros Bizjak wrote:
> > > > > Hello!
> > > > > 
> > > > > I was looking at the recent linux patch series [1] where
> > > > > segment
> > > > > qualifiers (named address spaces) were introduced to handle
> > > > > percpu
> > > > > variables. In the patch [2], the author mentions that:
> > > > > 
> > > > > --q--
> > > > > Unfortunately, gcc does not provide a way to remove segment
> > > > > qualifiers, which is needed to use typeof() to create local
> > > > > instances
> > > > > of the per-cpu variable. For this reason, do not use the
> > > > > segment
> > > > > qualifier for per-cpu variables, and do casting using the
> > > > > segment
> > > > > qualifier instead.
> > > > > --/q--
> > > > 
> > > > C in general does not provide means to strip qualifiers. We
> > > > recently had
> > > > a _lot_ of 'fun' trying to strip volatile from a type, see
> > > > here:
> > > > 
> > > >   
> > > > https://lore.kernel.org/lkml/875zimp0ay.fsf@mpe.ellerman.id.au
> > > > 
> > > > which resulted in the current __unqual_scalar_typeof() hack.
> > > > 
> > > > If we're going to do compiler extentions here, can we pretty
> > > > please have
> > > > a sane means of modifying qualifiers in general?
> > > 
> > > Another way to drop qualifiers is using a cast. So you
> > > can use typeof twice:
> > > 
> > > typeof((typeof(_var))_var) tmp__;
> > > 
> > > This also works for non-scalars but this is a GCC extension.

(That casts drop qualifiers is standard C. The extensions
are 'typeof' and that casts can be used for non-scalar types.)

> > > 
> > > WG14 plans to standardize typeof. I would like to hear opinion
> > > whether we should have typeof drop qualifiers or not.
> > > 
> > > Currently, it does not do this on all compilers I tested
> > > (except _Atomic on GCC) and there are also use cases for
> > > keeping qualifiers. This is an argument for keeping qualifiers
> > > should we standardize it, but then we need a way to drop
> > > qualifiers.
> > > 
> > > 
> > > lvalue conversion drops qualifers in C.  In GCC, this is not
> > > implemented correctly as it is unobvervable in standard C
> > > (but it using typeof).
> > > 
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97702
> > > 
> > > A have a working patch in preparation to change this. Then you
> > > could use
> > > 
> > > typeof( ((void)0, x) )
> 
> Neat, that actually already works with clang. And I suppose we can
> use
> the above GCC extention until such time as that GCC is fixed.
> 
> See below..
> 
> > > to drop qualifiers. But this would then
> > > also do array-to-pointer conversion. I am not sure
> > > whether this is a problem.
> 
> I don't _think_ so, but..
> 
> > > Of course, we could also introduce a new feature for
> > > dropping qualifiers. Thoughts?
> > Just add a new qualifier that un-qualifies?
> > 
> > _Unqual volatile T x;
> > 
> > is T with volatile (evenually) removed.  Or just a way to drop
> > all using _Unqual?
> > 
> > _Unqual T x;
> > 
> > removing all qualifiers from T.  Or add a special _Unqual_all
> > to achieve that.  I think removing a specific qualification is
> > useful.  Leaves cases like
> > 
> > _Unqual volatile volatile T x;
> > 
> > to be specified (that is ordering and cancellation of the
> > unqual and qual variants of qualifiers).
> 
> I rather like this, however I think I'd prefer the syntax be
> something
> like:
> 
> 	_Unqual T x;
> 
> for removing all qualifiers, and:
> 
> 	_Unqual(volatile) volatile T X;
> 
> for stripping specific qualifiers. The syntax as proposed above seems
> very error prone to me.


There is also the idea of adding 'auto' to C which
would drop
qualifiers. (__auto_type does not on GCC
but on some other compilers)

Best,
Martin



> 
> 
> ---
> Subject: compiler: Improve __unqual_typeof()
> 
> Improve our __unqual_scalar_typeof() implementation by relying on C
> dropping qualifiers for lvalue convesions. There is one small catch
> in
> that GCC is currently known broken in this respect, however it
> happens
> to have a C language extention that achieves the very same, it drops
> qualifiers on casts.
> 
> This gets rid of the _Generic() usage and should improve compile
> times
> (less preprocessor output) as well as increases the capabilities of
> the
> macros.
> 
> XXX: I've only verified the below actually compiles, I've not
> verified
>      the generated code is actually 'correct'.
> 
> Suggested-by: "Uecker, Martin" <Martin.Uecker@med.uni-goettingen.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-
> gcc.h
> index 74c6c0486eed..3c5cb52c12f9 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -156,3 +156,11 @@
>  #else
>  #define __diag_GCC_8(s)
>  #endif
> +
> +/*
> + * GCC has a bug where lvalue conversion doesn't drop qualifiers,
> use a GCC
> + * extention instead. GCC drops qualifiers on a cast, so use a
> double typeof().
> + *
> + *  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97702
> + */
> +#define __unqual_typeof(type)	typeof( (typeof(type))type )
> diff --git a/include/linux/compiler_types.h
> b/include/linux/compiler_types.h
> index ac3fa37a84f9..4a6e2caab17b 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -250,27 +250,14 @@ struct ftrace_likely_data {
>  /* Are two types/vars the same type (ignoring qualifiers)? */
>  #define __same_type(a, b) __builtin_types_compatible_p(typeof(a),
> typeof(b))
>  
> +#ifndef __unqual_typeof
>  /*
> - * __unqual_scalar_typeof(x) - Declare an unqualified scalar type,
> leaving
> - *			       non-scalar types unchanged.
> + * lvalue conversion drops qualifiers
>   */
> -/*
> - * Prefer C11 _Generic for better compile-times and simpler code.
> Note: 'char'
> - * is not type-compatible with 'signed char', and we define a
> separate case.
> - */
> -#define __scalar_type_to_expr_cases(type)				
> \
> -		unsigned type:	(unsigned type)0,			
> \
> -		signed type:	(signed type)0
> -
> -#define __unqual_scalar_typeof(x) typeof(				
> \
> -		_Generic((x),						
> \
> -			 char:	(char)0,				
> \
> -			 __scalar_type_to_expr_cases(char),		
> \
> -			 __scalar_type_to_expr_cases(short),		
> \
> -			 __scalar_type_to_expr_cases(int),		
> \
> -			 __scalar_type_to_expr_cases(long),		
> \
> -			 __scalar_type_to_expr_cases(long long),	\
> -			 default: (x)))
> +#define __unqual_typeof(type)	typeof( ((void)0, type) )
> +#endif
> +
> +#define __unqual_scalar_typeof(type) __unqual_typeof(type)
>  
>  /* Is this type a native word size -- useful for atomic operations
> */
>  #define __native_word(t) \

Re: Re: typeof and operands in named address spaces

[Peter Zijlstra]

On Mon, Nov 16, 2020 at 12:23:17PM +0000, Uecker, Martin wrote:

> > > > Another way to drop qualifiers is using a cast. So you
> > > > can use typeof twice:
> > > > 
> > > > typeof((typeof(_var))_var) tmp__;
> > > > 
> > > > This also works for non-scalars but this is a GCC extension.
> 
> (That casts drop qualifiers is standard C. The extensions
> are 'typeof' and that casts can be used for non-scalar types.)

Ah, I'll clarify. Thanks!

Re: Re: typeof and operands in named address spaces

[Linus Torvalds]

On Mon, Nov 16, 2020 at 3:11 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> XXX: I've only verified the below actually compiles, I've not verified
>      the generated code is actually 'correct'.

Well, it was mainly the arm64 code generation for load-acquire and
store-release that wanted this - so it's really the generated code
that matters.

Will, can you check?

Because:

> +#define __unqual_typeof(type)  typeof( (typeof(type))type )

that's certainly a much nicer version than the existing pre-processor
expansion from hell.

             Linus

Re: Re: typeof and operands in named address spaces

[Jakub Jelinek]

On Tue, Nov 17, 2020 at 11:13:52AM -0800, Linus Torvalds wrote:
> > +#define __unqual_typeof(type)  typeof( (typeof(type))type )

> that's certainly a much nicer version than the existing pre-processor
> expansion from hell.

It would need to be typeof( (typeof(type)) (type) ) to not be that
constrained on what kind of expressions it accepts as arguments.
Anyway, it won't work with array types at least,
  int a[10];
  typeof ((typeof (a)) (a)) b;
is an error (in both gcc and clang), while typeof (a) b; will work
(but not drop the qualifiers).  Don't know if the kernel cares or not.

	Jakub

Re: Re: typeof and operands in named address spaces

[Linus Torvalds]

On Tue, Nov 17, 2020 at 11:25 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> It would need to be typeof( (typeof(type)) (type) ) to not be that
> constrained on what kind of expressions it accepts as arguments.

Yup.

> Anyway, it won't work with array types at least,
>   int a[10];
>   typeof ((typeof (a)) (a)) b;
> is an error (in both gcc and clang), while typeof (a) b; will work
> (but not drop the qualifiers).  Don't know if the kernel cares or not.

Well, the kernel already doesn't allow that, because our existing
horror only handles simple integer scalar types.

So that macro is a clear improvement - if it actually works (local
testing says it does, but who knows about random compiler versions
etc)

          Linus

Re: Re: typeof and operands in named address spaces

[Linus Torvalds]

On Tue, Nov 17, 2020 at 11:13 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> > +#define __unqual_typeof(type)  typeof( (typeof(type))type )
>
> that's certainly a much nicer version than the existing pre-processor
> expansion from hell.

Oh, and sparse doesn't handle this, and doesn't remove any qualifiers
for the above. And so this horror-test-case fails:

    #define __unqual_typeof(type)  typeof( (typeof(type))(type) )

    int *function(volatile int x)
    {
        extern __unqual_typeof(x) y;
        return &y;
    }

with

  t.c:6:17: warning: incorrect type in return expression (different modifiers)
  t.c:6:17:    expected int *
  t.c:6:17:    got int volatile *
  t.c:3:5: warning: symbol 'function' was not declared. Should it be static?

adding Luc and the sparse mailing list to the participants list.

But it does work with both gcc and clang for me.

For Luc, quoting some earlier emails:

> > lvalue conversion drops qualifers in C.  In GCC, this is not
> > implemented correctly as it is unobservable in standard C
> > (but it using typeof).
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97702
> >
> > A have a working patch in preparation to change this. Then you
> > could use
> >
> > typeof( ((void)0, x) )

on another thing that fails with sparse. But since gcc gets that wrong
too and it only works with clang, that's not so relevant for the
kernel.

Luc - same test-case as above, just

    #define __unqual_typeof(x) typeof( ((void)0, (x)) )

instead.

           Linus

Re: Re: typeof and operands in named address spaces

[Will Deacon]

On Tue, Nov 17, 2020 at 11:31:57AM -0800, Linus Torvalds wrote:
> On Tue, Nov 17, 2020 at 11:25 AM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > It would need to be typeof( (typeof(type)) (type) ) to not be that
> > constrained on what kind of expressions it accepts as arguments.
> 
> Yup.
> 
> > Anyway, it won't work with array types at least,
> >   int a[10];
> >   typeof ((typeof (a)) (a)) b;
> > is an error (in both gcc and clang), while typeof (a) b; will work
> > (but not drop the qualifiers).  Don't know if the kernel cares or not.
> 
> Well, the kernel already doesn't allow that, because our existing
> horror only handles simple integer scalar types.
> 
> So that macro is a clear improvement - if it actually works (local
> testing says it does, but who knows about random compiler versions
> etc)

I'll give it a go now, although if it works I honestly won't know whether
to laugh or cry.

Will

Re: Re: typeof and operands in named address spaces

[Uecker, Martin]

Am Dienstag, den 17.11.2020, 11:31 -0800 schrieb Linus Torvalds:
> On Tue, Nov 17, 2020 at 11:25 AM Jakub Jelinek <jakub@redhat.com> wrote:
> > 
> > It would need to be typeof( (typeof(type)) (type) ) to not be that
> > constrained on what kind of expressions it accepts as arguments.
> 
> Yup.
> 
> > Anyway, it won't work with array types at least,
> >   int a[10];
> >   typeof ((typeof (a)) (a)) b;
> > is an error (in both gcc and clang), while typeof (a) b; will work
> > (but not drop the qualifiers).  Don't know if the kernel cares or not.
> 
> Well, the kernel already doesn't allow that, because our existing
> horror only handles simple integer scalar types.
> 
> So that macro is a clear improvement - if it actually works (local
> testing says it does, but who knows about random compiler versions
> etc)

Well, the version that also works for other types (except
multi-dim arrays) is slightly more complicated ;-)

https://godbolt.org/z/KTKqon

Best,
Martin

[PATCH] casts should drop qualifiers

[Luc Van Oostenryck]

Casts should drop qualifiers but Sparse doesn't do this yet.

The fix seems pretty simple: after having evaluated the type of
the cast, if this type is a SYM_NODE and contains qualifiers,
make a copy of the type with the qualifiers removed and use
this copy as the type.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---

This seems a bit too simple to be true but it seems correct and
it passes the testcase here under and a related testcase from GCC.

 evaluate.c                    | 13 +++++++++++++
 validation/eval/cast-unqual.c | 14 ++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 validation/eval/cast-unqual.c

diff --git a/evaluate.c b/evaluate.c
index 43a611696787..004cd2f9b339 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2998,6 +2998,18 @@ static struct symbol *evaluate_compound_literal(struct expression *expr, struct
 	return sym;
 }
 
+static struct symbol *unqualify_type(struct symbol *ctype)
+{
+	if (ctype->type == SYM_NODE && (ctype->ctype.modifiers & MOD_QUALIFIER)) {
+		struct symbol *unqual = alloc_symbol(ctype->pos, 0);
+
+		*unqual = *ctype;
+		unqual->ctype.modifiers &= ~MOD_QUALIFIER;
+		return unqual;
+	}
+	return ctype;
+}
+
 static struct symbol *evaluate_cast(struct expression *expr)
 {
 	struct expression *source = expr->cast_expression;
@@ -3025,6 +3037,7 @@ static struct symbol *evaluate_cast(struct expression *expr)
 		return evaluate_compound_literal(expr, source);
 
 	ctype = examine_symbol_type(expr->cast_type);
+	ctype = unqualify_type(ctype);
 	expr->ctype = ctype;
 	expr->cast_type = ctype;
 
diff --git a/validation/eval/cast-unqual.c b/validation/eval/cast-unqual.c
new file mode 100644
index 000000000000..0ea318875c96
--- /dev/null
+++ b/validation/eval/cast-unqual.c
@@ -0,0 +1,14 @@
+#define cvr const volatile restrict
+
+_Static_assert([typeof((cvr int) 0)] == [int]);
+_Static_assert([typeof((cvr int *) 0)] == [cvr int *]);
+
+static int *function(volatile int x)
+{
+	extern typeof((typeof(x)) (x)) y;
+	return &y;
+}
+
+/*
+ * check-name: cast-unqual
+ */
-- 
2.29.2

Re: Re: typeof and operands in named address spaces

[Will Deacon]

On Tue, Nov 17, 2020 at 09:10:53PM +0000, Will Deacon wrote:
> On Tue, Nov 17, 2020 at 11:31:57AM -0800, Linus Torvalds wrote:
> > On Tue, Nov 17, 2020 at 11:25 AM Jakub Jelinek <jakub@redhat.com> wrote:
> > >
> > > It would need to be typeof( (typeof(type)) (type) ) to not be that
> > > constrained on what kind of expressions it accepts as arguments.
> > 
> > Yup.
> > 
> > > Anyway, it won't work with array types at least,
> > >   int a[10];
> > >   typeof ((typeof (a)) (a)) b;
> > > is an error (in both gcc and clang), while typeof (a) b; will work
> > > (but not drop the qualifiers).  Don't know if the kernel cares or not.
> > 
> > Well, the kernel already doesn't allow that, because our existing
> > horror only handles simple integer scalar types.
> > 
> > So that macro is a clear improvement - if it actually works (local
> > testing says it does, but who knows about random compiler versions
> > etc)
> 
> I'll give it a go now, although if it works I honestly won't know whether
> to laugh or cry.

GCC 9 and Clang 11 both seem to generate decent code for aarch64 defconfig
with:

#define __unqual_scalar_typeof(x)      typeof( (typeof(x)) (x))

replacing the current monstrosity. allnoconfig and allmodconfig build fine
too.

However, GCC 4.9.0 goes mad and starts spilling to the stack when dealing
with a pointer to volatile, as though we were just using typeof(). I tried
GCC 5.4.0 and that looks ok, so I think if anybody cares about the potential
performance regression with 4.9 then perhaps they should consider upgrading
their toolchain.

In other words, let's do it.

Will

Re: [PATCH] casts should drop qualifiers

[Linus Torvalds]

On Tue, Nov 17, 2020 at 1:29 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> Casts should drop qualifiers but Sparse doesn't do this yet.
>
> The fix seems pretty simple: after having evaluated the type of
> the cast, if this type is a SYM_NODE and contains qualifiers,
> make a copy of the type with the qualifiers removed and use
> this copy as the type.

Did you look at the lvalue conversion issue too?

IOW, ((void)0,(x)) should end up also with qualifiers dropped on the
end result, because the comma expression will have turned x from an
lvalue to an rvalue.

Would doing the same unqualify_type() in degenerate() be sufficient?

No, the kernel doesn't care, even with that suggested patch, so maybe
that all doesn't matter.

           Linus

Re: [PATCH] casts should drop qualifiers

[Luc Van Oostenryck]

On Tue, Nov 17, 2020 at 03:22:21PM -0800, Linus Torvalds wrote:
> On Tue, Nov 17, 2020 at 1:29 PM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > Casts should drop qualifiers but Sparse doesn't do this yet.
> >
> > The fix seems pretty simple: after having evaluated the type of
> > the cast, if this type is a SYM_NODE and contains qualifiers,
> > make a copy of the type with the qualifiers removed and use
> > this copy as the type.
> 
> Did you look at the lvalue conversion issue too?

I'm looking at it. 

> IOW, ((void)0,(x)) should end up also with qualifiers dropped on the
> end result, because the comma expression will have turned x from an
> lvalue to an rvalue.
> 
> Would doing the same unqualify_type() in degenerate() be sufficient?

For the comma, yes, I think it should be sufficient.
I'm checking a few things around but I'll finish this tomorrow.

-- Luc

Re: [PATCH] casts should drop qualifiers

[Linus Torvalds]

On Tue, Nov 17, 2020 at 3:22 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Would doing the same unqualify_type() in degenerate() be sufficient?

Actually, that's a stupid suggestion. Forget I ever mentioned it.

I should have reacted to Martin Ucker pointing out

> > lvalue conversion drops qualifers in C.  In GCC, this is not
> > implemented correctly as it is unobvervable in standard C
> > (but it using typeof).

with the notable point that it is unobservable outside of "typeof".

I'm not actually entirely sure that is true: if you don't drop
qualifiers, it's potentially observable in code generation, in that a
"volatile" that didn't get dropped might perhaps cause unnecessary
memory ops. But from a kernel variable type standpoint where we want
to just drop qualifiers on variables using "typeof()", maybe the
simplest solution would be just special-casing typeof itself, using
something (entirely untested and probably complete garbage) like this:

  --- a/symbol.c
  +++ b/symbol.c
  @@ -509,6 +509,7 @@ static struct symbol
*examine_pointer_type(struct symbol *sym)

   static struct symbol *examine_typeof(struct symbol *sym)
   {
  +     int lvalue = lvalue_expression(sym->initializer);
        struct symbol *base = evaluate_expression(sym->initializer);
        unsigned long mod = 0;

  @@ -520,6 +521,8 @@ static struct symbol *examine_typeof(struct symbol *sym)
        }
        if (base->type == SYM_BITFIELD)
                warning(base->pos, "typeof applied to bitfield type");
  +     if (!lvalue)
  +             mod &= MOD_QUALIFIER;
        sym->type = SYM_NODE;
        sym->ctype.modifiers = mod;
        sym->ctype.base_type = base;

Hmm?

           Linus

Re: [PATCH] casts should drop qualifiers

[Luc Van Oostenryck]

On Wed, Nov 18, 2020 at 10:31:43AM -0800, Linus Torvalds wrote:
> On Tue, Nov 17, 2020 at 3:22 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Would doing the same unqualify_type() in degenerate() be sufficient?
> 
> Actually, that's a stupid suggestion. Forget I ever mentioned it.
> 
> I should have reacted to Martin Ucker pointing out
> 
> > > lvalue conversion drops qualifers in C.  In GCC, this is not
> > > implemented correctly as it is unobvervable in standard C
> > > (but it using typeof).
> 
> with the notable point that it is unobservable outside of "typeof".
> 
> I'm not actually entirely sure that is true: if you don't drop
> qualifiers, it's potentially observable in code generation, in that a
> "volatile" that didn't get dropped might perhaps cause unnecessary

Yes, I had already added some testcases with volatile because the
the rules for const & volatile are different.

> memory ops. But from a kernel variable type standpoint where we want
> to just drop qualifiers on variables using "typeof()", maybe the
> simplest solution would be just special-casing typeof itself, using
> something (entirely untested and probably complete garbage) like this:

I don't think it's a good idea. The focus now is all about dropping
the qualifiers but in code like:
	const int x;
	typeof(c) y;
don't we want 'y' to also have the type 'const int'?

For the moment I'm testing the patch here under. It fixes the
qualifier dropping for comma expressions, and same for statement
expressions. It also, I think, fixes evaluate_postop() which
has the inverse error of dropping qualifiers but shouldn't.

I think that all the other cases are covered (but the code is fragile
because most qualifier dropping are done implicitly via classify_type()
which strip everything).


diff --git a/evaluate.c b/evaluate.c
index fd84205c7f2c..8599fcee6875 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1028,7 +1028,7 @@ static struct symbol *evaluate_binop(struct expression *expr)
 
 static struct symbol *evaluate_comma(struct expression *expr)
 {
-	expr->ctype = degenerate(expr->right);
+	expr->ctype = unqualify_type(degenerate(expr->right));
 	if (expr->ctype == &null_ctype)
 		expr->ctype = &ptr_ctype;
 	expr->flags &= expr->left->flags & expr->right->flags;
@@ -1935,8 +1935,7 @@ static struct symbol *evaluate_postop(struct expression *expr)
 	if (multiply) {
 		evaluate_assign_to(op, op->ctype);
 		expr->op_value = multiply;
-		expr->ctype = ctype;
-		return ctype;
+		return expr->ctype = op->ctype;
 	}
 
 	expression_error(expr, "bad argument type for ++/--");
@@ -3950,7 +3949,7 @@ struct symbol *evaluate_statement(struct statement *stmt)
 			return NULL;
 		if (stmt->expression->ctype == &null_ctype)
 			stmt->expression = cast_to(stmt->expression, &ptr_ctype);
-		return degenerate(stmt->expression);
+		return unqualify_type(degenerate(stmt->expression));
 
 	case STMT_COMPOUND: {
 		struct statement *s;


-- Luc

Re: [PATCH] casts should drop qualifiers

[Linus Torvalds]

On Wed, Nov 18, 2020 at 11:17 AM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> I don't think it's a good idea. The focus now is all about dropping
> the qualifiers but in code like:
>         const int x;
>         typeof(c) y;
> don't we want 'y' to also have the type 'const int'?

I assume you meant "typeof(x)". But yes, absolutely.

Which is why my suggested example patch had that explicit test for
"is_lvalue()".  So only for non-lvalues would it strip the qualifiers.

So "typeof(((void)0,x)) y;" would be "int", because that expression
inside the typeof isn't an lvalue.

But if you have something that is already doing the generic case, then
that's obviously better. My suggestion was more of a "we can zero in
on just that typeof case" thing.

            Linus

Re: [PATCH] casts should drop qualifiers

[Luc Van Oostenryck]

On Wed, Nov 18, 2020 at 11:51:00AM -0800, Linus Torvalds wrote:
> On Wed, Nov 18, 2020 at 11:17 AM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > I don't think it's a good idea. The focus now is all about dropping
> > the qualifiers but in code like:
> >         const int x;
> >         typeof(c) y;
> > don't we want 'y' to also have the type 'const int'?
> 
> I assume you meant "typeof(x)". But yes, absolutely.

Yes, sure.
 
> Which is why my suggested example patch had that explicit test for
> "is_lvalue()".  So only for non-lvalues would it strip the qualifiers.
> 
> So "typeof(((void)0,x)) y;" would be "int", because that expression
> inside the typeof isn't an lvalue.

Oh yes, sorry. For some reasons I had things upside down.
 
> But if you have something that is already doing the generic case, then
> that's obviously better. My suggestion was more of a "we can zero in
> on just that typeof case" thing.

I just sent the series but it's not generic.

If I read the standard correctly (big 'if'), in:
	volatile int x;
	typeof(++x) y;
'y' should have the type 'volatile int' and GCC interpret it so.

-- Luc

Re: [PATCH] casts should drop qualifiers

[Linus Torvalds]

On Wed, Nov 18, 2020 at 1:30 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> If I read the standard correctly (big 'if'), in:
>         volatile int x;
>         typeof(++x) y;
> 'y' should have the type 'volatile int' and GCC interpret it so.

That sounds extremely odd to me. I think it should have the same type
as "x += 1" or "x = x+1",  no?

And what gcc does is clearly not indicative of anything, since gcc
gets the comma expression wrong, so..

clang seems to have a better track record, and clang drops qualifiers
on "typeof(++x)". Stupid test-case:

    int *fn(volatile int p)
    {
        extern typeof(++p) x;
        return &x;
    }

results in no warnings with clang (but warns about dropped volatile with gcc).

           Linus

Re: [PATCH] casts should drop qualifiers

[Luc Van Oostenryck]

On Wed, Nov 18, 2020 at 04:58:26PM -0800, Linus Torvalds wrote:
> On Wed, Nov 18, 2020 at 1:30 PM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > If I read the standard correctly (big 'if'), in:
> >         volatile int x;
> >         typeof(++x) y;
> > 'y' should have the type 'volatile int' and GCC interpret it so.
> 
> That sounds extremely odd to me. I think it should have the same type
> as "x += 1" or "x = x+1",  no?

Yes, but both cases are explicitly excluded from C's 6.3.2.1 where
lvalue-conversion is defined. This whole section was very confusing
to me but the note 112) in n1570's 6.5.16.1 is somehow clearer.

So yes, I'll drop this patch (I should have tagged it as RFC anyway).
Thanks for the feedback.

-- Luc.