| Message ID | 8d596a06-9f25-4d9f-8282-deb2d03a6b0a@moroto.mountain (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [v2] parse: handle __cleanup__ attribute | expand |
On Fri, Dec 08, 2023 at 12:49:34PM +0300, Dan Carpenter wrote: > The kernel has recently started using the __cleanup__ attribute. Save > a pointer to cleanup function. > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > v2: The first version of this patch had a bug handling a list of > declarations. I had to add a .cleanup = NULL at the start of > the loops iterations in declaration_list() and > external_declaration(). OK. See some notes here under, but first at all please forgive my very long delay. > diff --git a/parse.c b/parse.c > index 3d6fef7cb011..e5b5e6acc062 100644 > --- a/parse.c > +++ b/parse.c > @@ -537,6 +542,7 @@ static struct init_keyword { > /* Attributes */ > D("packed", &packed_op), > D("aligned", &aligned_op), > + D("__cleanup__", &cleanup_op), This should simply be D("cleanup" (to accept both the plain form and the __X__ form). > @@ -1964,6 +1984,7 @@ struct token *typename(struct token *token, struct symbol **p, int *forced) > token = declarator(token, &ctx); > apply_modifiers(token->pos, &ctx); > sym->ctype = ctx.ctype; > + sym->cleanup = ctx.cleanup; I don't think this should be needed because the cleanup attribute should be 'attached' to individual symbols, not their types (but I have no idea what GCC do). > @@ -2924,6 +2945,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis > > decl->ctype = ctx.ctype; > decl->ctype.modifiers |= mod; > + decl->cleanup = ctx.cleanup; Similarly, the attribute should only be applied to automatic variables, so this should not be needed/should be detected as an error. > diff --git a/symbol.h b/symbol.h > index 5270fcd73a10..88130c15d4bd 100644 > --- a/symbol.h > +++ b/symbol.h > @@ -204,6 +205,7 @@ struct symbol { > struct statement *inline_stmt; > struct symbol_list *inline_symbol_list; > struct expression *initializer; > + struct expression *cleanup; This annoys me a little bit as it adds one more member to struct symbol which is already the biggest memory user. But currently this is fine as other currently ignored attributes are also concerned. A solution is needed for all of them but there is no urgency. It would be nice to have some testcases though. Anyway, I can take this patch as-is modulo the few changes here above or you can send a v3 if you prefer. But it may take a few days before I'm able to push it to k.org. Best regards, -- Luc
On Mon, Dec 11, 2023 at 02:16:49PM +0100, Luc Van Oostenryck wrote: > On Fri, Dec 08, 2023 at 12:49:34PM +0300, Dan Carpenter wrote: > > The kernel has recently started using the __cleanup__ attribute. Save > > a pointer to cleanup function. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > v2: The first version of this patch had a bug handling a list of > > declarations. I had to add a .cleanup = NULL at the start of > > the loops iterations in declaration_list() and > > external_declaration(). > > OK. See some notes here under, but first at all please forgive my very long delay. > I would demand my money back, but I checked with my accountant and it turns out I'm not paying you anything. :P > > diff --git a/parse.c b/parse.c > > index 3d6fef7cb011..e5b5e6acc062 100644 > > --- a/parse.c > > +++ b/parse.c > > @@ -537,6 +542,7 @@ static struct init_keyword { > > /* Attributes */ > > D("packed", &packed_op), > > D("aligned", &aligned_op), > > + D("__cleanup__", &cleanup_op), > > This should simply be D("cleanup" (to accept both the plain form and the __X__ form). > > > @@ -1964,6 +1984,7 @@ struct token *typename(struct token *token, struct symbol **p, int *forced) > > token = declarator(token, &ctx); > > apply_modifiers(token->pos, &ctx); > > sym->ctype = ctx.ctype; > > + sym->cleanup = ctx.cleanup; > > I don't think this should be needed because the cleanup attribute should be > 'attached' to individual symbols, not their types (but I have no idea what GCC do). > > > @@ -2924,6 +2945,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis > > > > decl->ctype = ctx.ctype; > > decl->ctype.modifiers |= mod; > > + decl->cleanup = ctx.cleanup; > > Similarly, the attribute should only be applied to automatic variables, > so this should not be needed/should be detected as an error. > Yeah. There are a couple other "cleanup" lines later in the function that should be deleted as well, I see. Let me test this out resend in a few days. regards, dan carpenter
On Tue, Dec 12, 2023 at 12:39:40PM +0300, Dan Carpenter wrote: > > > @@ -2924,6 +2945,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis > > > > > > decl->ctype = ctx.ctype; > > > decl->ctype.modifiers |= mod; > > > + decl->cleanup = ctx.cleanup; > > > > Similarly, the attribute should only be applied to automatic variables, > > so this should not be needed/should be detected as an error. > > > > Yeah. There are a couple other "cleanup" lines later in the function > that should be deleted as well, I see. Hm... Something went wrong. When I remove this assignment then the cleanup function isn't saved here: void sched_exec(void) { struct task_struct *p = get_current(); struct migration_arg arg; int dest_cpu; for (class_raw_spinlock_irqsave_t scope __attribute__((__cleanup__(class_raw_spinlock_irqsave_destructor))) = class_raw_spinlock_irqsave_constructor(&p->pi_lock), *done = ((void *)0); class_raw_spinlock_irqsave_lock_ptr(&scope) && !done; done = (void *)1) { dest_cpu = p->sched_class->select_task_rq(p, task_cpu(p), 0x02); if (dest_cpu == debug_smp_processor_id()) return; if (__builtin_expect(!!(!cpu_active(dest_cpu)), 0)) return; arg = (struct migration_arg){ p, dest_cpu }; } stop_one_cpu(task_cpu(p), migration_cpu_stop, &arg); } regards, dan carpenter
On Wed, Dec 13, 2023 at 01:14:29PM +0300, Dan Carpenter wrote: > On Tue, Dec 12, 2023 at 12:39:40PM +0300, Dan Carpenter wrote: > > > > @@ -2924,6 +2945,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis > > > > > > > > decl->ctype = ctx.ctype; > > > > decl->ctype.modifiers |= mod; > > > > + decl->cleanup = ctx.cleanup; > > > > > > Similarly, the attribute should only be applied to automatic variables, > > > so this should not be needed/should be detected as an error. > > > > > > > Yeah. There are a couple other "cleanup" lines later in the function > > that should be deleted as well, I see. > > Hm... Something went wrong. When I remove this assignment then the > cleanup function isn't saved here: Mmmmhh yes, my bad. I thought that the parsing functions followed closely the names of the C grammar in the spec. They largely do but not in this case (they can't because some context is needed to distinguish between 'declaration' and 'function-definition'). Would the following patch be OK for you when applied on top of your v2? It contains: - the attribute can be removed from the list of ignored attributes - I prefer to add the "attribute_cleanup," on its own line - I added some checks and a few corresponding testcases - the s/D("__cleanup__"/D("cleanup"/ - and the removal of 'sym->cleanup = ctx.cleanup;' from typename() which I think is still unneeded. Best regards, -- Luc diff --git a/gcc-attr-list.h b/gcc-attr-list.h index c78001757229..928ea3889e01 100644 --- a/gcc-attr-list.h +++ b/gcc-attr-list.h @@ -24,7 +24,6 @@ GCC_ATTR(brk_interrupt) GCC_ATTR(callee_pop_aggregate_return) GCC_ATTR(cb) GCC_ATTR(cdecl) -GCC_ATTR(cleanup) GCC_ATTR(cmse_nonsecure_call) GCC_ATTR(cmse_nonsecure_entry) GCC_ATTR(cold) diff --git a/parse.c b/parse.c index e5b5e6acc062..f868bf63a0f5 100644 --- a/parse.c +++ b/parse.c @@ -79,11 +79,11 @@ typedef struct token *attr_t(struct token *, struct symbol *, struct decl_state *); static attr_t - attribute_packed, attribute_aligned, attribute_cleanup, - attribute_modifier, + attribute_packed, attribute_aligned, attribute_modifier, attribute_function, attribute_bitwise, attribute_address_space, attribute_context, + attribute_cleanup, attribute_designated_init, attribute_transparent_union, ignore_attribute, attribute_mode, attribute_force; @@ -542,7 +542,7 @@ static struct init_keyword { /* Attributes */ D("packed", &packed_op), D("aligned", &aligned_op), - D("__cleanup__", &cleanup_op), + D("cleanup", &cleanup_op), D("nocast", &attr_mod_op, .mods = MOD_NOCAST), D("noderef", &attr_mod_op, .mods = MOD_NODEREF), D("safe", &attr_mod_op, .mods = MOD_SAFE), @@ -1125,10 +1125,18 @@ static struct token *attribute_cleanup(struct token *token, struct symbol *attr, struct expression *expr = NULL; if (match_op(token, '(')) { - token = parens_expression(token, &expr, "in attribute"); + token = token->next; + if (match_op(token, ')')) + sparse_error(token->pos, "an argument is expected for attribute 'cleanup'"); + else if (token_type(token) != TOKEN_IDENT) + sparse_error(token->pos, "argument is not an identifier"); + token = primary_expression(token, &expr); if (expr && expr->type == EXPR_SYMBOL) ctx->cleanup = expr; + return expect(token, ')', "after attribute's argument'"); } + + sparse_error(token->pos, "an argument is expected for attribute 'cleanup'"); return token; } @@ -1984,7 +1992,6 @@ struct token *typename(struct token *token, struct symbol **p, int *forced) token = declarator(token, &ctx); apply_modifiers(token->pos, &ctx); sym->ctype = ctx.ctype; - sym->cleanup = ctx.cleanup; sym->endpos = token->pos; class = ctx.storage_class; if (forced) diff --git a/validation/parsing/attr-cleanup.c b/validation/parsing/attr-cleanup.c new file mode 100644 index 000000000000..ac64649c2ac1 --- /dev/null +++ b/validation/parsing/attr-cleanup.c @@ -0,0 +1,33 @@ +#define __cleanup(F) __attribute__((__cleanup__(F))) + +void fun(int *ptr); + +int test(int n); +int test(int n) +{ + int var __attribute__((cleanup(fun))) = 1; + int alt __cleanup(fun) = 2; + int mis __cleanup(0) = 3; + int non __attribute__((cleanup)); + int mis __attribute__((cleanup())); + int two __attribute__((cleanup(fun, fun))); + + for (int i __cleanup(fun) = 0; i < n; i++) + ; + + var = 5; + return 0; +} + +/* + * check-name: attr-cleanup + * check-command: sparse -Wunknown-attribute $file + * + * check-error-start +parsing/attr-cleanup.c:10:17: error: argument is not an identifier +parsing/attr-cleanup.c:11:39: error: an argument is expected for attribute 'cleanup' +parsing/attr-cleanup.c:12:40: error: an argument is expected for attribute 'cleanup' +parsing/attr-cleanup.c:13:43: error: Expected ) after attribute's argument' +parsing/attr-cleanup.c:13:43: error: got , + * check-error-end + */
On Thu, Dec 14, 2023 at 02:05:22PM +0100, Luc Van Oostenryck wrote: > On Wed, Dec 13, 2023 at 01:14:29PM +0300, Dan Carpenter wrote: > > On Tue, Dec 12, 2023 at 12:39:40PM +0300, Dan Carpenter wrote: > > > > > @@ -2924,6 +2945,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis > > > > > > > > > > decl->ctype = ctx.ctype; > > > > > decl->ctype.modifiers |= mod; > > > > > + decl->cleanup = ctx.cleanup; > > > > > > > > Similarly, the attribute should only be applied to automatic variables, > > > > so this should not be needed/should be detected as an error. > > > > > > > > > > Yeah. There are a couple other "cleanup" lines later in the function > > > that should be deleted as well, I see. > > > > Hm... Something went wrong. When I remove this assignment then the > > cleanup function isn't saved here: > > Mmmmhh yes, my bad. > I thought that the parsing functions followed closely the names of the > C grammar in the spec. They largely do but not in this case (they can't > because some context is needed to distinguish between 'declaration' and > 'function-definition'). > > Would the following patch be OK for you when applied on top of your v2? > It contains: > - the attribute can be removed from the list of ignored attributes > - I prefer to add the "attribute_cleanup," on its own line > - I added some checks and a few corresponding testcases > - the s/D("__cleanup__"/D("cleanup"/ > - and the removal of 'sym->cleanup = ctx.cleanup;' from typename() which > I think is still unneeded. Yep. Perfect. Thanks so much! regards, dan carpenter
On Thu, Dec 14, 2023 at 04:20:20PM +0300, Dan Carpenter wrote:
> Yep. Perfect. Thanks so much!
Pushed now.
-- Luc
Hi, On Mon, Dec 18, 2023 at 02:51:32PM +0100, Luc Van Oostenryck wrote: > On Thu, Dec 14, 2023 at 04:20:20PM +0300, Dan Carpenter wrote: > > Yep. Perfect. Thanks so much! > > Pushed now. Any chance someone is looking at making context tracking working for code annotated as __cleanup? We already have a bunch of code using constructs like: ... guard(spinlock_irqsave)(&gpio_lock); if (!test_bit(FLAG_REQUESTED, &desc->flags)) return NULL; ... which resuls in: $ make C=1 W=1 drivers/gpio/gpiolib.o CALL scripts/checksyscalls.sh DESCEND objtool INSTALL libsubcmd_headers CC drivers/gpio/gpiolib.o CHECK drivers/gpio/gpiolib.c drivers/gpio/gpiolib.c:2359:6: warning: context imbalance in 'gpiochip_dup_line_label' - different lock contexts for basic block and I expect we'll see more and more of this. Thanks.
On Wed, Jan 17, 2024 at 12:20:13PM -0800, Dmitry Torokhov wrote: > On Mon, Dec 18, 2023 at 02:51:32PM +0100, Luc Van Oostenryck wrote: > > On Thu, Dec 14, 2023 at 04:20:20PM +0300, Dan Carpenter wrote: > > > Yep. Perfect. Thanks so much! > > > > Pushed now. > > Any chance someone is looking at making context tracking working for > code annotated as __cleanup? We already have a bunch of code using > constructs like: > > ... > guard(spinlock_irqsave)(&gpio_lock); > > if (!test_bit(FLAG_REQUESTED, &desc->flags)) > return NULL; > ... > > which resuls in: > > $ make C=1 W=1 drivers/gpio/gpiolib.o > CALL scripts/checksyscalls.sh > DESCEND objtool > INSTALL libsubcmd_headers > CC drivers/gpio/gpiolib.o > CHECK drivers/gpio/gpiolib.c > drivers/gpio/gpiolib.c:2359:6: warning: context imbalance in 'gpiochip_dup_line_label' - different lock contexts for basic block > > and I expect we'll see more and more of this. +1 here. It's quite annoying for every Linux kernel developer in the world (which are at least 2k of active ones).
diff --git a/parse.c b/parse.c index 3d6fef7cb011..e5b5e6acc062 100644 --- a/parse.c +++ b/parse.c @@ -79,7 +79,8 @@ typedef struct token *attr_t(struct token *, struct symbol *, struct decl_state *); static attr_t - attribute_packed, attribute_aligned, attribute_modifier, + attribute_packed, attribute_aligned, attribute_cleanup, + attribute_modifier, attribute_function, attribute_bitwise, attribute_address_space, attribute_context, @@ -361,6 +362,10 @@ static struct symbol_op aligned_op = { .attribute = attribute_aligned, }; +static struct symbol_op cleanup_op = { + .attribute = attribute_cleanup, +}; + static struct symbol_op attr_mod_op = { .attribute = attribute_modifier, }; @@ -537,6 +542,7 @@ static struct init_keyword { /* Attributes */ D("packed", &packed_op), D("aligned", &aligned_op), + D("__cleanup__", &cleanup_op), D("nocast", &attr_mod_op, .mods = MOD_NOCAST), D("noderef", &attr_mod_op, .mods = MOD_NODEREF), D("safe", &attr_mod_op, .mods = MOD_SAFE), @@ -1114,6 +1120,18 @@ static struct token *attribute_aligned(struct token *token, struct symbol *attr, return token; } +static struct token *attribute_cleanup(struct token *token, struct symbol *attr, struct decl_state *ctx) +{ + struct expression *expr = NULL; + + if (match_op(token, '(')) { + token = parens_expression(token, &expr, "in attribute"); + if (expr && expr->type == EXPR_SYMBOL) + ctx->cleanup = expr; + } + return token; +} + static void apply_mod(struct position *pos, unsigned long *mods, unsigned long mod) { if (*mods & mod & ~MOD_DUP_OK) @@ -1899,6 +1917,7 @@ static struct token *declaration_list(struct token *token, struct symbol_list ** saved = ctx.ctype; for (;;) { struct symbol *decl = alloc_symbol(token->pos, SYM_NODE); + ctx.cleanup = NULL; ctx.ident = &decl->ident; token = declarator(token, &ctx); @@ -1910,6 +1929,7 @@ static struct token *declaration_list(struct token *token, struct symbol_list ** decl->ctype = ctx.ctype; decl->ctype.modifiers |= mod; + decl->cleanup = ctx.cleanup; decl->endpos = token->pos; add_symbol(list, decl); if (!match_op(token, ',')) @@ -1964,6 +1984,7 @@ struct token *typename(struct token *token, struct symbol **p, int *forced) token = declarator(token, &ctx); apply_modifiers(token->pos, &ctx); sym->ctype = ctx.ctype; + sym->cleanup = ctx.cleanup; sym->endpos = token->pos; class = ctx.storage_class; if (forced) @@ -2924,6 +2945,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis decl->ctype = ctx.ctype; decl->ctype.modifiers |= mod; + decl->cleanup = ctx.cleanup; decl->endpos = token->pos; /* Just a type declaration? */ @@ -3041,6 +3063,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis ident = NULL; decl = alloc_symbol(token->pos, SYM_NODE); ctx.ctype = saved; + ctx.cleanup = NULL; token = handle_attributes(token, &ctx); token = declarator(token, &ctx); token = handle_asm_name(token, &ctx); @@ -3048,6 +3071,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis apply_modifiers(token->pos, &ctx); decl->ctype = ctx.ctype; decl->ctype.modifiers |= mod; + decl->cleanup = ctx.cleanup; decl->endpos = token->pos; if (!ident) { sparse_error(token->pos, "expected identifier name in type definition"); diff --git a/symbol.h b/symbol.h index 5270fcd73a10..88130c15d4bd 100644 --- a/symbol.h +++ b/symbol.h @@ -107,6 +107,7 @@ struct decl_state { struct ctype ctype; struct ident **ident; struct symbol_op *mode; + struct expression *cleanup; unsigned long f_modifiers; // function attributes unsigned long storage_class; unsigned char prefer_abstract; @@ -204,6 +205,7 @@ struct symbol { struct statement *inline_stmt; struct symbol_list *inline_symbol_list; struct expression *initializer; + struct expression *cleanup; struct entrypoint *ep; struct symbol *definition; };
The kernel has recently started using the __cleanup__ attribute. Save a pointer to cleanup function. Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- v2: The first version of this patch had a bug handling a list of declarations. I had to add a .cleanup = NULL at the start of the loops iterations in declaration_list() and external_declaration(). parse.c | 26 +++++++++++++++++++++++++- symbol.h | 2 ++ 2 files changed, 27 insertions(+), 1 deletion(-)