The require_context attribute
PATCH 1/1 The require_context attribute
Hi,
To make sparse more usefull for me, I extended sparse with a require_context attribute. With that sparse can warn you if one accesses a variable outside of the corresponding locking context.
A small example to illustrate the idea:
static int to_protect __protected_by(the_lock);
static int helper(void) __must_hold(the_lock)
{
to_protect = 1;
}static int valid1(void)
{
lock(the_lock);
helper();
unlock(the_lock);
}static int invalid1(void)
{
helper();
}static int invalid2(void)
{
to_protect = 1;
}Sparse will warn you about the out of context access in invalid2().
I realize that automatic propagation of the lock context from caller to callee seems to be desirable. On the other hand as it is now the programmer is forced to annotate each function that requires a lock with the appropriate __must_hold() macro. -- Which is high value documentations, and helps reviewers to understand the code quicker.
So far the burden to maintain these __must_hold() annotations was feasible on the kernel code we develop and maintain here.
A complete example is included in the patch: validation/context_requirement.c
-Phil
commit 17978b26f051dcdbd4de498cd464a3acbc272c3c
Author: Philipp Reisner <[email protected]> Date: Mon Mar 31 13:21:02 2008 +0200
Implemented the require_context attribute
The require_context attribute allowes you to annotate variables, struct members or union members and functions with context requirements.
This patch does not change the single pass nature of sparse, i.e. sparse does not find out on its own that a function that accesses an annotated variable is only called with the appropriate lock held. One has to annotate the functions that get called with a locks held as well.
allocate.c +1 -0
@@ -114,6 +114,7 @@ void show_allocations(struct allocator_struct *x)ALLOCATOR (ident,"identifiers" );ALLOCATOR (token,"tokens" );ALLOCATOR (context,"contexts" ); +ALLOCATOR (context_requirement,"context requirements" );ALLOCATOR (symbol,"symbols" );ALLOCATOR (expression,"expressions" );ALLOCATOR (statement,"statements" );
allocate.h +1 -0
@@ -65,6 +65,7 @@ extern void show_allocations(struct allocator_struct *);DECLARE_ALLOCATOR (ident);DECLARE_ALLOCATOR (token);DECLARE_ALLOCATOR (context); +DECLARE_ALLOCATOR (context_requirement);DECLARE_ALLOCATOR (symbol);DECLARE_ALLOCATOR (expression);DECLARE_ALLOCATOR (statement);
linearize.c +55 -2
126 lines not on this page · raw message
linearize.h +6 -0
@@ -124,6 +124,11 @@ struct instruction {const char *string;struct asm_rules *asm_rules; }; +struct /* context_req */ { +int access_type; +struct context_requirement * ctx_req; + }; + }; };
@@ -212,6 +217,7 @@ enum opcode {/* Sparse tagging (line numbers, context, whatever) */ OP_CONTEXT , +OP_CONTEXT_REQ , OP_RANGE,/* Needed to translate SSA back to normal form */
parse.c +77 -0
119 lines not on this page · raw message
sparse.c +33 -11
59 lines not on this page · raw message
symbol.c +9 -0
@@ -57,6 +57,11 @@ struct context *alloc_context(void)return __alloc_context (0 ); } +struct context_requirement *alloc_context_requirement (void ) +{ +return __alloc_context_requirement (0 ); +} +struct symbol *alloc_symbol (struct position pos,int type) {struct symbol *sym =__alloc_symbol (0 );
@@ -201,6 +206,8 @@ static struct symbol *examine_base_type(struct symbol *sym) sym->ctype.modifiers |= base_type->ctype.modifiers &MOD_PTRINHERIT ;concat_ptr_list ((struct ptr_list *)base_type->ctype.contexts, (struct ptr_list **)&sym->ctype.contexts); +concat_ptr_list ((struct ptr_list *)base_type->ctype.ctx_reqs, + (struct ptr_list **)&sym->ctype.ctx_reqs);if (base_type->type ==SYM_NODE ) { base_type = base_type->ctype.base_type; sym->ctype.base_type = base_type;
@@ -257,6 +264,8 @@ void merge_type(struct symbol *sym, struct symbol *base_type) sym->ctype.modifiers |= (base_type->ctype.modifiers & ~MOD_STORAGE );concat_ptr_list ((struct ptr_list *)base_type->ctype.contexts, (struct ptr_list **)&sym->ctype.contexts); +concat_ptr_list ((struct ptr_list *)base_type->ctype.ctx_reqs, + (struct ptr_list **)&sym->ctype.ctx_reqs); sym->ctype.base_type = base_type->ctype.base_type;if (sym->ctype.base_type->type ==SYM_NODE )merge_type (sym, sym->ctype.base_type);
symbol.h +9 -0
@@ -74,14 +74,23 @@ struct context {unsigned int in, out; }; +struct context_requirement { +struct expression *context; +unsigned int min, max; +enum {READ ,WRITE ,RDWR ,CALL } access_type; +}; +extern struct context *alloc_context (void ); +extern struct context_requirement *alloc_context_requirement (void ); DECLARE_PTR_LIST(context_list, struct context); +DECLARE_PTR_LIST(ctx_req_list, struct context_requirement);struct ctype {unsigned long modifiers;unsigned long alignment;struct context_list *contexts; +struct ctx_req_list *ctx_reqs;unsigned int as;struct symbol *base_type; };
validation/context_requirement.c new file +185 -0
185 lines not on this page · raw message
Signature · 3 lines
: Dipl-Ing Philipp Reisner Tel +43-1-8178292-50 : : LINBIT Information Technologies GmbH Fax +43-1-8178292-82 : : Vivenotgasse 48, 1120 Vienna, Austria http://www.linbit.com :
G'day Philipp
Quoted 3 lines · Philipp Reisner
+ ctc =
&ctype->ctype;
+ if (ctc)
+ad->ctx_reqs=ctc->ctx_reqs;
is this right ? or am I just confused ?
Darren J.
Am Dienstag, 1. April 2008 12:07:17 schrieb Darren Jenkins:
Quoted 11 lines
G'day Philipp
On Mon, Mar 31, 2008 at 11:00 PM, Philipp Reisner
Quoted 3 lines · <[email protected]> wrote:
+ ctc =
&ctype->ctype;
+ if (ctc)
+ad->ctx_reqs=ctc->ctx_reqs;is this right ? or am I just confused ?
Good day Darren,
Could you elaborate in a bit more detail your concerns regarding these lines ?
-Phil
Signature · 3 lines
: Dipl-Ing Philipp Reisner Tel +43-1-8178292-50 : : LINBIT Information Technologies GmbH Fax +43-1-8178292-82 : : Vivenotgasse 48, 1120 Vienna, Austria http://www.linbit.com :
Quoted 10 lines · Philipp Reisner
Am Dienstag, 1. April 2008 12:07:17 schrieb Darren Jenkins:
Quoted 9 lines
G'day Philipp
On Mon, Mar 31, 2008 at 11:00 PM, Philipp Reisner
Quoted 3 lines · <[email protected]> wrote:
+ ctc =
&ctype->ctype;
+ if (ctc)
+ad->ctx_reqs=ctc->ctx_reqs;
Quoted 6 lines
Quoted 1 line
is this right ? or am I just confused ?
Good day Darren,
Could you elaborate in a bit more detail your concerns regarding these lines ?
You take an address, how can it be NULL?
Signature · 1 line
marko
Am Dienstag, 1. April 2008 16:10:27 schrieb Marko Kreen:
Quoted 11 lines · Philipp Reisner
Quoted 10 lines
Am Dienstag, 1. April 2008 12:07:17 schrieb Darren Jenkins:
Quoted 9 lines
G'day Philipp
On Mon, Mar 31, 2008 at 11:00 PM, Philipp Reisner
Quoted 3 lines · <[email protected]> wrote:
+ ctc =
&ctype->ctype;
+ if (ctc)
+ad->ctx_reqs=ctc->ctx_reqs;
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
is this right ? or am I just confused ?
Good day Darren,
Could you elaborate in a bit more detail your concerns regarding these lines ?
You take an address, how can it be NULL?
Oh, right, now I see it. Sorry. Of course that can not be NULL.
linearize.c +1 -4
@@ -888,14 +888,11 @@ static int linearize_address_gen(struct entrypoint *ep, struct access_data *ad) {struct symbol *ctype = expr->ctype; -struct ctype *ctc =NULL ;if (!ctype)return 0 ; - ctc = &ctype->ctype; -if (ctc) - ad->ctx_reqs = ctc->ctx_reqs; + ad->ctx_reqs = ctype->ctype.ctx_reqs; ad->pos = expr->pos; ad->result_type = ctype; ad->source_type =base_type (ctype);
-Phil
Signature · 3 lines
: Dipl-Ing Philipp Reisner Tel +43-1-8178292-50 : : LINBIT Information Technologies GmbH Fax +43-1-8178292-82 : : Vivenotgasse 48, 1120 Vienna, Austria http://www.linbit.com :
Quoted 3 lines · Marko Kreen
You take an address, how can it be NULL?
Yep, this could only happen if both the pointer was NULL and ctype was the first structure member of struct symbol.
However neither of these things are true here.