Re: Re: typeof and operands in named address spaces

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

[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: [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.