| Message ID | 20201117212829.99552-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | casts should drop qualifiers | expand |
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
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
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
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
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
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
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
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.
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 + */
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