* [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 13:25 [PATCH 0/3] improve context handling Johannes Berg
@ 2008-04-10 13:25 ` Johannes Berg
2008-04-10 15:24 ` Philipp Reisner
2008-04-21 18:04 ` Josh Triplett
2008-04-10 13:25 ` [PATCH 2/3] sparse test suite: add test mixing __context__ and __attribute__((context(...))) Johannes Berg
` (4 subsequent siblings)
5 siblings, 2 replies; 26+ messages in thread
From: Johannes Berg @ 2008-04-10 13:25 UTC (permalink / raw)
To: Josh Triplett; +Cc: linux-sparse
[-- Attachment #1: contexts.patch --]
[-- Type: text/plain, Size: 33247 bytes --]
The sparse man page promises that it will check this:
Functions with the extended attribute
__attribute__((context(expression,in_context,out_context))
require the context expression (for instance, a lock) to have the
value in_context (a constant nonnegative integer) when called,
and return with the value out_context (a constant nonnegative
integer).
It doesn't keep that promise though, nor can it, especially with
contexts that can be acquired recursively (like RCU in the kernel.)
This patch makes sparse track different contexts, and also follows
up on that promise, but with slightly different semantics:
* the "require the context to have the value" is changed to require
it to have /at least/ the value if 'in_context',
* an exact_context(...) attribute is introduced with the previously
described semantics (to be used for non-recursive contexts),
* the __context__ statement is extended to also include a required
context argument (same at least semantics),
Unfortunately, I wasn't able to keep the same output, so now you'll
see different messages from sparse, especially when trying to unlock
a lock that isn't locked you'll see a message pointing to the unlock
function rather than complaining about the basic block, you can see
that in the test suite changes.
This patch also contains test updates and a lot of new tests for the
new functionality. Except for the changed messages, old functionality
should not be affected.
However, the kernel use of __attribute__((context(...)) is actually
wrong, the kernel often does things like:
static void *dev_mc_seq_start(struct seq_file *seq, loff_t * pos)
__acquires(dev_base_lock)
{
[...]
read_lock(&dev_base_lock);
[...]
}
rather than
static void *dev_mc_seq_start(struct seq_file *seq, loff_t * pos)
__acquires(dev_base_lock)
{
[...]
__acquire__(dev_base_lock);
read_lock(&dev_base_lock);
[...]
}
(and possibly more when read_lock() is annotated appropriately, such
as dropping whatever context read_lock() returns to convert the context
to the dev_base_lock one.)
Currently, sparse doesn't care, but if it's going to check the context
of functions contained within another function then we need to put the
actual __acquire__ together with acquiring the context.
The great benefit of this patch is that you can now document at least
some locking assumptions in a machine-readable way:
before:
/* requires mylock held */
static void myfunc(void)
{...}
after:
static void myfunc(void)
__requires(mylock)
{...}
where, for sparse,
#define __requires(x) __attribute__((context(x,1,1)))
Doing so may result in lots of other functions that need to be annoated
along with it because they also have the same locking requirements, but
ultimately sparse can check a lot of locking assumptions that way.
I have already used this patch and identify a number of kernel bugs by
marking things to require certain locks or RCU-protection and checking
sparse output. To do that, you need a few kernel patches which I'll
send separately.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
inline.c | 12
linearize.c | 10
linearize.h | 2
parse.c | 82 +++++-
parse.h | 3
sparse.1 | 32 +-
sparse.c | 209 +++++++++++++----
symbol.h | 1
validation/context-named.c | 496 +++++++++++++++++++++++++++++++++++++++++
validation/context-statement.c | 58 ++++
validation/context.c | 40 ++-
11 files changed, 860 insertions(+), 85 deletions(-)
--- sparse.orig/sparse.c 2008-04-10 11:02:06.000000000 +0200
+++ sparse/sparse.c 2008-04-10 11:02:42.000000000 +0200
@@ -24,77 +24,184 @@
#include "expression.h"
#include "linearize.h"
-static int context_increase(struct basic_block *bb, int entry)
+struct context_check {
+ int val;
+ char name[32];
+};
+
+DECLARE_ALLOCATOR(context_check);
+DECLARE_PTR_LIST(context_check_list, struct context_check);
+ALLOCATOR(context_check, "context check list");
+
+static const char *unnamed_context = "<unnamed>";
+
+static const char *context_name(struct context *context)
{
- int sum = 0;
- struct instruction *insn;
+ if (context->context && context->context->symbol_name)
+ return show_ident(context->context->symbol_name);
+ return unnamed_context;
+}
- FOR_EACH_PTR(bb->insns, insn) {
- int val;
- if (insn->opcode != OP_CONTEXT)
- continue;
- val = insn->increment;
- if (insn->check) {
- int current = sum + entry;
- if (!val) {
- if (!current)
- continue;
- } else if (current >= val)
- continue;
- warning(insn->pos, "context check failure");
+static void context_add(struct context_check_list **ccl, const char *name, int offs)
+{
+ struct context_check *check, *found = NULL;
+
+ FOR_EACH_PTR(*ccl, check) {
+ if (strcmp(name, check->name))
continue;
- }
- sum += val;
- } END_FOR_EACH_PTR(insn);
- return sum;
+ found = check;
+ break;
+ } END_FOR_EACH_PTR(check);
+
+ if (!found) {
+ found = __alloc_context_check(0);
+ strncpy(found->name, name, sizeof(found->name));
+ found->name[sizeof(found->name) - 1] = '\0';
+ add_ptr_list(ccl, found);
+ }
+ found->val += offs;
}
-static int imbalance(struct entrypoint *ep, struct basic_block *bb, int entry, int exit, const char *why)
+static int imbalance(struct entrypoint *ep, struct position pos, const char *name, const char *why)
{
if (Wcontext) {
struct symbol *sym = ep->name;
- warning(bb->pos, "context imbalance in '%s' - %s", show_ident(sym->ident), why);
+ if (strcmp(name, unnamed_context))
+ warning(pos, "context imbalance in '%s' - %s (%s)", show_ident(sym->ident), why, name);
+ else
+ warning(pos, "context imbalance in '%s' - %s", show_ident(sym->ident), why);
}
return -1;
}
-static int check_bb_context(struct entrypoint *ep, struct basic_block *bb, int entry, int exit);
+static int context_list_check(struct entrypoint *ep, struct position pos,
+ struct context_check_list *ccl_cur,
+ struct context_check_list *ccl_target)
+{
+ struct context_check *c1, *c2;
+ int cur, tgt;
+
+ /* make sure the loop below checks all */
+ FOR_EACH_PTR(ccl_target, c1) {
+ context_add(&ccl_cur, c1->name, 0);
+ } END_FOR_EACH_PTR(c1);
+
+ FOR_EACH_PTR(ccl_cur, c1) {
+ cur = c1->val;
+ tgt = 0;
+
+ FOR_EACH_PTR(ccl_target, c2) {
+ if (strcmp(c2->name, c1->name))
+ continue;
+ tgt = c2->val;
+ break;
+ } END_FOR_EACH_PTR(c2);
+
+ if (cur > tgt)
+ return imbalance(ep, pos, c1->name, "wrong count at exit");
+ else if (cur < tgt)
+ return imbalance(ep, pos, c1->name, "unexpected unlock");
+ } END_FOR_EACH_PTR(c1);
-static int check_children(struct entrypoint *ep, struct basic_block *bb, int entry, int exit)
+ return 0;
+}
+
+static int check_bb_context(struct entrypoint *ep, struct basic_block *bb,
+ struct context_check_list *ccl_in,
+ struct context_check_list *ccl_target)
{
+ struct context_check_list *combined = NULL;
+ struct context_check *c;
struct instruction *insn;
struct basic_block *child;
+ struct context *ctx;
+ const char *name;
+ int ok, val;
+
+ /* recurse in once to catch bad loops */
+ if (bb->context > 0)
+ return 0;
+
+ bb->context++;
+
+ FOR_EACH_PTR(ccl_in, c) {
+ context_add(&combined, c->name, c->val);
+ } END_FOR_EACH_PTR(c);
+
+ FOR_EACH_PTR(bb->insns, insn) {
+ if (!insn->bb)
+ continue;
+ switch (insn->opcode) {
+ case OP_CALL:
+ if (!insn->func || !insn->func->sym || insn->func->type != PSEUDO_SYM)
+ break;
+ FOR_EACH_PTR(insn->func->sym->ctype.contexts, ctx) {
+ name = context_name(ctx);
+ val = 0;
+
+ FOR_EACH_PTR(combined, c) {
+ if (strcmp(c->name, name) == 0) {
+ val = c->val;
+ break;
+ }
+ } END_FOR_EACH_PTR(c);
+
+ if (ctx->exact)
+ ok = ctx->in == val;
+ else
+ ok = ctx->in <= val;
+
+ if (!ok) {
+ const char *call = strdup(show_ident(insn->func->ident));
+ warning(insn->pos, "context problem in '%s' - function '%s' expected different context",
+ show_ident(ep->name->ident), call);
+ free((void *)call);
+ return -1;
+ }
+ } END_FOR_EACH_PTR (ctx);
+ break;
+ case OP_CONTEXT:
+ val = 0;
+
+ name = unnamed_context;
+ if (insn->context_expr)
+ name = show_ident(insn->context_expr->symbol_name);
+
+ FOR_EACH_PTR(combined, c) {
+ if (strcmp(c->name, name) == 0) {
+ val = c->val;
+ break;
+ }
+ } END_FOR_EACH_PTR(c);
+
+ ok = insn->required <= val;
+
+ if (!ok) {
+ name = strdup(name);
+ imbalance(ep, insn->pos, name, "__context__ statement expected different lock context");
+ free((void *)name);
+ return -1;
+ }
+ context_add(&combined, name, insn->increment);
+ break;
+ }
+ } END_FOR_EACH_PTR(insn);
insn = last_instruction(bb->insns);
if (!insn)
return 0;
if (insn->opcode == OP_RET)
- return entry != exit ? imbalance(ep, bb, entry, exit, "wrong count at exit") : 0;
+ return context_list_check(ep, insn->pos, combined, ccl_target);
FOR_EACH_PTR(bb->children, child) {
- if (check_bb_context(ep, child, entry, exit))
+ if (check_bb_context(ep, child, combined, ccl_target))
return -1;
} END_FOR_EACH_PTR(child);
- return 0;
-}
-static int check_bb_context(struct entrypoint *ep, struct basic_block *bb, int entry, int exit)
-{
- if (!bb)
- return 0;
- if (bb->context == entry)
- return 0;
+ /* contents will be freed once we return out of recursion */
+ free_ptr_list(&combined);
- /* Now that's not good.. */
- if (bb->context >= 0)
- return imbalance(ep, bb, entry, bb->context, "different lock contexts for basic block");
-
- bb->context = entry;
- entry += context_increase(bb, entry);
- if (entry < 0)
- return imbalance(ep, bb, entry, exit, "unexpected unlock");
-
- return check_children(ep, bb, entry, exit);
+ return 0;
}
static void check_cast_instruction(struct instruction *insn)
@@ -235,7 +342,7 @@ static void check_context(struct entrypo
{
struct symbol *sym = ep->name;
struct context *context;
- unsigned int in_context = 0, out_context = 0;
+ struct context_check_list *ccl_in = NULL, *ccl_target = NULL;
if (Wuninitialized && verbose && ep->entry->bb->needs) {
pseudo_t pseudo;
@@ -249,10 +356,16 @@ static void check_context(struct entrypo
check_instructions(ep);
FOR_EACH_PTR(sym->ctype.contexts, context) {
- in_context += context->in;
- out_context += context->out;
+ const char *name = context_name(context);
+
+ context_add(&ccl_in, name, context->in);
+ context_add(&ccl_target, name, context->out);
} END_FOR_EACH_PTR(context);
- check_bb_context(ep, ep->entry->bb, in_context, out_context);
+
+ check_bb_context(ep, ep->entry->bb, ccl_in, ccl_target);
+ free_ptr_list(&ccl_in);
+ free_ptr_list(&ccl_target);
+ clear_context_check_alloc();
}
static void check_symbols(struct symbol_list *list)
--- sparse.orig/validation/context.c 2008-04-10 11:02:06.000000000 +0200
+++ sparse/validation/context.c 2008-04-10 11:02:42.000000000 +0200
@@ -314,23 +314,35 @@ static void warn_cond_lock1(void)
condition2 = 1; /* do stuff */
r();
}
+
+static void warn_odd_looping(void)
+{
+ int i;
+
+ for (i = 0; i < 2; i++)
+ a();
+ for (i = 0; i < 2; i++)
+ r();
+}
+
/*
* check-name: Check -Wcontext
*
* check-error-start
-context.c:69:13: warning: context imbalance in 'warn_lock1' - wrong count at exit
-context.c:74:13: warning: context imbalance in 'warn_lock2' - wrong count at exit
-context.c:81:13: warning: context imbalance in 'warn_lock3' - wrong count at exit
-context.c:88:13: warning: context imbalance in 'warn_unlock1' - unexpected unlock
-context.c:93:13: warning: context imbalance in 'warn_unlock2' - unexpected unlock
-context.c:131:12: warning: context imbalance in 'warn_if1' - wrong count at exit
-context.c:140:12: warning: context imbalance in 'warn_if2' - different lock contexts for basic block
-context.c:202:2: warning: context imbalance in 'warn_while1' - different lock contexts for basic block
-context.c:210:3: warning: context imbalance in 'warn_while2' - unexpected unlock
-context.c:216:2: warning: context imbalance in 'warn_while3' - wrong count at exit
-context.c:274:13: warning: context imbalance in 'warn_goto1' - wrong count at exit
-context.c:283:13: warning: context imbalance in 'warn_goto2' - wrong count at exit
-context.c:300:5: warning: context imbalance in 'warn_goto3' - different lock contexts for basic block
-context.c:315:5: warning: context imbalance in 'warn_cond_lock1' - different lock contexts for basic block
+context.c:71:3: warning: context imbalance in 'warn_lock1' - wrong count at exit
+context.c:78:3: warning: context imbalance in 'warn_lock2' - wrong count at exit
+context.c:85:3: warning: context imbalance in 'warn_lock3' - wrong count at exit
+context.c:90:3: warning: context problem in 'warn_unlock1' - function 'r' expected different context
+context.c:97:3: warning: context problem in 'warn_unlock2' - function 'r' expected different context
+context.c:137:9: warning: context imbalance in 'warn_if1' - wrong count at exit
+context.c:147:9: warning: context imbalance in 'warn_if2' - wrong count at exit
+context.c:203:4: warning: context imbalance in 'warn_while1' - wrong count at exit
+context.c:210:4: warning: context problem in 'warn_while2' - function 'r' expected different context
+context.c:220:4: warning: context imbalance in 'warn_while3' - wrong count at exit
+context.c:280:5: warning: context imbalance in 'warn_goto1' - wrong count at exit
+context.c:290:6: warning: context imbalance in 'warn_goto2' - wrong count at exit
+context.c:300:6: warning: context problem in 'warn_goto3' - function 'r' expected different context
+context.c:315:6: warning: context problem in 'warn_cond_lock1' - function 'r' expected different context
+context.c:325:10: warning: context problem in 'warn_odd_looping' - function 'r' expected different context
* check-error-end
*/
--- sparse.orig/parse.c 2008-04-10 11:02:06.000000000 +0200
+++ sparse/parse.c 2008-04-10 11:02:42.000000000 +0200
@@ -66,6 +66,7 @@ static struct token *attribute_address_s
static struct token *attribute_aligned(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *attribute_mode(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *attribute_context(struct token *token, struct symbol *attr, struct ctype *ctype);
+static struct token *attribute_exact_context(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *attribute_transparent_union(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *ignore_attribute(struct token *token, struct symbol *attr, struct ctype *ctype);
@@ -184,6 +185,10 @@ static struct symbol_op context_op = {
.attribute = attribute_context,
};
+static struct symbol_op exact_context_op = {
+ .attribute = attribute_exact_context,
+};
+
static struct symbol_op transparent_union_op = {
.attribute = attribute_transparent_union,
};
@@ -265,6 +270,7 @@ static struct init_keyword {
{ "address_space",NS_KEYWORD, .op = &address_space_op },
{ "mode", NS_KEYWORD, .op = &mode_op },
{ "context", NS_KEYWORD, .op = &context_op },
+ { "exact_context", NS_KEYWORD, .op = &exact_context_op },
{ "__transparent_union__", NS_KEYWORD, .op = &transparent_union_op },
{ "__mode__", NS_KEYWORD, .op = &mode_op },
@@ -863,7 +869,7 @@ static struct token *attribute_mode(stru
return token;
}
-static struct token *attribute_context(struct token *token, struct symbol *attr, struct ctype *ctype)
+static struct token *_attribute_context(struct token *token, struct symbol *attr, struct ctype *ctype, int exact)
{
struct context *context = alloc_context();
struct expression *args[3];
@@ -877,6 +883,8 @@ static struct token *attribute_context(s
break;
if (argc < 3)
args[argc++] = expr;
+ else
+ argc++;
if (!match_op(token, ','))
break;
token = token->next;
@@ -898,8 +906,13 @@ static struct token *attribute_context(s
context->in = get_expression_value(args[1]);
context->out = get_expression_value(args[2]);
break;
+ default:
+ sparse_error(token->pos, "too many arguments to context attribute");
+ break;
}
+ context->exact = exact;
+
if (argc)
add_ptr_list(&ctype->contexts, context);
@@ -907,6 +920,16 @@ static struct token *attribute_context(s
return token;
}
+static struct token *attribute_context(struct token *token, struct symbol *attr, struct ctype *ctype)
+{
+ return _attribute_context(token, attr, ctype, 0);
+}
+
+static struct token *attribute_exact_context(struct token *token, struct symbol *attr, struct ctype *ctype)
+{
+ return _attribute_context(token, attr, ctype, 1);
+}
+
static struct token *attribute_transparent_union(struct token *token, struct symbol *attr, struct ctype *ctype)
{
if (Wtransparent_union)
@@ -1738,17 +1761,56 @@ static struct token *parse_goto_statemen
static struct token *parse_context_statement(struct token *token, struct statement *stmt)
{
+ struct expression *args[3];
+ int argc = 0;
+
stmt->type = STMT_CONTEXT;
- token = parse_expression(token->next, &stmt->expression);
- if(stmt->expression->type == EXPR_PREOP
- && stmt->expression->op == '('
- && stmt->expression->unop->type == EXPR_COMMA) {
- struct expression *expr;
- expr = stmt->expression->unop;
- stmt->context = expr->left;
- stmt->expression = expr->right;
+ token = token->next;
+ token = expect(token, '(', "after __context__ statement");
+ while (!match_op(token, ')')) {
+ struct expression *expr = NULL;
+ token = conditional_expression(token, &expr);
+ if (!expr)
+ break;
+ if (argc < 3)
+ args[argc++] = expr;
+ else
+ argc++;
+ if (!match_op(token, ','))
+ break;
+ token = token->next;
}
- return expect(token, ';', "at end of statement");
+
+ stmt->expression = args[0];
+ stmt->context = NULL;
+
+ switch (argc) {
+ case 0:
+ sparse_error(token->pos, "__context__ statement needs argument(s)");
+ return token;
+ case 1:
+ /* already done */
+ break;
+ case 2:
+ if (args[0]->type != STMT_EXPRESSION) {
+ stmt->context = args[0];
+ stmt->expression = args[1];
+ } else {
+ stmt->expression = args[0];
+ stmt->required = args[1];
+ }
+ break;
+ case 3:
+ stmt->context = args[0];
+ stmt->expression = args[1];
+ stmt->required = args[2];
+ break;
+ default:
+ sparse_error(token->pos, "too many arguments for __context__ statement");
+ return token->next;
+ }
+
+ return expect(token, ')', "at end of __context__");
}
static struct token *parse_range_statement(struct token *token, struct statement *stmt)
--- sparse.orig/symbol.h 2008-04-10 11:02:06.000000000 +0200
+++ sparse/symbol.h 2008-04-10 11:02:42.000000000 +0200
@@ -72,6 +72,7 @@ enum keyword {
struct context {
struct expression *context;
unsigned int in, out;
+ int exact;
};
extern struct context *alloc_context(void);
--- sparse.orig/sparse.1 2008-04-10 11:02:07.000000000 +0200
+++ sparse/sparse.1 2008-04-10 11:02:42.000000000 +0200
@@ -73,20 +73,34 @@ Warn about potential errors in synchroni
Sparse supports several means of designating functions or statements that
delimit contexts, such as synchronization. Functions with the extended
attribute
-.BI __attribute__((context( expression , in_context , out_context ))
-require the context \fIexpression\fR (for instance, a lock) to have the value
+.BI __attribute__((context( [expression ,] in_context , out_context ))
+require the context \fIexpression\fR (for instance, a lock) to have at least the value
\fIin_context\fR (a constant nonnegative integer) when called, and return with
-the value \fIout_context\fR (a constant nonnegative integer). For APIs
-defined via macros, use the statement form
-.BI __context__( expression , in_value , out_value )
-in the body of the macro.
+the value adjusted by \fIout_context - in_context\fR (where
+\fIout_context\fR is a constant nonnegative integer). To change the value
+of a context (for example in macros), use the statement
+.BI __context__( [expression , ]adjust_value[ , required] )
+where \fIadjust_value\fR is a constant integer and \fIrequired\fR is a
+constant nonnegative integer. Not giving \fIrequired\fR is equivalent to
+giving zero and means that the statement does not need the context as a
+precondition, when given it means that the context must at least have the
+value of \fIrequired\fR.
+
+To indicate that a function requires
+.BI exactly
+a certain lock context (not "at least" as above), use the form
+.BI __attribute__((exact_context( [expression ,] in_context , out_context ))
+There currently is no corresponding
+.BI __exact_context__( [expression , ]adjust_value[ , required] )
+statement.
-With \fB-Wcontext\fR Sparse will warn when it sees a function change the
-context without indicating this with a \fBcontext\fR attribute, either by
+Sparse will warn when it sees a function change a
+context without indicating this with a \fBcontext\fR or \fBexact_context\fR attribute, either by
decreasing a context below zero (such as by releasing a lock without acquiring
it), or returning with a changed context (such as by acquiring a lock without
releasing it). Sparse will also warn about blocks of code which may
-potentially execute with different contexts.
+potentially execute with different contexts and about functions that are
+executed without a lock they require.
Sparse issues these warnings by default. To turn them off, use
\fB\-Wno\-context\fR.
--- sparse.orig/inline.c 2008-04-10 11:02:06.000000000 +0200
+++ sparse/inline.c 2008-04-10 11:02:42.000000000 +0200
@@ -331,10 +331,18 @@ static struct statement *copy_one_statem
case STMT_CONTEXT:
case STMT_EXPRESSION: {
struct expression *expr = copy_expression(stmt->expression);
+ struct statement *newstmt;
if (expr == stmt->expression)
break;
- stmt = dup_statement(stmt);
- stmt->expression = expr;
+ newstmt = dup_statement(stmt);
+ newstmt->expression = expr;
+ if (stmt->required) {
+ expr = copy_expression(stmt->required);
+ if (expr == stmt->required)
+ break;
+ newstmt->required = expr;
+ }
+ stmt = newstmt;
break;
}
case STMT_RANGE: {
--- sparse.orig/linearize.c 2008-04-10 11:02:07.000000000 +0200
+++ sparse/linearize.c 2008-04-10 11:02:42.000000000 +0200
@@ -1247,6 +1247,7 @@ static pseudo_t linearize_call_expressio
if (check || context_diff) {
insn = alloc_instruction(OP_CONTEXT, 0);
insn->increment = context_diff;
+ /* what's check for? */
insn->check = check;
insn->context_expr = context->context;
add_one_insn(ep, insn);
@@ -1683,6 +1684,15 @@ static pseudo_t linearize_context(struct
value = expr->value;
insn->increment = value;
+
+ expr = stmt->required;
+ value = 0;
+
+ if (expr && expr->type == EXPR_VALUE)
+ value = expr->value;
+
+ insn->required = value;
+
insn->context_expr = stmt->context;
add_one_insn(ep, insn);
return VOID;
--- sparse.orig/linearize.h 2008-04-10 11:02:07.000000000 +0200
+++ sparse/linearize.h 2008-04-10 11:02:42.000000000 +0200
@@ -116,7 +116,7 @@ struct instruction {
struct pseudo_list *arguments;
};
struct /* context */ {
- int increment;
+ int increment, required;
int check;
struct expression *context_expr;
};
--- sparse.orig/parse.h 2008-04-10 11:02:07.000000000 +0200
+++ sparse/parse.h 2008-04-10 11:02:42.000000000 +0200
@@ -39,9 +39,10 @@ struct statement {
struct symbol *label;
struct statement *label_statement;
};
- struct {
+ struct { /* __context__ */
struct expression *expression;
struct expression *context;
+ struct expression *required;
};
struct /* return_statement */ {
struct expression *ret_value;
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ sparse/validation/context-statement.c 2008-04-10 11:02:42.000000000 +0200
@@ -0,0 +1,58 @@
+#define a() __context__(LOCK, 1)
+#define r() __context__(LOCK, -1)
+#define m() __context__(LOCK, 0, 1)
+#define m2() __context__(LOCK, 0, 2)
+
+static void good_ar(void)
+{
+ a();
+ r();
+}
+
+static void bad_arr(void)
+{
+ a();
+ r();
+ r();
+}
+
+static void good_macro1(void)
+{
+ a();
+ m();
+ r();
+}
+
+static void good_macro2(void)
+{
+ a();
+ a();
+ m();
+ m2();
+ r();
+ r();
+}
+
+static void bad_macro1(void)
+{
+ m();
+ a();
+ r();
+}
+
+static void bad_macro2(void)
+{
+ a();
+ r();
+ m();
+}
+
+/*
+ * check-name: Check __context__ statement with required context
+ *
+ * check-error-start
+context-statement.c:16:8: warning: context imbalance in 'bad_arr' - unexpected unlock (LOCK)
+context-statement.c:38:5: warning: context imbalance in 'bad_macro1' - __context__ statement expected different lock context (LOCK)
+context-statement.c:47:5: warning: context imbalance in 'bad_macro2' - __context__ statement expected different lock context (LOCK)
+ * check-error-end
+ */
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ sparse/validation/context-named.c 2008-04-10 11:02:42.000000000 +0200
@@ -0,0 +1,496 @@
+static void a(void) __attribute__((context(TEST,0,1)))
+{
+ __context__(TEST,1);
+}
+
+static void r(void) __attribute__((context(TEST,1,0)))
+{
+ __context__(TEST,-1,1);
+}
+
+static void a2(void) __attribute__((context(TEST2,0,1)))
+{
+ __context__(TEST2,1);
+}
+
+static void r2(void) __attribute__((context(TEST2,1,0)))
+{
+ __context__(TEST2,-1,1);
+}
+
+#define check_test2() __context__(TEST2,0,1)
+
+static void good_paired1(void)
+{
+ a();
+ a2();
+ r();
+ r2();
+}
+
+static void good_paired2(void)
+{
+ a();
+ r();
+ a();
+ r();
+ a2();
+ r2();
+}
+
+static void good_paired3(void)
+{
+ a();
+ a();
+ r();
+ r();
+ a2();
+ a2();
+ r2();
+ r2();
+}
+
+static void good_lock1(void) __attribute__((context(TEST,0,1)))
+{
+ a();
+}
+
+static void good_lock2(void) __attribute__((context(TEST,0,1)))
+{
+ a();
+ r();
+ a();
+}
+
+static void good_lock3(void) __attribute__((context(TEST,0,1)))
+{
+ a();
+ a();
+ r();
+}
+
+static void good_unlock1(void) __attribute__((context(TEST,1,0)))
+{
+ r();
+}
+
+static void good_unlock2(void) __attribute__((context(TEST,1,0)))
+{
+ a();
+ r();
+ r();
+}
+
+static void warn_lock1(void)
+{
+ a();
+}
+
+static void warn_lock2(void)
+{
+ a();
+ r();
+ a();
+}
+
+static void warn_lock3(void)
+{
+ a();
+ a();
+ r();
+}
+
+static void warn_unlock1(void)
+{
+ r();
+}
+
+static void warn_unlock2(void)
+{
+ a();
+ r();
+ r();
+}
+
+extern int condition, condition2;
+
+static int good_if1(void)
+{
+ a();
+ if(condition) {
+ r();
+ return -1;
+ }
+ r();
+ return 0;
+}
+
+static void good_if2(void)
+{
+ if(condition) {
+ a();
+ r();
+ }
+}
+
+static void good_if3(void)
+{
+ a();
+ if(condition) {
+ a();
+ r();
+ }
+ r();
+}
+
+static int warn_if1(void)
+{
+ a();
+ if(condition)
+ return -1;
+ r();
+ return 0;
+}
+
+static int warn_if2(void)
+{
+ a();
+ if(condition) {
+ r();
+ return -1;
+ }
+ return 0;
+}
+
+static void good_while1(void)
+{
+ a();
+ while(condition)
+ ;
+ r();
+}
+
+static void good_while2(void)
+{
+ while(condition) {
+ a();
+ r();
+ }
+}
+
+static void good_while3(void)
+{
+ while(condition) {
+ a();
+ r();
+ if(condition2)
+ break;
+ a();
+ r();
+ }
+}
+
+static void good_while4(void)
+{
+ a();
+ while(1) {
+ if(condition2) {
+ r();
+ break;
+ }
+ }
+}
+
+static void good_while5(void)
+{
+ a();
+ while(1) {
+ r();
+ if(condition2)
+ break;
+ a();
+ }
+}
+
+static void warn_while1(void)
+{
+ while(condition) {
+ a();
+ }
+}
+
+static void warn_while2(void)
+{
+ while(condition) {
+ r();
+ }
+}
+
+static void warn_while3(void)
+{
+ while(condition) {
+ a();
+ if(condition2)
+ break;
+ r();
+ }
+}
+
+static void good_goto1(void)
+{
+ a();
+ goto label;
+label:
+ r();
+}
+
+static void good_goto2(void)
+{
+ a();
+ goto label;
+ a();
+ r();
+label:
+ r();
+}
+
+static void good_goto3(void)
+{
+ a();
+ if(condition)
+ goto label;
+ a();
+ r();
+label:
+ r();
+}
+
+static void good_goto4(void)
+{
+ if(condition)
+ goto label;
+ a();
+ r();
+label:
+ ;
+}
+
+static void good_goto5(void)
+{
+ a();
+ if(condition)
+ goto label;
+ r();
+ return;
+label:
+ r();
+}
+
+static void warn_goto1(void)
+{
+ a();
+ goto label;
+ r();
+label:
+ ;
+}
+
+static void warn_goto2(void)
+{
+ a();
+ goto label;
+ r();
+label:
+ a();
+ r();
+}
+
+static void warn_goto3(void)
+{
+ a();
+ if(condition)
+ goto label;
+ r();
+label:
+ r();
+}
+
+static void warn_multiple1(void)
+{
+ a();
+ a2();
+}
+
+static void warn_multiple2(void)
+{
+ a2();
+ a();
+}
+
+static void warn_mixed1(void)
+{
+ a2();
+ r();
+}
+
+static void warn_mixed2(void)
+{
+ a2();
+ if (condition) {
+ a();
+ r2();
+ }
+ r();
+}
+
+static void warn_mixed3(void)
+{
+ a2();
+ if (condition) {
+ r2();
+ return;
+ }
+ r();
+}
+
+static void warn_mixed4(void)
+{
+ a2();
+ if (condition) {
+ a();
+ r();
+ return;
+ }
+ r();
+}
+
+static void good_mixed1(void)
+{
+ if (condition) {
+ a();
+ r();
+ } else {
+ a2();
+ r2();
+ }
+}
+
+static void good_mixed2(void)
+{
+ if (condition) {
+ a();
+ r();
+ }
+ a2();
+ r2();
+}
+
+static int need_lock(void) __attribute__((context(TEST,1,1)))
+{
+}
+
+static void need_lock_exact(void) __attribute__((exact_context(TEST,1,1)))
+{
+}
+
+static void need_lock2(void) __attribute__((context(TEST,1,1)))
+{
+ need_lock();
+}
+
+static void good_fn(void)
+{
+ a();
+ need_lock();
+ r();
+}
+
+static void good_fn2(void)
+{
+ a();
+ a();
+ need_lock();
+ r();
+ r();
+}
+
+static void good_fn2(void)
+{
+ a();
+ if (condition)
+ need_lock();
+ r();
+}
+
+static void good_fn3(void) __attribute__((context(TEST,1,1)))
+{
+ if (condition)
+ need_lock2();
+}
+
+static void warn_fn(void)
+{
+ a2();
+ need_lock();
+ r2();
+}
+
+static void warn_fn2(void)
+{
+ a2();
+ need_lock2();
+ r2();
+}
+
+static void good_exact_fn(void)
+{
+ a();
+ need_lock_exact();
+ r();
+}
+
+static void warn_exact_fn1(void)
+{
+ a();
+ a();
+ need_lock_exact();
+ r();
+ r();
+}
+
+static void warn_exact_fn2(void)
+{
+ a2();
+ need_lock_exact();
+ r2();
+}
+
+/*
+ * check-name: Check -Wcontext with lock names
+ *
+ * check-error-start
+context-named.c:86:3: warning: context imbalance in 'warn_lock1' - wrong count at exit (TEST)
+context-named.c:93:3: warning: context imbalance in 'warn_lock2' - wrong count at exit (TEST)
+context-named.c:100:3: warning: context imbalance in 'warn_lock3' - wrong count at exit (TEST)
+context-named.c:105:3: warning: context problem in 'warn_unlock1' - function 'r' expected different context
+context-named.c:112:3: warning: context problem in 'warn_unlock2' - function 'r' expected different context
+context-named.c:152:9: warning: context imbalance in 'warn_if1' - wrong count at exit (TEST)
+context-named.c:162:9: warning: context imbalance in 'warn_if2' - wrong count at exit (TEST)
+context-named.c:218:4: warning: context imbalance in 'warn_while1' - wrong count at exit (TEST)
+context-named.c:225:4: warning: context problem in 'warn_while2' - function 'r' expected different context
+context-named.c:235:4: warning: context imbalance in 'warn_while3' - wrong count at exit (TEST)
+context-named.c:295:5: warning: context imbalance in 'warn_goto1' - wrong count at exit (TEST)
+context-named.c:305:6: warning: context imbalance in 'warn_goto2' - wrong count at exit (TEST)
+context-named.c:315:6: warning: context problem in 'warn_goto3' - function 'r' expected different context
+context-named.c:321:7: warning: context imbalance in 'warn_multiple1' - wrong count at exit (TEST)
+context-named.c:327:6: warning: context imbalance in 'warn_multiple2' - wrong count at exit (TEST2)
+context-named.c:333:6: warning: context problem in 'warn_mixed1' - function 'r' expected different context
+context-named.c:343:6: warning: context problem in 'warn_mixed2' - function 'r' expected different context
+context-named.c:353:6: warning: context problem in 'warn_mixed3' - function 'r' expected different context
+context-named.c:364:6: warning: context imbalance in 'warn_mixed4' - wrong count at exit (TEST2)
+context-named.c:434:14: warning: context problem in 'warn_fn' - function 'need_lock' expected different context
+context-named.c:441:15: warning: context problem in 'warn_fn2' - function 'need_lock2' expected different context
+context-named.c:456:20: warning: context problem in 'warn_exact_fn1' - function 'need_lock_exact' expected different context
+context-named.c:464:20: warning: context problem in 'warn_exact_fn2' - function 'need_lock_exact' expected different context
+ * check-error-end
+ */
--
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 13:25 ` [PATCH 1/3] make sparse keep its promise about context tracking Johannes Berg
@ 2008-04-10 15:24 ` Philipp Reisner
2008-04-10 15:30 ` Johannes Berg
2008-04-21 18:04 ` Josh Triplett
1 sibling, 1 reply; 26+ messages in thread
From: Philipp Reisner @ 2008-04-10 15:24 UTC (permalink / raw)
To: Josh Triplett; +Cc: Johannes Berg, linux-sparse
Am Donnerstag, 10. April 2008 15:25:20 schrieb Johannes Berg:
> The sparse man page promises that it will check this:
>
> Functions with the extended attribute
> __attribute__((context(expression,in_context,out_context))
> require the context expression (for instance, a lock) to have the
> value in_context (a constant nonnegative integer) when called,
> and return with the value out_context (a constant nonnegative
> integer).
>
> It doesn't keep that promise though, nor can it, especially with
> contexts that can be acquired recursively (like RCU in the kernel.)
>
Hi Josh,
Hi Johannes,
I just have implemented nearly the same here. Hopefully Josh will
decide for one of these patches soon.
diff --git a/expression.c b/expression.c
index 289927a..00cfb00 100644
--- a/expression.c
+++ b/expression.c
@@ -929,4 +929,76 @@ struct token *parse_expression(struct token *token,
struct expression **tree)
return comma_expression(token,tree);
}
+int ident_equal(struct ident *ident1, struct ident *ident2)
+{
+ return ident1->len == ident2->len &&
+ !strncmp(ident1->name, ident2->name, ident1->len);
+}
+
+int expressions_equal(struct expression *expr1, struct expression *expr2)
+{
+ if (expr1 == expr2)
+ return 1;
+
+ if (expr1 == NULL || expr2 == NULL)
+ return 0;
+
+ if (expr1->type != expr2->type)
+ return 0;
+
+ switch (expr1->type) {
+ case EXPR_SYMBOL:
+ return ident_equal(expr1->symbol_name, expr2->symbol_name);
+
+ case EXPR_VALUE:
+ return expr1->value == expr2->value;
+
+ case EXPR_FVALUE:
+ return expr1->fvalue == expr2->fvalue;
+
+ case EXPR_STRING:
+ return expr1->string->length == expr2->string->length &&
+ !strncmp(expr1->string->data, expr2->string->data, expr1->string->length);
+
+ case EXPR_BINOP:
+ return expr1->op == expr2->op &&
+ expressions_equal(expr1->left, expr2->left) &&
+ expressions_equal(expr1->right, expr2->right);
+
+ case EXPR_COMMA:
+ case EXPR_ASSIGNMENT:
+ return expressions_equal(expr1->left, expr2->left) &&
+ expressions_equal(expr1->right, expr2->right);
+
+ case EXPR_DEREF:
+ return expressions_equal(expr1->deref, expr2->deref) &&
+ ident_equal(expr1->member, expr2->member);
+
+ case EXPR_PREOP:
+ case EXPR_POSTOP:
+ return expr1->op == expr2->op &&
+ expressions_equal(expr1->unop, expr2->unop);
+
+ /* Not needed right now, but for sake of completness ...
+ case EXPR_LABEL:
+ case EXPR_STATEMENT:
+ case EXPR_CALL:
+ case EXPR_LOGICAL:
+ case EXPR_COMPARE:
+ case EXPR_SELECT:
+ case EXPR_CONDITIONAL:
+ case EXPR_CAST:
+ case EXPR_FORCE_CAST:
+ case EXPR_IMPLIED_CAST:
+ case EXPR_SLICE:
+ case EXPR_INITIALIZER:
+ case EXPR_POS:
+ */
+
+ default:
+ printf("Missing code in expressions_equal for %d\n", expr1->type);
+ }
+
+ return 0;
+}
diff --git a/expression.h b/expression.h
index 5136b9b..e89ddb7 100644
--- a/expression.h
+++ b/expression.h
@@ -216,4 +216,5 @@ struct token *compound_statement(struct token *, struct
statement *);
void cast_value(struct expression *expr, struct symbol *newtype,
struct expression *old, struct symbol *oldtype);
+extern int expressions_equal(struct expression *expr1, struct expression
*expr2);
#endif
diff --git a/linearize.c b/linearize.c
index 8a68f05..5aae3d6 100644
--- a/linearize.c
+++ b/linearize.c
@@ -67,6 +67,7 @@ static struct basic_block *alloc_basic_block(struct
entrypoint *ep, struct posit
{
struct basic_block *bb = __alloc_basic_block(0);
bb->context = -1;
+ bb->context_expr = NULL;
bb->pos = pos;
bb->ep = ep;
return bb;
diff --git a/linearize.h b/linearize.h
index 7b2961b..a71f9aa 100644
--- a/linearize.h
+++ b/linearize.h
@@ -225,6 +225,7 @@ struct basic_block {
struct position pos;
unsigned long generation;
int context;
+ struct expression *context_expr;
struct entrypoint *ep;
struct basic_block_list *parents; /* sources */
struct basic_block_list *children; /* destinations */
diff --git a/sparse.c b/sparse.c
index 4026ba7..50c5e51 100644
--- a/sparse.c
+++ b/sparse.c
@@ -24,7 +24,110 @@
#include "expression.h"
#include "linearize.h"
-static int context_increase(struct basic_block *bb, int entry)
+
+int ident_str(struct ident *ident, char *buffer, int length)
+{
+ return snprintf(buffer, length, "%.*s", ident->len, ident->name);
+}
+
+
+int expression_str(struct expression *expr, char *buffer, int length)
+{
+ int n;
+
+ if (!expr) {
+ buffer[0] = 0;
+ return 0;
+ }
+
+ /* TODO, think about necessary braces () */
+
+ switch (expr->type) {
+ case EXPR_SYMBOL:
+ return ident_str(expr->symbol_name, buffer, length);
+
+ case EXPR_VALUE:
+ return snprintf(buffer, length, "%llu", expr->value);
+
+ case EXPR_FVALUE:
+ return snprintf(buffer, length, "%Lf", expr->fvalue);
+
+ case EXPR_STRING:
+ return snprintf(buffer, length, "%.*s", expr->string->length,
expr->string->data);
+
+ case EXPR_BINOP:
+ n = expression_str(expr->left, buffer, length);
+ n += snprintf(buffer+n, length-n, "%c", expr->op);
+ n += expression_str(expr->right, buffer+n, length-n);
+ return n;
+
+ case EXPR_COMMA:
+ n = expression_str(expr->left, buffer, length);
+ n += snprintf(buffer+n, length-n, ",");
+ n += expression_str(expr->right, buffer+n, length-n);
+ return n;
+
+ case EXPR_ASSIGNMENT:
+ n = expression_str(expr->left, buffer, length);
+ n += snprintf(buffer+n, length-n, "=");
+ n += expression_str(expr->right, buffer+n, length-n);
+ return n;
+
+ case EXPR_DEREF:
+ if (expr->left->type == EXPR_PREOP &&
+ expr->left->op == '*') {
+ n = expression_str(expr->left->unop, buffer, length);
+ n += snprintf(buffer+n, length-n, "->");
+ n += ident_str(expr->member, buffer+n, length-n);
+ } else {
+ n = expression_str(expr->left, buffer, length);
+ n += snprintf(buffer+n, length-n, ".");
+ n += ident_str(expr->member, buffer+n, length-n);
+ }
+ return n;
+
+ case EXPR_PREOP:
+ n = snprintf(buffer, length, "%c", expr->op);
+ n += expression_str(expr->unop, buffer+n, length-n);
+ return n;
+
+ case EXPR_POSTOP:
+ n = expression_str(expr->unop, buffer, length);
+ n += snprintf(buffer+n, length-n, "%c", expr->op);
+ return n;
+
+ /* Not needed right now, but for sake of completness ...
+ case EXPR_LABEL:
+ case EXPR_STATEMENT:
+ case EXPR_CALL:
+ case EXPR_LOGICAL:
+ case EXPR_COMPARE:
+ case EXPR_SELECT:
+ case EXPR_CONDITIONAL:
+ case EXPR_CAST:
+ case EXPR_FORCE_CAST:
+ case EXPR_IMPLIED_CAST:
+ case EXPR_SLICE:
+ case EXPR_INITIALIZER:
+ case EXPR_POS:
+ */
+
+ default:
+ printf("Missing code in expression_str for %d\n", expr->type);
+ }
+
+ return 0;
+}
+
+char *expression_sstr(struct expression *expr)
+{
+ static char expr_string[37] = " for ";
+ static char empty[] = "";
+
+ return expression_str(expr, expr_string+5, 32) ? expr_string : empty;
+}
+
+static int context_increase(struct basic_block *bb, struct expression *expr,
int entry)
{
int sum = 0;
struct instruction *insn;
@@ -33,6 +136,8 @@ static int context_increase(struct basic_block *bb, int
entry)
int val;
if (insn->opcode != OP_CONTEXT)
continue;
+ if (!expressions_equal(expr, insn->context_expr))
+ continue;
val = insn->increment;
if (insn->check) {
int current = sum + entry;
@@ -49,18 +154,19 @@ static int context_increase(struct basic_block *bb, int
entry)
return sum;
}
-static int imbalance(struct entrypoint *ep, struct basic_block *bb, int
entry, int exit, const char *why)
+static int imbalance(struct basic_block *bb, struct expression *expr, int
entry, int exit, const char *why)
{
if (Wcontext) {
- struct symbol *sym = ep->name;
- warning(bb->pos, "context imbalance in '%s' - %s", show_ident(sym->ident),
why);
+ struct symbol *sym = bb->ep->name;
+ warning(bb->pos, "context imbalance in '%s' - %s%s",
show_ident(sym->ident), why,
+ expression_sstr(expr));
}
return -1;
}
-static int check_bb_context(struct entrypoint *ep, struct basic_block *bb,
int entry, int exit);
+static int check_bb_context(struct basic_block *bb, struct expression *expr,
int entry, int exit);
-static int check_children(struct entrypoint *ep, struct basic_block *bb, int
entry, int exit)
+static int check_children(struct basic_block *bb, struct expression *expr,
int entry, int exit)
{
struct instruction *insn;
struct basic_block *child;
@@ -69,32 +175,36 @@ static int check_children(struct entrypoint *ep, struct
basic_block *bb, int ent
if (!insn)
return 0;
if (insn->opcode == OP_RET)
- return entry != exit ? imbalance(ep, bb, entry, exit, "wrong count at
exit") : 0;
+ return entry != exit ? imbalance(bb, expr, entry, exit, "wrong count at
exit") : 0;
FOR_EACH_PTR(bb->children, child) {
- if (check_bb_context(ep, child, entry, exit))
+ if (check_bb_context(child, expr, entry, exit))
return -1;
} END_FOR_EACH_PTR(child);
return 0;
}
-static int check_bb_context(struct entrypoint *ep, struct basic_block *bb,
int entry, int exit)
+static int check_bb_context(struct basic_block *bb, struct expression *expr,
int entry, int exit)
{
+ int eq;
+
if (!bb)
return 0;
- if (bb->context == entry)
+ eq = expressions_equal(expr, bb->context_expr);
+ if (eq && bb->context == entry)
return 0;
/* Now that's not good.. */
- if (bb->context >= 0)
- return imbalance(ep, bb, entry, bb->context, "different lock contexts for
basic block");
+ if (eq && bb->context >= 0)
+ return imbalance(bb, expr, entry, bb->context, "different lock contexts for
basic block");
bb->context = entry;
- entry += context_increase(bb, entry);
+ bb->context_expr = expr;
+ entry += context_increase(bb, expr, entry);
if (entry < 0)
- return imbalance(ep, bb, entry, exit, "unexpected unlock");
+ return imbalance(bb, expr, entry, exit, "unexpected unlock");
- return check_children(ep, bb, entry, exit);
+ return check_children(bb, expr, entry, exit);
}
static void check_cast_instruction(struct instruction *insn)
@@ -195,7 +305,31 @@ static void check_call_instruction(struct instruction
*insn)
}
}
-static void check_one_instruction(struct instruction *insn)
+static void add_expression_once(struct expression_list **expr_list, struct
expression* expr)
+{
+ struct expression *e;
+
+ FOR_EACH_PTR(*expr_list, e) {
+ if (expressions_equal(e, expr))
+ return;
+ } END_FOR_EACH_PTR(e);
+
+ add_expression(expr_list, expr);
+}
+
+static void remove_expression(struct expression_list **expr_list, struct
expression* expr)
+{
+ struct expression *e;
+
+ FOR_EACH_PTR(*expr_list, e) {
+ if (expressions_equal(e, expr)) {
+ DELETE_CURRENT_PTR(e);
+ return;
+ }
+ } END_FOR_EACH_PTR(e);
+}
+
+static void check_one_instruction(struct instruction *insn, struct
expression_list **expr_list)
{
switch (insn->opcode) {
case OP_CAST: case OP_SCAST:
@@ -208,26 +342,29 @@ static void check_one_instruction(struct instruction
*insn)
case OP_CALL:
check_call_instruction(insn);
break;
+ case OP_CONTEXT:
+ add_expression_once(expr_list, insn->context_expr);
+ break;
default:
break;
}
}
-static void check_bb_instructions(struct basic_block *bb)
+static void check_bb_instructions(struct basic_block *bb, struct
expression_list **expr_list)
{
struct instruction *insn;
FOR_EACH_PTR(bb->insns, insn) {
if (!insn->bb)
continue;
- check_one_instruction(insn);
+ check_one_instruction(insn, expr_list);
} END_FOR_EACH_PTR(insn);
}
-static void check_instructions(struct entrypoint *ep)
+static void check_instructions(struct entrypoint *ep, struct expression_list
**expr_list)
{
struct basic_block *bb;
FOR_EACH_PTR(ep->bbs, bb) {
- check_bb_instructions(bb);
+ check_bb_instructions(bb, expr_list);
} END_FOR_EACH_PTR(bb);
}
@@ -235,7 +372,8 @@ static void check_context(struct entrypoint *ep)
{
struct symbol *sym = ep->name;
struct context *context;
- unsigned int in_context = 0, out_context = 0;
+ struct expression_list *expr_list = NULL;
+ struct expression *expr;
if (Wuninitialized && verbose && ep->entry->bb->needs) {
pseudo_t pseudo;
@@ -246,13 +384,16 @@ static void check_context(struct entrypoint *ep)
} END_FOR_EACH_PTR(pseudo);
}
- check_instructions(ep);
+ check_instructions(ep, &expr_list);
FOR_EACH_PTR(sym->ctype.contexts, context) {
- in_context += context->in;
- out_context += context->out;
+ remove_expression(&expr_list, context->context);
+ check_bb_context(ep->entry->bb, context->context, context->in,
context->out);
} END_FOR_EACH_PTR(context);
- check_bb_context(ep, ep->entry->bb, in_context, out_context);
+
+ FOR_EACH_PTR(expr_list, expr) {
+ check_bb_context(ep->entry->bb, expr, 0, 0);
+ } END_FOR_EACH_PTR(expr);
}
static void check_symbols(struct symbol_list *list)
diff --git a/validation/named_context.c b/validation/named_context.c
new file mode 100644
index 0000000..9b6d77f
--- /dev/null
+++ b/validation/named_context.c
@@ -0,0 +1,32 @@
+#ifdef __CHECKER__
+# define __acquires(x) __attribute__((context(x,0,1)))
+# define __releases(x) __attribute__((context(x,1,0)))
+# define __acquire(x) __context__(x,1)
+# define __release(x) __context__(x,-1)
+#else
+# define __acquires(x)
+# define __releases(x)
+# define __acquire(x) (void)0
+# define __release(x) (void)0
+#endif
+
+static void invalid1(void)
+{
+ __acquire(lock1);
+ __release(lock2);
+}
+
+static void global_lock(void) __acquires(lock1) __acquires(lock2)
+{
+ __acquire(lock1);
+ __acquire(lock2);
+}
+
+/*
+ * check-name: Check -Wcontext
+ *
+ * check-error-start
+named_context.c:13:13: warning: context imbalance in 'invalid1' - wrong count
at exit for lock1
+named_context.c:13:13: warning: context imbalance in 'invalid1' - unexpected
unlock for lock2
+ * check-error-end
+ */
--
: 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 :
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 15:24 ` Philipp Reisner
@ 2008-04-10 15:30 ` Johannes Berg
2008-04-10 15:46 ` Philipp Reisner
2008-04-21 19:22 ` Josh Triplett
0 siblings, 2 replies; 26+ messages in thread
From: Johannes Berg @ 2008-04-10 15:30 UTC (permalink / raw)
To: Philipp Reisner; +Cc: Josh Triplett, linux-sparse
[-- Attachment #1: Type: text/plain, Size: 713 bytes --]
Hi Philip,
> I just have implemented nearly the same here. Hopefully Josh will
> decide for one of these patches soon.
> +int ident_equal(struct ident *ident1, struct ident *ident2)
> +int expressions_equal(struct expression *expr1, struct expression *expr2)
That code looks pretty nice, I guess I should look at getting that into
my version instead of just printing the identifier to a string.
As far as I can see your version doesn't actually implement
__attribute__((context(x,1,1))) as the man-page envisioned it for
checking that a function is run under the lock context it wants, which
was one of the more important goals to me. Doing separate locks sort of
fell out.
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 15:30 ` Johannes Berg
@ 2008-04-10 15:46 ` Philipp Reisner
2008-04-10 15:51 ` Johannes Berg
2008-04-10 15:54 ` Johannes Berg
2008-04-21 19:22 ` Josh Triplett
1 sibling, 2 replies; 26+ messages in thread
From: Philipp Reisner @ 2008-04-10 15:46 UTC (permalink / raw)
To: Johannes Berg; +Cc: Josh Triplett, linux-sparse
Am Donnerstag, 10. April 2008 17:30:54 schrieb Johannes Berg:
> Hi Philip,
>
> > I just have implemented nearly the same here. Hopefully Josh will
> > decide for one of these patches soon.
> >
> >
> > +int ident_equal(struct ident *ident1, struct ident *ident2)
> >
> > +int expressions_equal(struct expression *expr1, struct expression
> > *expr2)
>
> That code looks pretty nice, I guess I should look at getting that into
> my version instead of just printing the identifier to a string.
>
> As far as I can see your version doesn't actually implement
> __attribute__((context(x,1,1))) as the man-page envisioned it for
> checking that a function is run under the lock context it wants, which
> was one of the more important goals to me. Doing separate locks sort of
> fell out.
>
Hi Johannes,
I also worked on that part, although I have to admitt that I did not
got that part of the manpage. Instead I invented the require_context
attribute.
Here is the second patch. It applies on top of the first one...
I hope that we get the good ideas of our two works combined and
accepted into sparse...
-Phil
diff --git a/allocate.c b/allocate.c
index 5cc52a9..198297b 100644
--- a/allocate.c
+++ b/allocate.c
@@ -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");
diff --git a/allocate.h b/allocate.h
index 9f1dc8c..8fa3efd 100644
--- a/allocate.h
+++ b/allocate.h
@@ -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);
diff --git a/linearize.c b/linearize.c
index 5aae3d6..e47f862 100644
--- a/linearize.c
+++ b/linearize.c
@@ -240,6 +240,7 @@ static const char *opcodes[] = {
/* Sparse tagging (line numbers, context, whatever) */
[OP_CONTEXT] = "context",
+ [OP_CONTEXT_REQ] = "context req",
[OP_RANGE] = "range-check",
[OP_COPY] = "copy",
@@ -442,6 +443,9 @@ const char *show_instruction(struct instruction *insn)
case OP_CONTEXT:
buf += sprintf(buf, "%s%d", insn->check ? "check: " : "", insn->increment);
break;
+ case OP_CONTEXT_REQ:
+ buf += sprintf(buf, "%d to %d, type %d", insn->ctx_req->min, insn->ctx_req->max, insn->access_type);
+ break;
case OP_RANGE:
buf += sprintf(buf, "%s between %s..%s", show_pseudo(insn->src1), show_pseudo(insn->src2), show_pseudo(insn->src3));
break;
@@ -839,6 +843,7 @@ struct access_data {
unsigned int offset, alignment; // byte offset
unsigned int bit_size, bit_offset; // which bits
struct position pos;
+ struct ctx_req_list *ctx_reqs; // required contexts
};
static void finish_address_gen(struct entrypoint *ep, struct access_data *ad)
@@ -887,6 +892,8 @@ static int linearize_address_gen(struct entrypoint *ep,
if (!ctype)
return 0;
+
+ ad->ctx_reqs = ctype->ctype.ctx_reqs;
ad->pos = expr->pos;
ad->result_type = ctype;
ad->source_type = base_type(ctype);
@@ -938,6 +945,7 @@ static pseudo_t linearize_store_gen(struct entrypoint *ep,
struct access_data *ad)
{
pseudo_t store = value;
+ struct context_requirement *ctx_req;
if (type_size(ad->source_type) != type_size(ad->result_type)) {
pseudo_t orig = add_load(ep, ad);
@@ -951,6 +959,19 @@ static pseudo_t linearize_store_gen(struct entrypoint *ep,
orig = add_binary_op(ep, ad->source_type, OP_AND, orig, value_pseudo(~mask));
store = add_binary_op(ep, ad->source_type, OP_OR, orig, store);
}
+ if (ad->ctx_reqs) {
+ struct instruction *insn;
+ FOR_EACH_PTR(ad->ctx_reqs, ctx_req) {
+ if (ctx_req->access_type == WRITE ||
+ ctx_req->access_type == RDWR) {
+ insn = alloc_instruction(OP_CONTEXT_REQ, 0);
+ insn->ctx_req = ctx_req;
+ insn->access_type = WRITE;
+ add_one_insn(ep, insn);
+ }
+ } END_FOR_EACH_PTR(ctx_req);
+
+ }
add_store(ep, ad, store);
return value;
}
@@ -989,14 +1010,30 @@ static pseudo_t add_symbol_address(struct entrypoint *ep, struct symbol *sym)
static pseudo_t linearize_load_gen(struct entrypoint *ep, struct access_data *ad)
{
- pseudo_t new = add_load(ep, ad);
+ pseudo_t new;
+ struct context_requirement *ctx_req;
+
+ if (ad->ctx_reqs) {
+ struct instruction *insn;
+ FOR_EACH_PTR(ad->ctx_reqs, ctx_req) {
+ if (ctx_req->access_type == READ ||
+ ctx_req->access_type == RDWR) {
+ insn = alloc_instruction(OP_CONTEXT_REQ, 0);
+ insn->ctx_req = ctx_req;
+ insn->access_type = READ;
+ add_one_insn(ep, insn);
+ }
+ } END_FOR_EACH_PTR(ctx_req);
+
+ }
+ new = add_load(ep, ad);
if (ad->bit_offset) {
pseudo_t shift = value_pseudo(ad->bit_offset);
pseudo_t newval = add_binary_op(ep, ad->source_type, OP_LSR, new, shift);
new = newval;
}
-
+
return new;
}
@@ -1195,6 +1232,7 @@ static pseudo_t linearize_call_expression(struct entrypoint *ep, struct expressi
pseudo_t retval, call;
struct ctype *ctype = NULL;
struct context *context;
+ struct context_requirement *ctx_req;
if (!expr->ctype) {
warning(expr->pos, "call with no type!");
@@ -1211,6 +1249,17 @@ static pseudo_t linearize_call_expression(struct entrypoint *ep, struct expressi
if (fn->ctype)
ctype = &fn->ctype->ctype;
+ if (ctype) {
+ FOR_EACH_PTR(ctype->ctx_reqs, ctx_req) {
+ struct instruction *cinsn;
+
+ cinsn = alloc_instruction(OP_CONTEXT_REQ, 0);
+ cinsn->ctx_req = ctx_req;
+ cinsn->access_type = CALL;
+ add_one_insn(ep, cinsn);
+ } END_FOR_EACH_PTR(ctx_req);
+ }
+
if (fn->type == EXPR_PREOP) {
if (fn->unop->type == EXPR_SYMBOL) {
struct symbol *sym = fn->unop->symbol;
@@ -1236,6 +1285,7 @@ static pseudo_t linearize_call_expression(struct entrypoint *ep, struct expressi
int out = context->out;
int check = 0;
int context_diff;
+
if (in < 0) {
check = 1;
in = 0;
diff --git a/linearize.h b/linearize.h
index a71f9aa..8901a82 100644
--- a/linearize.h
+++ b/linearize.h
@@ -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 */
diff --git a/parse.c b/parse.c
index 6255737..6a8e1ac 100644
--- a/parse.c
+++ b/parse.c
@@ -66,6 +66,7 @@ static struct token *attribute_address_space(struct token *token, struct symbol
static struct token *attribute_aligned(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *attribute_mode(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *attribute_context(struct token *token, struct symbol *attr, struct ctype *ctype);
+static struct token *attribute_require_context(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *attribute_transparent_union(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *ignore_attribute(struct token *token, struct symbol *attr, struct ctype *ctype);
@@ -184,6 +185,10 @@ static struct symbol_op context_op = {
.attribute = attribute_context,
};
+static struct symbol_op require_context_op = {
+ .attribute = attribute_require_context,
+};
+
static struct symbol_op transparent_union_op = {
.attribute = attribute_transparent_union,
};
@@ -265,6 +270,7 @@ static struct init_keyword {
{ "address_space",NS_KEYWORD, .op = &address_space_op },
{ "mode", NS_KEYWORD, .op = &mode_op },
{ "context", NS_KEYWORD, .op = &context_op },
+ { "require_context", NS_KEYWORD, .op = &require_context_op },
{ "__transparent_union__", NS_KEYWORD, .op = &transparent_union_op },
{ "__mode__", NS_KEYWORD, .op = &mode_op },
@@ -907,6 +913,72 @@ static struct token *attribute_context(struct token *token, struct symbol *attr,
return token;
}
+static int token_to_accesstype(struct token *token)
+{
+ static const char *at_text[] = {
+ [READ] = "\"read\"",
+ [WRITE] = "\"write\"",
+ [RDWR] = "\"rdwr\"",
+ [CALL] = "\"call\""
+ };
+ const char *ats;
+ int i;
+
+ if (token_type(token) != TOKEN_STRING)
+ goto err;
+
+ ats = show_token(token);
+ for (i = 0; i < sizeof at_text/sizeof at_text[0]; i++) {
+ if (!strcmp(ats,at_text[i]))
+ return i;
+ }
+
+err:
+ sparse_error(token->pos, "Expected \"read\", \"write\", \"rdwr\" or \"call\""
+ " as 4th argument of require_context");
+
+ return RDWR;
+}
+
+static struct token *attribute_require_context(struct token *token, struct symbol *attr, struct ctype *ctype)
+{
+ struct context_requirement *ctx_req = alloc_context_requirement();
+ struct expression *args[3];
+ int argc = 0;
+
+ token = expect(token, '(', "after require_context attribute");
+ while (!match_op(token, ')')) {
+ struct expression *expr = NULL;
+ if (argc < 3) {
+ token = conditional_expression(token, &expr);
+ if (!expr)
+ break;
+ } else {
+ ctx_req->access_type = token_to_accesstype(token);
+ token = token->next;
+ argc++;
+ }
+
+ if (argc < 3)
+ args[argc++] = expr;
+ if (!match_op(token, ','))
+ break;
+ token = token->next;
+ }
+
+ if (argc == 4) {
+ ctx_req->context = args[0];
+ ctx_req->min = get_expression_value(args[1]);
+ ctx_req->max = get_expression_value(args[2]);
+ add_ptr_list(&ctype->ctx_reqs, ctx_req);
+ } else
+ sparse_error(token->pos, "expected context name, min, max and type values");
+
+ token = expect(token, ')', "after context attribute");
+ return token;
+}
+
+
static struct token *attribute_transparent_union(struct token *token, struct symbol *attr, struct ctype *ctype)
{
if (Wtransparent_union)
@@ -1039,6 +1111,8 @@ static void apply_ctype(struct position pos, struct ctype *thistype, struct ctyp
/* Context */
concat_ptr_list((struct ptr_list *)thistype->contexts,
(struct ptr_list **)&ctype->contexts);
+ concat_ptr_list((struct ptr_list *)thistype->ctx_reqs,
+ (struct ptr_list **)&ctype->ctx_reqs);
/* Alignment */
if (thistype->alignment & (thistype->alignment-1)) {
@@ -1246,6 +1320,8 @@ static struct token *pointer(struct token *token, struct ctype *ctype)
ptr->ctype.as = ctype->as;
concat_ptr_list((struct ptr_list *)ctype->contexts,
(struct ptr_list **)&ptr->ctype.contexts);
+ concat_ptr_list((struct ptr_list *)ctype->ctx_reqs,
+ (struct ptr_list **)&ptr->ctype.ctx_reqs);
ptr->ctype.base_type = base_type;
base_type = ptr;
@@ -1253,6 +1329,7 @@ static struct token *pointer(struct token *token, struct ctype *ctype)
ctype->base_type = base_type;
ctype->as = 0;
free_ptr_list(&ctype->contexts);
+ free_ptr_list(&ctype->ctx_reqs);
token = declaration_specifiers(token->next, ctype, 1);
modifiers = ctype->modifiers;
diff --git a/sparse.c b/sparse.c
index 50c5e51..45a1394 100644
--- a/sparse.c
+++ b/sparse.c
@@ -24,13 +24,11 @@
#include "expression.h"
#include "linearize.h"
-
int ident_str(struct ident *ident, char *buffer, int length)
{
return snprintf(buffer, length, "%.*s", ident->len, ident->name);
}
-
int expression_str(struct expression *expr, char *buffer, int length)
{
int n;
@@ -127,6 +125,22 @@ char *expression_sstr(struct expression *expr)
return expression_str(expr, expr_string+5, 32) ? expr_string : empty;
}
+static void out_of_context_access(struct basic_block *bb, struct expression *expr,
+ struct instruction *insn, const char *why)
+{
+ static const char *at_text[] = {
+ [READ] = "read access",
+ [WRITE] = "write access",
+ [CALL] = "call"
+ };
+
+ if (Wcontext) {
+ warning(insn->pos, "out of context %s in '%s' - context too %s%s",
+ at_text[insn->access_type], show_ident(bb->ep->name->ident), why,
+ expression_sstr(expr));
+ }
+}
+
static int context_increase(struct basic_block *bb, struct expression *expr, int entry)
{
int sum = 0;
@@ -134,22 +148,33 @@ static int context_increase(struct basic_block *bb, struct expression *expr, int
FOR_EACH_PTR(bb->insns, insn) {
int val;
- if (insn->opcode != OP_CONTEXT)
- continue;
- if (!expressions_equal(expr, insn->context_expr))
- continue;
- val = insn->increment;
- if (insn->check) {
- int current = sum + entry;
- if (!val) {
- if (!current)
+
+ switch (insn->opcode) {
+ case OP_CONTEXT:
+ if (!expressions_equal(expr, insn->context_expr))
+ continue;
+ val = insn->increment;
+ if (insn->check) {
+ int current = sum + entry;
+ if (!val) {
+ if (!current)
+ continue;
+ } else if (current >= val)
continue;
- } else if (current >= val)
+ warning(insn->pos, "context check failure");
continue;
- warning(insn->pos, "context check failure");
- continue;
+ }
+ sum += val;
+ break;
+ case OP_CONTEXT_REQ:
+ if (!expressions_equal(expr, insn->ctx_req->context))
+ continue;
+ if (sum+entry < insn->ctx_req->min)
+ out_of_context_access(bb, expr, insn, "small");
+ if (sum+entry > insn->ctx_req->max)
+ out_of_context_access(bb, expr, insn, "high");
+ break;
}
- sum += val;
} END_FOR_EACH_PTR(insn);
return sum;
}
@@ -345,6 +370,8 @@ static void check_one_instruction(struct instruction *insn, struct expression_li
case OP_CONTEXT:
add_expression_once(expr_list, insn->context_expr);
break;
+ case OP_CONTEXT_REQ:
+ add_expression_once(expr_list, insn->ctx_req->context);
default:
break;
}
diff --git a/symbol.c b/symbol.c
index 7539817..4564843 100644
--- a/symbol.c
+++ b/symbol.c
@@ -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);
diff --git a/symbol.h b/symbol.h
index 4362f9a..cdb2452 100644
--- a/symbol.h
+++ b/symbol.h
@@ -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;
};
diff --git a/validation/context_requirement.c b/validation/context_requirement.c
new file mode 100644
index 0000000..24fe598
--- /dev/null
+++ b/validation/context_requirement.c
@@ -0,0 +1,185 @@
+#ifdef __CHECKER__
+# define __acquires(x) __attribute__((context(x,0,1)))
+# define __releases(x) __attribute__((context(x,1,0)))
+# define __acquire(x) __context__(x,1)
+# define __release(x) __context__(x,-1)
+# define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0)
+# define __protected_by(x) __attribute__((require_context(x,1,1,"rdwr")))
+# define __protected_read_by(x) __attribute__((require_context(x,1,1,"read")))
+# define __protected_write_by(x) __attribute__((require_context(x,1,1,"write")))
+# define __must_hold(x) __attribute__((context(x,1,1), require_context(x,1,1,"call")))
+#else
+# define __acquires(x)
+# define __releases(x)
+# define __acquire(x) (void)0
+# define __release(x) (void)0
+# define __cond_lock(x,c) (c)
+# define __protected_by(x)
+# define __protected_read_by(x)
+# define __protected_write_by(x)
+# define __must_hold(x)
+#endif
+
+
+#include <stdio.h>
+#include <stdlib.h>
+
+static int to_protect __protected_by(local);
+
+static int local_cnt=0;
+
+static void inc_local(void) __acquires(local)
+{
+ __acquire(local);
+ local_cnt++;
+}
+
+
+#define try_inc_local() __cond_lock(local, _try_inc_local())
+static int _try_inc_local(void)
+{
+ if (random() > RAND_MAX/2) {
+ local_cnt++;
+ return 1;
+ }
+ return 0;
+}
+
+#define dec_local(void) do { __release(local); _dec_local(); } while(0)
+static void _dec_local(void)
+{
+ local_cnt--;
+}
+
+static int valid1(void)
+{
+ inc_local();
+ to_protect = 1;
+ dec_local();
+}
+
+static int valid2(void)
+{
+ inc_local();
+ inc_local();
+ dec_local();
+ dec_local();
+}
+
+static int valid3(void)
+{
+ if (try_inc_local()) {
+ dec_local();
+ }
+}
+
+static int valid4(void)
+{
+ if ((random() > RAND_MAX/2) && try_inc_local()) {
+ dec_local();
+ }
+}
+
+static int helper(void) __must_hold(local)
+{
+ to_protect = 1;
+}
+
+static int valid5(void)
+{
+ inc_local();
+ helper();
+ dec_local();
+}
+
+static int invalid0(void) __releases(local)
+{
+ if (random() > RAND_MAX/2)
+ dec_local();
+}
+
+static int invalid1(void)
+{
+ inc_local();
+}
+
+static int invalid2(void)
+{
+ dec_local();
+}
+
+static int invalid3(void)
+{
+ if (try_inc_local()) {
+ }
+ dec_local();
+}
+
+static int invalid4(void)
+{
+ while (random() > RAND_MAX/2)
+ inc_local();
+
+ dec_local();
+}
+
+static int invalid5(void)
+{
+ try_inc_local();
+ dec_local();
+}
+
+static int invalid6(void)
+{
+ to_protect = 1;
+ inc_local();
+ dec_local();
+}
+
+static int invalid7(void)
+{
+ inc_local();
+ dec_local();
+ to_protect = 1;
+}
+
+static int invalid8(void)
+{
+ inc_local();
+ inc_local();
+ to_protect = 2;
+ dec_local();
+ dec_local();
+}
+
+static int invalid9(void)
+{
+ int lv;
+
+ lv = to_protect;
+ inc_local();
+ dec_local();
+}
+
+static int invalid10(void)
+{
+ helper();
+}
+
+/*
+ * check-name: Check -Wcontext
+ *
+ * check-error-start
+context_requirement.c:97:2: warning: context imbalance in 'invalid0' - different lock contexts for basic block
+context_requirement.c:101:12: warning: context imbalance in 'invalid1' - wrong count at exit
+context_requirement.c:106:12: warning: context imbalance in 'invalid2' - unexpected unlock
+context_requirement.c:113:6: warning: context imbalance in 'invalid3' - different lock contexts for basic block
+context_requirement.c:120:2: warning: context imbalance in 'invalid4' - different lock contexts for basic block
+context_requirement.c:129:2: warning: context imbalance in 'invalid5' - different lock contexts for basic block
+context_requirement.c:134:15: warning: out of context write access in 'invalid6' - context too small
+context_requirement.c:143:15: warning: out of context write access in 'invalid7' - context too small
+context_requirement.c:150:15: warning: out of context write access in 'invalid8' - context too high
+context_requirement.c:159:7: warning: out of context read access in 'invalid9' - context too small
+context_requirement.c:166:8: warning: out of context call in 'invalid10' - context too small
+ * check-error-end
+ */
--
: 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 :
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 15:46 ` Philipp Reisner
@ 2008-04-10 15:51 ` Johannes Berg
2008-04-10 16:05 ` Philipp Reisner
2008-04-10 15:54 ` Johannes Berg
1 sibling, 1 reply; 26+ messages in thread
From: Johannes Berg @ 2008-04-10 15:51 UTC (permalink / raw)
To: Philipp Reisner; +Cc: Josh Triplett, linux-sparse
[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]
> I also worked on that part, although I have to admitt that I did not
> got that part of the manpage. Instead I invented the require_context
> attribute.
Heh.
> Here is the second patch. It applies on top of the first one...
I don't really have time to look through it right now, sorry.
> I hope that we get the good ideas of our two works combined and
> accepted into sparse...
That would be good :)
> +# define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0)
> +# define __protected_by(x) __attribute__((require_context(x,1,1,"rdwr")))
> +# define __protected_read_by(x) __attribute__((require_context(x,1,1,"read")))
> +# define __protected_write_by(x) __attribute__((require_context(x,1,1,"write")))
> +# define __must_hold(x) __attribute__((context(x,1,1), require_context(x,1,1,"call")))
What's this "rdwr" etc. for? And "call"? Also, how are you planning to
handle nested contexts, where 1,1 doesn't cut it any >0,>0 is really
more like what we need?
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 15:51 ` Johannes Berg
@ 2008-04-10 16:05 ` Philipp Reisner
2008-04-10 16:12 ` Johannes Berg
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Philipp Reisner @ 2008-04-10 16:05 UTC (permalink / raw)
To: Johannes Berg; +Cc: Josh Triplett, linux-sparse
Am Donnerstag, 10. April 2008 17:51:27 schrieb Johannes Berg:
> > I also worked on that part, although I have to admitt that I did not
> > got that part of the manpage. Instead I invented the require_context
> > attribute.
>
> Heh.
>
> > Here is the second patch. It applies on top of the first one...
>
> I don't really have time to look through it right now, sorry.
>
> > I hope that we get the good ideas of our two works combined and
> > accepted into sparse...
>
> That would be good :)
>
>
> What's this "rdwr" etc. for? And "call"? Also, how are you planning to
> handle nested contexts, where 1,1 doesn't cut it any >0,>0 is really
> more like what we need?
>
I also use it to check a recusive lock mechanism.
The sematics of what I implemented is:
You simply use the __attribute__((context(ctx,in,out))) to
annotate the context changes of functions.
to annotate a variable, struct/union member or a function that
a certain locking primitive is required for accessing it, you
do it by
__attribute__((require_context(ctx,min,max,"type")))
ctx ... the expression that describes the locking primitive
min ... the minimum of locks required of that locking primitive
max ... the maximum allowed of locks of that locking primitive.
type .. read, write, rdwr or call for the access type.
So you can express you need to hold this and that locking
primitive to write to something. But an other locking
primitive might be sufficient for reading that something.
The annotation for a variable foo protected by a recursive lock
bar would be:
int foo __attribute__((require_context(bar,1,99999,"rdwr")))
PS: I just realized that there my e-mail client introduced linebreakes
in the first patch I posted. I will repost on request of course.
-Phil
--
: 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 :
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 16:05 ` Philipp Reisner
@ 2008-04-10 16:12 ` Johannes Berg
2008-04-10 21:21 ` Philipp Reisner
2008-04-11 11:06 ` Johannes Berg
2008-04-21 19:34 ` Josh Triplett
2 siblings, 1 reply; 26+ messages in thread
From: Johannes Berg @ 2008-04-10 16:12 UTC (permalink / raw)
To: Philipp Reisner; +Cc: Josh Triplett, linux-sparse
[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]
> I also use it to check a recusive lock mechanism.
>
> The sematics of what I implemented is:
> You simply use the __attribute__((context(ctx,in,out))) to
> annotate the context changes of functions.
Yeah, mostly because that's how the man page documents it :)
> to annotate a variable, struct/union member or a function that
> a certain locking primitive is required for accessing it, you
> do it by
>
> __attribute__((require_context(ctx,min,max,"type")))
>
> ctx ... the expression that describes the locking primitive
> min ... the minimum of locks required of that locking primitive
> max ... the maximum allowed of locks of that locking primitive.
> type .. read, write, rdwr or call for the access type.
>
> So you can express you need to hold this and that locking
> primitive to write to something. But an other locking
> primitive might be sufficient for reading that something.
>
> The annotation for a variable foo protected by a recursive lock
> bar would be:
>
> int foo __attribute__((require_context(bar,1,99999,"rdwr")))
Ah. Makes sense as well, though now you can't do something like "require
context count two and decrement by one", can you? Or do you just do that
by combining the various attributes?
Anyhow, I don't really care which patch gets chosen.
Though, how can you actually track multiple contexts within a single
function if you have just a "struct expression *context_expr" for each
basic block? I guess I should just try your code :)
johannes
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 16:12 ` Johannes Berg
@ 2008-04-10 21:21 ` Philipp Reisner
2008-04-11 19:53 ` Josh Triplett
0 siblings, 1 reply; 26+ messages in thread
From: Philipp Reisner @ 2008-04-10 21:21 UTC (permalink / raw)
To: Johannes Berg; +Cc: Josh Triplett, linux-sparse
> >
> > int foo __attribute__((require_context(bar,1,99999,"rdwr")))
>
> Ah. Makes sense as well, though now you can't do something like "require
> context count two and decrement by one", can you? Or do you just do that
> by combining the various attributes?
Right, by combining a context and a require_context attribute.
>
> Anyhow, I don't really care which patch gets chosen.
>
Nor do I really care. I just hope that there is some reaction from Josh
that indicates how he wishes to continue with that toppic...
I am also ready to do further work in this area.
> Though, how can you actually track multiple contexts within a single
> function if you have just a "struct expression *context_expr" for each
> basic block? I guess I should just try your code :)
>
In the check_context() I build a expression_list of all the context
expressions that are of interest for that entrypoint. Then check_bb_context()
is called for each of these expressions that describe a locking primitive.
-Phil
--
: 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 :
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 21:21 ` Philipp Reisner
@ 2008-04-11 19:53 ` Josh Triplett
2008-04-18 12:35 ` Johannes Berg
0 siblings, 1 reply; 26+ messages in thread
From: Josh Triplett @ 2008-04-11 19:53 UTC (permalink / raw)
To: Philipp Reisner; +Cc: Johannes Berg, linux-sparse
[-- Attachment #1: Type: text/plain, Size: 547 bytes --]
Philipp Reisner wrote:
>> Anyhow, I don't really care which patch gets chosen.
>
> Nor do I really care. I just hope that there is some reaction from Josh
> that indicates how he wishes to continue with that toppic...
> I am also ready to do further work in this area.
Definitely interested in the patches, just spoiled for choice.
However, having now reviewed the various proposals, I think I will
accept Johannes Berg's patches to support his corresponding proposed
changes in Linux's include/linux/compiler.h.
- Josh Triplett
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-11 19:53 ` Josh Triplett
@ 2008-04-18 12:35 ` Johannes Berg
0 siblings, 0 replies; 26+ messages in thread
From: Johannes Berg @ 2008-04-18 12:35 UTC (permalink / raw)
To: Josh Triplett; +Cc: Philipp Reisner, linux-sparse
[-- Attachment #1: Type: text/plain, Size: 696 bytes --]
On Fri, 2008-04-11 at 12:53 -0700, Josh Triplett wrote:
> Philipp Reisner wrote:
> >> Anyhow, I don't really care which patch gets chosen.
> >
> > Nor do I really care. I just hope that there is some reaction from Josh
> > that indicates how he wishes to continue with that toppic...
> > I am also ready to do further work in this area.
>
> Definitely interested in the patches, just spoiled for choice.
> However, having now reviewed the various proposals, I think I will
> accept Johannes Berg's patches to support his corresponding proposed
> changes in Linux's include/linux/compiler.h.
Do you want me to fold the later cleanup patches into the first
two/three?
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 16:05 ` Philipp Reisner
2008-04-10 16:12 ` Johannes Berg
@ 2008-04-11 11:06 ` Johannes Berg
2008-04-21 19:34 ` Josh Triplett
2 siblings, 0 replies; 26+ messages in thread
From: Johannes Berg @ 2008-04-11 11:06 UTC (permalink / raw)
To: Philipp Reisner; +Cc: Josh Triplett, linux-sparse
[-- Attachment #1: Type: text/plain, Size: 4224 bytes --]
Hi,
Managed to take a closer look now.
> > What's this "rdwr" etc. for? And "call"? Also, how are you planning to
> > handle nested contexts, where 1,1 doesn't cut it any >0,>0 is really
> > more like what we need?
> >
>
> I also use it to check a recusive lock mechanism.
>
> The sematics of what I implemented is:
> You simply use the __attribute__((context(ctx,in,out))) to
> annotate the context changes of functions.
>
> to annotate a variable, struct/union member or a function that
> a certain locking primitive is required for accessing it, you
> do it by
>
> __attribute__((require_context(ctx,min,max,"type")))
>
> ctx ... the expression that describes the locking primitive
> min ... the minimum of locks required of that locking primitive
> max ... the maximum allowed of locks of that locking primitive.
> type .. read, write, rdwr or call for the access type.
>
> So you can express you need to hold this and that locking
> primitive to write to something. But an other locking
> primitive might be sufficient for reading that something.
>
> The annotation for a variable foo protected by a recursive lock
> bar would be:
>
> int foo __attribute__((require_context(bar,1,99999,"rdwr")))
To be honest, that confuses me. I haven't been able to come up with a
way to make this test case I have result in a warning:
static void bad_macro1(void)
{
m();
a();
r();
}
where a() is acquire(), r() is release() and m() should require the
lock, and all should be implemented as macros. I even tried playing with
dummy inline functions that are called from the macros.
As for read/write, what's wrong with encoding that in the context?
Something like
void read_lock(something) __attribute__((context(read_something,0,1)));
void write_lock(something) __attribute__((context(write_something,0,1)));
or similar. And if it has to be a parameter, why a string?
Another thing I actually like a lot about the fact that my patch changes
the warnings is that it tells you
- which function caused the error
- where that function is
I recently had the pleasure of digging through a huge switch statement
that looked like this:
function {
[lots of code]
lock();
[lots more code]
switch(something) {
case A:
[lots of code]
unlock();
[lots of code]
break;
[repeat a dozen times]
case M:
[lots of code]
unlock();
[lots of code]
// *** NOTE MISSING break;
case N:
[lots of code]
unlock(); // <----- this is where my code tells you the error is
[lots of code]
break;
[repeat another dozen times]
}
[lots more code in the function]
} // <--- this is where current sparse tells you the error is
Also, my code tells you the function name, and not just "oh, some lock
context was wrong" :) I have just created another patch on top of mine
that will make the error look like this:
context.c:360:10: warning: context problem in 'warn_huge_switch': 'r' expected different context
context.c:360:10: default context: wanted >= 1, got 0
I like the cond_lock() trick, but I was actually planning on expanding
the checking to switch() statements as well and I don't think we can
make it work there with such a trick.
Maybe something like
__attribute__((conditional_context(ctx,req,default,val=ctxchange,...)))
where you could have e.g.
__attribute__((conditional_context(L,1,0,1=1,-1=-1)))
Not sure yet though whether that is useful or not.
Finally, I think you have a bit of an inconsistency: You introduced the
require_context attribute, yet your context() macro still partially
fulfils that role, i.e. if you say that a function decreases a context
then you require that it gets one when called. Hence, extending that to
arbitrary context changes defined via the context attribute seems more
natural to me than to require using the require_context attribute for
positive assertions and the context attribute for negative assertions.
> PS: I just realized that there my e-mail client introduced linebreakes
> in the first patch I posted. I will repost on request of course.
No worries, was easy enough to fix up.
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 16:05 ` Philipp Reisner
2008-04-10 16:12 ` Johannes Berg
2008-04-11 11:06 ` Johannes Berg
@ 2008-04-21 19:34 ` Josh Triplett
2008-04-21 19:37 ` Johannes Berg
2 siblings, 1 reply; 26+ messages in thread
From: Josh Triplett @ 2008-04-21 19:34 UTC (permalink / raw)
To: Philipp Reisner; +Cc: Johannes Berg, linux-sparse
[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]
Philipp Reisner wrote:
> The sematics of what I implemented is:
> You simply use the __attribute__((context(ctx,in,out))) to
> annotate the context changes of functions.
>
> to annotate a variable, struct/union member or a function that
> a certain locking primitive is required for accessing it, you
> do it by
>
> __attribute__((require_context(ctx,min,max,"type")))
>
> ctx ... the expression that describes the locking primitive
> min ... the minimum of locks required of that locking primitive
> max ... the maximum allowed of locks of that locking primitive.
> type .. read, write, rdwr or call for the access type.
>
> So you can express you need to hold this and that locking
> primitive to write to something. But an other locking
> primitive might be sufficient for reading that something.
>
> The annotation for a variable foo protected by a recursive lock
> bar would be:
>
> int foo __attribute__((require_context(bar,1,99999,"rdwr")))
I would *love* to see this work applied on top of the patches I
applied from Johannes. I really want the ability to mark a variable
or struct field as requiring a context to access.
I would probably call this new attribute something like
"data_context". Also, I'd prefer an explicit way of saying "no
maximum context", rather than the hack of using a large number.
I hestiate to introduce a third semantic for a pair of numbers
describing a context. The context attribute uses in and out contexts,
__context__ uses a delta and a required minimum, and this proposed
attribute uses a minimum and a maximum.
Similar to GCC's format attribute using the identifier printf rather
than the string "printf", I think I'd prefer the last argument as an
identifier. Also, a minor nit: could you please use "readwrite"
rather than "rdwr"?
- Josh Triplett
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 15:46 ` Philipp Reisner
2008-04-10 15:51 ` Johannes Berg
@ 2008-04-10 15:54 ` Johannes Berg
1 sibling, 0 replies; 26+ messages in thread
From: Johannes Berg @ 2008-04-10 15:54 UTC (permalink / raw)
To: Philipp Reisner; +Cc: Josh Triplett, linux-sparse
[-- Attachment #1: Type: text/plain, Size: 376 bytes --]
> +# define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0)
> +#define try_inc_local() __cond_lock(local, _try_inc_local())
> +static int _try_inc_local(void)
> +{
> + if (random() > RAND_MAX/2) {
> + local_cnt++;
> + return 1;
> + }
> + return 0;
> +}
That actually works? I guess it shows that I haven't slept enough while
working on this.
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 15:30 ` Johannes Berg
2008-04-10 15:46 ` Philipp Reisner
@ 2008-04-21 19:22 ` Josh Triplett
1 sibling, 0 replies; 26+ messages in thread
From: Josh Triplett @ 2008-04-21 19:22 UTC (permalink / raw)
To: Johannes Berg; +Cc: Philipp Reisner, linux-sparse
[-- Attachment #1: Type: text/plain, Size: 532 bytes --]
Johannes Berg wrote:
>> I just have implemented nearly the same here. Hopefully Josh will
>> decide for one of these patches soon.
>
>> +int ident_equal(struct ident *ident1, struct ident *ident2)
>
>> +int expressions_equal(struct expression *expr1, struct expression *expr2)
>
> That code looks pretty nice, I guess I should look at getting that into
> my version instead of just printing the identifier to a string.
I would love to see this work applied on top of the merged patches as well.
- Josh Triplett
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-10 13:25 ` [PATCH 1/3] make sparse keep its promise about context tracking Johannes Berg
2008-04-10 15:24 ` Philipp Reisner
@ 2008-04-21 18:04 ` Josh Triplett
2008-04-21 18:11 ` Johannes Berg
1 sibling, 1 reply; 26+ messages in thread
From: Josh Triplett @ 2008-04-21 18:04 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-sparse
[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]
One comment on your patch description, by the way:
Johannes Berg wrote:
> However, the kernel use of __attribute__((context(...)) is actually
> wrong, the kernel often does things like:
>
> static void *dev_mc_seq_start(struct seq_file *seq, loff_t * pos)
> __acquires(dev_base_lock)
> {
> [...]
> read_lock(&dev_base_lock);
> [...]
> }
>
> rather than
>
> static void *dev_mc_seq_start(struct seq_file *seq, loff_t * pos)
> __acquires(dev_base_lock)
> {
> [...]
> __acquire__(dev_base_lock);
> read_lock(&dev_base_lock);
> [...]
> }
I don't understand why you count this as wrong. read_lock should
handle acquiring the context itself, either via an __acquires
annotation if written as a function, or via an __acquire__ statement
if written as a macro. Callers of read_lock shouldn't need to
explicitly call __acquire__.
- Josh Triplett
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-21 18:04 ` Josh Triplett
@ 2008-04-21 18:11 ` Johannes Berg
2008-04-21 18:26 ` Josh Triplett
0 siblings, 1 reply; 26+ messages in thread
From: Johannes Berg @ 2008-04-21 18:11 UTC (permalink / raw)
To: Josh Triplett; +Cc: linux-sparse
[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]
> > static void *dev_mc_seq_start(struct seq_file *seq, loff_t * pos)
> > __acquires(dev_base_lock)
> > {
> > [...]
> > __acquire__(dev_base_lock);
> > read_lock(&dev_base_lock);
> > [...]
> > }
>
> I don't understand why you count this as wrong. read_lock should
> handle acquiring the context itself, either via an __acquires
> annotation if written as a function, or via an __acquire__ statement
> if written as a macro. Callers of read_lock shouldn't need to
> explicitly call __acquire__.
Well, the question is how you want to name things. What I did is that
the context identifier is just a name and bears no other relation to the
code. Hence, read_lock() can't say that it acquires 'dev_base_lock'
because it doesn't know what the name should be.
With a slightly different sparse patch than mine you could declare
read_lock() something like this:
#define read_lock(x) do { \
__acquire__(x); \
__read_lock((x)); \
} while (0)
but then you'd have different names everywhere, say somebody passed
'&dev_base_lock' and somebody else used
readlock_t *dbl = &dev_base_lock;
read_lock(dbl)
and you'd end up with one context named "dbl" and another one named
"&dev_base_lock".
If you have suggestions on how to solve this I'm all ears, so far I
decided it wasn't worth it and opted for explicitly naming all the
contexts.
(with my patch the above would just create a context called "x")
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-21 18:11 ` Johannes Berg
@ 2008-04-21 18:26 ` Josh Triplett
2008-04-21 18:30 ` Johannes Berg
0 siblings, 1 reply; 26+ messages in thread
From: Josh Triplett @ 2008-04-21 18:26 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-sparse
[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]
Johannes Berg wrote:
>>> static void *dev_mc_seq_start(struct seq_file *seq, loff_t * pos)
>>> __acquires(dev_base_lock)
>>> {
>>> [...]
>>> __acquire__(dev_base_lock);
>>> read_lock(&dev_base_lock);
>>> [...]
>>> }
>> I don't understand why you count this as wrong. read_lock should
>> handle acquiring the context itself, either via an __acquires
>> annotation if written as a function, or via an __acquire__ statement
>> if written as a macro. Callers of read_lock shouldn't need to
>> explicitly call __acquire__.
>
> Well, the question is how you want to name things. What I did is that
> the context identifier is just a name and bears no other relation to the
> code. Hence, read_lock() can't say that it acquires 'dev_base_lock'
> because it doesn't know what the name should be.
>
> With a slightly different sparse patch than mine you could declare
> read_lock() something like this:
>
> #define read_lock(x) do { \
> __acquire__(x); \
> __read_lock((x)); \
> } while (0)
>
> but then you'd have different names everywhere, say somebody passed
> '&dev_base_lock' and somebody else used
>
> readlock_t *dbl = &dev_base_lock;
> read_lock(dbl)
>
> and you'd end up with one context named "dbl" and another one named
> "&dev_base_lock".
That might still work, depending on how consistently kernel code uses
the same argument. If you suggest explicitly changing calls to
read_lock to call __acquire__ with the appropriate context, it might
prove equally easy to make the argument of read_lock that context.
> If you have suggestions on how to solve this I'm all ears, so far I
> decided it wasn't worth it and opted for explicitly naming all the
> contexts.
>
> (with my patch the above would just create a context called "x")
That doesn't make sense to me; after preprocessing, x no longer
exists, so I don't see how you could pick up the identifier "x". I
can understand that you might pick up two different expressions in
place of x which you can't easily compare, though.
And as for how to solve it: I think alias analysis might work.
- Josh Triplett
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-21 18:26 ` Josh Triplett
@ 2008-04-21 18:30 ` Johannes Berg
2008-04-21 18:51 ` Josh Triplett
0 siblings, 1 reply; 26+ messages in thread
From: Johannes Berg @ 2008-04-21 18:30 UTC (permalink / raw)
To: Josh Triplett; +Cc: linux-sparse
[-- Attachment #1: Type: text/plain, Size: 874 bytes --]
> > and you'd end up with one context named "dbl" and another one named
> > "&dev_base_lock".
>
> That might still work, depending on how consistently kernel code uses
> the same argument. If you suggest explicitly changing calls to
> read_lock to call __acquire__ with the appropriate context, it might
> prove equally easy to make the argument of read_lock that context.
True.
> That doesn't make sense to me; after preprocessing, x no longer
> exists, so I don't see how you could pick up the identifier "x". I
> can understand that you might pick up two different expressions in
> place of x which you can't easily compare, though.
Oops, yes, preprocessing will remove it, of course. But yeah, you can
end up with two different expressions.
> And as for how to solve it: I think alias analysis might work.
How would I do that?
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] make sparse keep its promise about context tracking
2008-04-21 18:30 ` Johannes Berg
@ 2008-04-21 18:51 ` Josh Triplett
0 siblings, 0 replies; 26+ messages in thread
From: Josh Triplett @ 2008-04-21 18:51 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-sparse
[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]
Johannes Berg wrote:
>> And as for how to solve it: I think alias analysis might work.
>
> How would I do that?
Real compilers have alias analysis code, which builds up information
on which variables and expressions may/must alias each other. I think
that that alias information might prove sufficient to compare
contexts: if two contexts alias each other then they represent the
same context.
Basic type-based alias analysis should prove sufficient to handle
cases like:
spinlock_t s;
struct something_with_a_spinlock st;
spin_lock(&st.s);
spin_unlock(&st.s); /* contexts match */
spin_lock(&st.s);
spin_unlock(&s); /* warn */
Flow-based alias analysis may also handle cases like you mentioned,
where you assign a lock to a variable and then use it through that
variable.
Alternatively, some simple dataflow analysis and symbolic execution
might let you figure out the lock a variable points to in most cases.
As a simpler solution, you could just do what lockdep does: make
assumptions that handle the common cases, and allow explicit overrides
for the uncommon cases. I suspect that most code does not assign a
lock to a variable and use the lock through that variable.
In particular, you could perhaps make the assumption that any
expression which resolves to the "foo" field of a "struct bar"
represents the same context, rather than attempting to distinguish
between them.
- Josh Triplett
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/3] sparse test suite: add test mixing __context__ and __attribute__((context(...)))
2008-04-10 13:25 [PATCH 0/3] improve context handling Johannes Berg
2008-04-10 13:25 ` [PATCH 1/3] make sparse keep its promise about context tracking Johannes Berg
@ 2008-04-10 13:25 ` Johannes Berg
2008-04-10 13:25 ` [PATCH 3/3] sparse: simple conditional context tracking Johannes Berg
` (3 subsequent siblings)
5 siblings, 0 replies; 26+ messages in thread
From: Johannes Berg @ 2008-04-10 13:25 UTC (permalink / raw)
To: Josh Triplett; +Cc: linux-sparse
[-- Attachment #1: context-named-mixed-goto.patch --]
[-- Type: text/plain, Size: 828 bytes --]
An earlier version of the next patch had a bug that this test catches.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
validation/context-named.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
--- sparse.orig/validation/context-named.c 2008-04-10 14:57:26.000000000 +0200
+++ sparse/validation/context-named.c 2008-04-10 15:23:16.000000000 +0200
@@ -465,6 +465,27 @@ static void warn_exact_fn2(void)
r2();
}
+#define __acquire(x) __context__(x,1)
+#define __release(x) __context__(x,-1)
+
+#define rl() \
+ do { __acquire(RCU); } while (0)
+
+#define ru() \
+ do { __release(RCU); } while (0)
+
+static void good_mixed_with_if(void)
+{
+ rl();
+
+ if (condition) {
+ a();
+ r();
+ }
+
+ ru();
+}
+
/*
* check-name: Check -Wcontext with lock names
*
--
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 3/3] sparse: simple conditional context tracking
2008-04-10 13:25 [PATCH 0/3] improve context handling Johannes Berg
2008-04-10 13:25 ` [PATCH 1/3] make sparse keep its promise about context tracking Johannes Berg
2008-04-10 13:25 ` [PATCH 2/3] sparse test suite: add test mixing __context__ and __attribute__((context(...))) Johannes Berg
@ 2008-04-10 13:25 ` Johannes Berg
2008-04-11 11:07 ` [PATCH 4/3] inlined call bugfix & test Johannes Berg
` (2 subsequent siblings)
5 siblings, 0 replies; 26+ messages in thread
From: Johannes Berg @ 2008-04-10 13:25 UTC (permalink / raw)
To: Josh Triplett; +Cc: linux-sparse
[-- Attachment #1: conditional.patch --]
[-- Type: text/plain, Size: 13899 bytes --]
This patch enables a very simple form of conditional context tracking,
namely something like
if (spin_trylock(...)) {
[...]
spin_unlock(...);
}
Note that
__ret = spin_trylock(...);
if (__ret) {
[...]
spin_unlock(...);
}
does /not/ work since that would require tracking the variable and doing
extra checks to ensure the variable isn't globally accessible or similar
which could lead to race conditions.
To declare a trylock, one uses:
int spin_trylock(...) __attribute__((conditional_context(spinlock,0,1,0)))
{...}
Note that doing this currently excludes that function itself from context
checking completely.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
linearize.c | 22 +----
linearize.h | 3
parse.c | 52 +++++++++++++
sparse.1 | 9 ++
sparse.c | 54 ++++++++++----
symbol.h | 2
validation/context-dynamic.c | 161 +++++++++++++++++++++++++++++++++++++++++++
7 files changed, 270 insertions(+), 33 deletions(-)
--- sparse.orig/sparse.c 2008-04-10 15:21:21.000000000 +0200
+++ sparse/sparse.c 2008-04-10 15:23:33.000000000 +0200
@@ -25,7 +25,7 @@
#include "linearize.h"
struct context_check {
- int val;
+ int val, val_false;
char name[32];
};
@@ -42,7 +42,7 @@ static const char *context_name(struct c
return unnamed_context;
}
-static void context_add(struct context_check_list **ccl, const char *name, int offs)
+static void context_add(struct context_check_list **ccl, const char *name, int offs, int offs_false)
{
struct context_check *check, *found = NULL;
@@ -60,6 +60,7 @@ static void context_add(struct context_c
add_ptr_list(ccl, found);
}
found->val += offs;
+ found->val_false += offs_false;
}
static int imbalance(struct entrypoint *ep, struct position pos, const char *name, const char *why)
@@ -83,7 +84,7 @@ static int context_list_check(struct ent
/* make sure the loop below checks all */
FOR_EACH_PTR(ccl_target, c1) {
- context_add(&ccl_cur, c1->name, 0);
+ context_add(&ccl_cur, c1->name, 0, 0);
} END_FOR_EACH_PTR(c1);
FOR_EACH_PTR(ccl_cur, c1) {
@@ -108,12 +109,13 @@ static int context_list_check(struct ent
static int check_bb_context(struct entrypoint *ep, struct basic_block *bb,
struct context_check_list *ccl_in,
- struct context_check_list *ccl_target)
+ struct context_check_list *ccl_target,
+ int in_false)
{
struct context_check_list *combined = NULL;
struct context_check *c;
struct instruction *insn;
- struct basic_block *child;
+ struct multijmp *mj;
struct context *ctx;
const char *name;
int ok, val;
@@ -125,7 +127,10 @@ static int check_bb_context(struct entry
bb->context++;
FOR_EACH_PTR(ccl_in, c) {
- context_add(&combined, c->name, c->val);
+ if (in_false)
+ context_add(&combined, c->name, c->val_false, c->val_false);
+ else
+ context_add(&combined, c->name, c->val, c->val);
} END_FOR_EACH_PTR(c);
FOR_EACH_PTR(bb->insns, insn) {
@@ -182,7 +187,23 @@ static int check_bb_context(struct entry
free((void *)name);
return -1;
}
- context_add(&combined, name, insn->increment);
+
+ context_add(&combined, name, insn->increment, insn->inc_false);
+ break;
+ case OP_BR:
+ if (insn->bb_true)
+ if (check_bb_context(ep, insn->bb_true, combined, ccl_target, 0))
+ return -1;
+ if (insn->bb_false)
+ if (check_bb_context(ep, insn->bb_false, combined, ccl_target, 1))
+ return -1;
+ break;
+ case OP_SWITCH:
+ case OP_COMPUTEDGOTO:
+ FOR_EACH_PTR(insn->multijmp_list, mj) {
+ if (check_bb_context(ep, mj->target, combined, ccl_target, 0))
+ return -1;
+ } END_FOR_EACH_PTR(mj);
break;
}
} END_FOR_EACH_PTR(insn);
@@ -193,10 +214,12 @@ static int check_bb_context(struct entry
if (insn->opcode == OP_RET)
return context_list_check(ep, insn->pos, combined, ccl_target);
- FOR_EACH_PTR(bb->children, child) {
- if (check_bb_context(ep, child, combined, ccl_target))
- return -1;
- } END_FOR_EACH_PTR(child);
+ FOR_EACH_PTR(bb->insns, insn) {
+ if (!insn->bb)
+ continue;
+ switch (insn->opcode) {
+ }
+ } END_FOR_EACH_PTR(insn);
/* contents will be freed once we return out of recursion */
free_ptr_list(&combined);
@@ -358,11 +381,14 @@ static void check_context(struct entrypo
FOR_EACH_PTR(sym->ctype.contexts, context) {
const char *name = context_name(context);
- context_add(&ccl_in, name, context->in);
- context_add(&ccl_target, name, context->out);
+ context_add(&ccl_in, name, context->in, context->in);
+ context_add(&ccl_target, name, context->out, context->out_false);
+ /* we don't currently check the body of trylock functions */
+ if (context->out != context->out_false)
+ return;
} END_FOR_EACH_PTR(context);
- check_bb_context(ep, ep->entry->bb, ccl_in, ccl_target);
+ check_bb_context(ep, ep->entry->bb, ccl_in, ccl_target, 0);
free_ptr_list(&ccl_in);
free_ptr_list(&ccl_target);
clear_context_check_alloc();
--- sparse.orig/symbol.h 2008-04-10 15:21:21.000000000 +0200
+++ sparse/symbol.h 2008-04-10 15:22:43.000000000 +0200
@@ -71,7 +71,7 @@ enum keyword {
struct context {
struct expression *context;
- unsigned int in, out;
+ unsigned int in, out, out_false;
int exact;
};
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ sparse/validation/context-dynamic.c 2008-04-10 15:22:43.000000000 +0200
@@ -0,0 +1,161 @@
+static void a(void) __attribute__ ((context(A, 0, 1)))
+{
+ __context__(A, 1);
+}
+
+static void r(void) __attribute__ ((context(A, 1, 0)))
+{
+ __context__(A, -1);
+}
+
+extern int condition, condition2;
+
+static int tl(void) __attribute__ ((conditional_context(A, 0, 1, 0)))
+{
+ if (condition) {
+ a();
+ return 1;
+ }
+ return 0;
+}
+
+static int tl2(void) __attribute__ ((conditional_context(A, 0, 0, 1)))
+{
+ if (condition) {
+ a();
+ return 1;
+ }
+ return 0;
+}
+
+static int dummy(void)
+{
+ return condition + condition2;
+}
+
+static int good_trylock1(void)
+{
+ if (tl()) {
+ r();
+ }
+}
+
+static int good_trylock2(void)
+{
+ if (tl()) {
+ r();
+ }
+
+ if (tl()) {
+ r();
+ }
+}
+static int good_trylock3(void)
+{
+ a();
+ if (tl()) {
+ r();
+ }
+ r();
+ if (tl()) {
+ r();
+ }
+}
+
+static int good_trylock4(void)
+{
+ a();
+ if (tl()) {
+ r();
+ }
+ if (tl()) {
+ r();
+ }
+ r();
+}
+
+static void bad_trylock1(void)
+{
+ a();
+ if (dummy()) {
+ r();
+ }
+ r();
+}
+
+static int good_trylock5(void)
+{
+ if (!tl2()) {
+ r();
+ }
+}
+
+static int good_trylock6(void)
+{
+ if (!tl2()) {
+ r();
+ }
+
+ if (!tl2()) {
+ r();
+ }
+}
+static int good_trylock7(void)
+{
+ a();
+ if (!tl2()) {
+ r();
+ }
+ r();
+ if (!tl2()) {
+ r();
+ }
+}
+
+static int good_trylock8(void)
+{
+ a();
+ if (!tl2()) {
+ r();
+ }
+ if (!tl2()) {
+ r();
+ }
+ r();
+}
+
+static void bad_trylock2(void)
+{
+ a();
+ if (!dummy()) {
+ r();
+ }
+ r();
+}
+
+static int good_switch(void)
+{
+ switch (condition) {
+ case 1:
+ a();
+ break;
+ case 2:
+ a();
+ break;
+ case 3:
+ a();
+ break;
+ default:
+ a();
+ }
+ r();
+}
+
+/*
+ * check-name: Check -Wcontext with lock trylocks
+ *
+ * check-error-start
+context-dynamic.c:83:6: warning: context problem in 'bad_trylock1' - function 'r' expected different context
+context-dynamic.c:133:6: warning: context problem in 'bad_trylock2' - function 'r' expected different context
+ * check-error-end
+ */
--- sparse.orig/linearize.c 2008-04-10 15:21:21.000000000 +0200
+++ sparse/linearize.c 2008-04-10 15:23:22.000000000 +0200
@@ -439,7 +439,7 @@ const char *show_instruction(struct inst
break;
case OP_CONTEXT:
- buf += sprintf(buf, "%s%d", insn->check ? "check: " : "", insn->increment);
+ buf += sprintf(buf, "%s%d,%d", "", insn->increment, insn->inc_false);
break;
case OP_RANGE:
buf += sprintf(buf, "%s between %s..%s", show_pseudo(insn->src1), show_pseudo(insn->src2), show_pseudo(insn->src3));
@@ -1233,23 +1233,12 @@ static pseudo_t linearize_call_expressio
FOR_EACH_PTR(ctype->contexts, context) {
int in = context->in;
int out = context->out;
- int check = 0;
- int context_diff;
- if (in < 0) {
- check = 1;
- in = 0;
- }
- if (out < 0) {
- check = 0;
- out = 0;
- }
- context_diff = out - in;
- if (check || context_diff) {
+
+ if (out - in || context->out_false - in) {
insn = alloc_instruction(OP_CONTEXT, 0);
- insn->increment = context_diff;
- /* what's check for? */
- insn->check = check;
+ insn->increment = out - in;
insn->context_expr = context->context;
+ insn->inc_false = context->out_false - in;
add_one_insn(ep, insn);
}
} END_FOR_EACH_PTR(context);
@@ -1684,6 +1673,7 @@ static pseudo_t linearize_context(struct
value = expr->value;
insn->increment = value;
+ insn->inc_false = value;
expr = stmt->required;
value = 0;
--- sparse.orig/linearize.h 2008-04-10 15:21:21.000000000 +0200
+++ sparse/linearize.h 2008-04-10 15:22:43.000000000 +0200
@@ -116,8 +116,7 @@ struct instruction {
struct pseudo_list *arguments;
};
struct /* context */ {
- int increment, required;
- int check;
+ int increment, required, inc_false;
struct expression *context_expr;
};
struct /* asm */ {
--- sparse.orig/parse.c 2008-04-10 15:21:21.000000000 +0200
+++ sparse/parse.c 2008-04-10 15:22:43.000000000 +0200
@@ -66,6 +66,7 @@ static struct token *attribute_address_s
static struct token *attribute_aligned(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *attribute_mode(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *attribute_context(struct token *token, struct symbol *attr, struct ctype *ctype);
+static struct token *attribute_conditional_context(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *attribute_exact_context(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *attribute_transparent_union(struct token *token, struct symbol *attr, struct ctype *ctype);
static struct token *ignore_attribute(struct token *token, struct symbol *attr, struct ctype *ctype);
@@ -185,6 +186,10 @@ static struct symbol_op context_op = {
.attribute = attribute_context,
};
+static struct symbol_op conditional_context_op = {
+ .attribute = attribute_conditional_context,
+};
+
static struct symbol_op exact_context_op = {
.attribute = attribute_exact_context,
};
@@ -270,6 +275,7 @@ static struct init_keyword {
{ "address_space",NS_KEYWORD, .op = &address_space_op },
{ "mode", NS_KEYWORD, .op = &mode_op },
{ "context", NS_KEYWORD, .op = &context_op },
+ { "conditional_context", NS_KEYWORD, .op = &conditional_context_op },
{ "exact_context", NS_KEYWORD, .op = &exact_context_op },
{ "__transparent_union__", NS_KEYWORD, .op = &transparent_union_op },
@@ -912,6 +918,7 @@ static struct token *_attribute_context(
}
context->exact = exact;
+ context->out_false = context->out;
if (argc)
add_ptr_list(&ctype->contexts, context);
@@ -930,6 +937,51 @@ static struct token *attribute_exact_con
return _attribute_context(token, attr, ctype, 1);
}
+static struct token *attribute_conditional_context(struct token *token, struct symbol *attr, struct ctype *ctype)
+{
+ struct context *context = alloc_context();
+ struct expression *args[4];
+ int argc = 0;
+
+ token = expect(token, '(', "after conditional_context attribute");
+ while (!match_op(token, ')')) {
+ struct expression *expr = NULL;
+ token = conditional_expression(token, &expr);
+ if (!expr)
+ break;
+ if (argc < 4)
+ args[argc++] = expr;
+ else
+ argc++;
+ if (!match_op(token, ','))
+ break;
+ token = token->next;
+ }
+
+ switch(argc) {
+ case 3:
+ context->in = get_expression_value(args[0]);
+ context->out = get_expression_value(args[1]);
+ context->out_false = get_expression_value(args[2]);
+ break;
+ case 4:
+ context->context = args[0];
+ context->in = get_expression_value(args[1]);
+ context->out = get_expression_value(args[2]);
+ context->out_false = get_expression_value(args[3]);
+ break;
+ default:
+ sparse_error(token->pos, "invalid number of arguments to conditional_context attribute");
+ break;
+ }
+
+ if (argc)
+ add_ptr_list(&ctype->contexts, context);
+
+ token = expect(token, ')', "after conditional_context attribute");
+ return token;
+}
+
static struct token *attribute_transparent_union(struct token *token, struct symbol *attr, struct ctype *ctype)
{
if (Wtransparent_union)
--- sparse.orig/sparse.1 2008-04-10 15:21:21.000000000 +0200
+++ sparse/sparse.1 2008-04-10 15:22:43.000000000 +0200
@@ -94,6 +94,15 @@ There currently is no corresponding
.BI __exact_context__( [expression , ]adjust_value[ , required] )
statement.
+To indicate that a certain function acquires a context depending on its
+return value, use
+.BI __attribute__((conditional_context( [expression ,] in_context , out_success , out_failure ))
+where \fIout_success\fR and \fIout_failure\fR indicate the context change
+done depending on success (non-zero) or failure (zero) return of the
+function. Note that currently, using this attribute on a function means that
+the function itself won't be checked for context handling at all. See the
+testsuite for examples.
+
Sparse will warn when it sees a function change a
context without indicating this with a \fBcontext\fR or \fBexact_context\fR attribute, either by
decreasing a context below zero (such as by releasing a lock without acquiring
--
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 4/3] inlined call bugfix & test
2008-04-10 13:25 [PATCH 0/3] improve context handling Johannes Berg
` (2 preceding siblings ...)
2008-04-10 13:25 ` [PATCH 3/3] sparse: simple conditional context tracking Johannes Berg
@ 2008-04-11 11:07 ` Johannes Berg
2008-04-11 11:08 ` [PATCH 5/3] improve -Wcontext code and messages Johannes Berg
2008-04-21 18:37 ` [PATCH 0/3] improve context handling Josh Triplett
5 siblings, 0 replies; 26+ messages in thread
From: Johannes Berg @ 2008-04-11 11:07 UTC (permalink / raw)
To: Josh Triplett; +Cc: linux-sparse
This patch fixes an oversight in my other patches, inlined
calls weren't checked for context properly. Also adds a test
case for this.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
I'll roll this into the other patches if wanted.
sparse.c | 1 +
validation/context-named.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
--- sparse.orig/sparse.c 2008-04-11 12:42:49.000000000 +0200
+++ sparse/sparse.c 2008-04-11 12:43:44.000000000 +0200
@@ -137,6 +137,7 @@ static int check_bb_context(struct entry
if (!insn->bb)
continue;
switch (insn->opcode) {
+ case OP_INLINED_CALL:
case OP_CALL:
if (!insn->func || !insn->func->sym || insn->func->type != PSEUDO_SYM)
break;
--- sparse.orig/validation/context-named.c 2008-04-11 12:42:49.000000000 +0200
+++ sparse/validation/context-named.c 2008-04-11 12:46:36.000000000 +0200
@@ -465,6 +465,17 @@ static void warn_exact_fn2(void)
r2();
}
+static inline void need_lock3(void) __attribute__((context(TEST,1,1)))
+{
+}
+
+static void warn_fn3(void)
+{
+ a2();
+ need_lock3();
+ r2();
+}
+
#define __acquire(x) __context__(x,1)
#define __release(x) __context__(x,-1)
@@ -513,5 +524,6 @@ context-named.c:434:14: warning: context
context-named.c:441:15: warning: context problem in 'warn_fn2' - function 'need_lock2' expected different context
context-named.c:456:20: warning: context problem in 'warn_exact_fn1' - function 'need_lock_exact' expected different context
context-named.c:464:20: warning: context problem in 'warn_exact_fn2' - function 'need_lock_exact' expected different context
+context-named.c:475:15: warning: context problem in 'warn_fn3' - function 'need_lock3' expected different context
* check-error-end
*/
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 5/3] improve -Wcontext code and messages
2008-04-10 13:25 [PATCH 0/3] improve context handling Johannes Berg
` (3 preceding siblings ...)
2008-04-11 11:07 ` [PATCH 4/3] inlined call bugfix & test Johannes Berg
@ 2008-04-11 11:08 ` Johannes Berg
2008-04-21 18:37 ` [PATCH 0/3] improve context handling Josh Triplett
5 siblings, 0 replies; 26+ messages in thread
From: Johannes Berg @ 2008-04-11 11:08 UTC (permalink / raw)
To: Josh Triplett; +Cc: linux-sparse
This builds on my previous code improving the code and the messages,
the messages now always tell you the expected and actual context
value. Also add another test since I had mentioned that case.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
I can also roll this into the other patches if wanted.
sparse.c | 234 +++++++++++++++++++++++++++--------------
validation/context-dynamic.c | 14 ++
validation/context-named.c | 72 ++++++++----
validation/context-statement.c | 17 ++
validation/context.c | 90 +++++++++++++--
5 files changed, 307 insertions(+), 120 deletions(-)
--- sparse.orig/sparse.c 2008-04-11 12:43:44.000000000 +0200
+++ sparse/sparse.c 2008-04-11 12:54:45.000000000 +0200
@@ -42,7 +42,8 @@ static const char *context_name(struct c
return unnamed_context;
}
-static void context_add(struct context_check_list **ccl, const char *name, int offs, int offs_false)
+static void context_add(struct context_check_list **ccl, const char *name,
+ int offs, int offs_false)
{
struct context_check *check, *found = NULL;
@@ -63,16 +64,19 @@ static void context_add(struct context_c
found->val_false += offs_false;
}
-static int imbalance(struct entrypoint *ep, struct position pos, const char *name, const char *why)
+#define IMBALANCE_IN "context imbalance in '%s': "
+#define DEFAULT_CONTEXT_DESCR " default context: "
+
+static void get_context_string(char **buf, const char **name)
{
- if (Wcontext) {
- struct symbol *sym = ep->name;
- if (strcmp(name, unnamed_context))
- warning(pos, "context imbalance in '%s' - %s (%s)", show_ident(sym->ident), why, name);
- else
- warning(pos, "context imbalance in '%s' - %s", show_ident(sym->ident), why);
+ if (strcmp(*name, unnamed_context)) {
+ *buf = malloc(strlen(*name) + 16);
+ sprintf(*buf, " context '%s': ", *name);
+ *name = *buf;
+ } else {
+ *name = DEFAULT_CONTEXT_DESCR;
+ *buf = NULL;
}
- return -1;
}
static int context_list_check(struct entrypoint *ep, struct position pos,
@@ -81,6 +85,8 @@ static int context_list_check(struct ent
{
struct context_check *c1, *c2;
int cur, tgt;
+ const char *name;
+ char *buf;
/* make sure the loop below checks all */
FOR_EACH_PTR(ccl_target, c1) {
@@ -98,15 +104,131 @@ static int context_list_check(struct ent
break;
} END_FOR_EACH_PTR(c2);
+ if (cur == tgt || !Wcontext)
+ continue;
+
if (cur > tgt)
- return imbalance(ep, pos, c1->name, "wrong count at exit");
+ warning(pos, IMBALANCE_IN "wrong count at exit",
+ show_ident(ep->name->ident));
else if (cur < tgt)
- return imbalance(ep, pos, c1->name, "unexpected unlock");
+ warning(pos, IMBALANCE_IN "unexpected unlock",
+ show_ident(ep->name->ident));
+
+ name = c1->name;
+ get_context_string(&buf, &name);
+
+ info(pos, "%swanted %d, got %d",
+ name, tgt, cur);
+
+ free(buf);
+
+ return -1;
} END_FOR_EACH_PTR(c1);
return 0;
}
+static int handle_call(struct entrypoint *ep, struct basic_block *bb,
+ struct instruction *insn,
+ struct context_check_list *combined)
+{
+ struct context *ctx;
+ struct context_check *c;
+ const char *name, *call, *cmp;
+ char *buf;
+ int val, ok;
+
+ if (!insn->func || !insn->func->sym ||
+ insn->func->type != PSEUDO_SYM)
+ return 0;
+
+ /*
+ * Check all contexts the function wants.
+ */
+ FOR_EACH_PTR(insn->func->sym->ctype.contexts, ctx) {
+ name = context_name(ctx);
+ val = 0;
+
+ FOR_EACH_PTR(combined, c) {
+ if (strcmp(c->name, name) == 0) {
+ val = c->val;
+ break;
+ }
+ } END_FOR_EACH_PTR(c);
+
+ if (ctx->exact) {
+ ok = ctx->in == val;
+ cmp = "";
+ } else {
+ ok = ctx->in <= val;
+ cmp = ">= ";
+ }
+
+ if (!ok && Wcontext) {
+ get_context_string(&buf, &name);
+ call = strdup(show_ident(insn->func->ident));
+
+ warning(insn->pos, "context problem in '%s': "
+ "'%s' expected different context",
+ show_ident(ep->name->ident), call);
+
+ info(insn->pos, "%swanted %s%d, got %d",
+ name, cmp, ctx->in, val);
+
+ free((void *)call);
+ free(buf);
+
+ return -1;
+ }
+ } END_FOR_EACH_PTR (ctx);
+
+ return 0;
+}
+
+static int handle_context(struct entrypoint *ep, struct basic_block *bb,
+ struct instruction *insn,
+ struct context_check_list **combined)
+{
+ struct context_check *c;
+ const char *name;
+ char *buf;
+ int val, ok;
+
+ val = 0;
+
+ name = unnamed_context;
+ if (insn->context_expr)
+ name = show_ident(insn->context_expr->symbol_name);
+
+ FOR_EACH_PTR(*combined, c) {
+ if (strcmp(c->name, name) == 0) {
+ val = c->val;
+ break;
+ }
+ } END_FOR_EACH_PTR(c);
+
+ ok = insn->required <= val;
+
+ if (!ok && Wcontext) {
+ get_context_string(&buf, &name);
+
+ warning(insn->pos,
+ IMBALANCE_IN
+ "__context__ statement expected different context",
+ show_ident(ep->name->ident));
+
+ info(insn->pos, "%swanted >= %d, got %d",
+ name, insn->required, val);
+
+ free(buf);
+ return -1;
+ }
+
+ context_add(combined, name, insn->increment, insn->inc_false);
+
+ return 0;
+}
+
static int check_bb_context(struct entrypoint *ep, struct basic_block *bb,
struct context_check_list *ccl_in,
struct context_check_list *ccl_target,
@@ -116,16 +238,21 @@ static int check_bb_context(struct entry
struct context_check *c;
struct instruction *insn;
struct multijmp *mj;
- struct context *ctx;
- const char *name;
- int ok, val;
- /* recurse in once to catch bad loops */
+ /*
+ * recurse in once to catch bad loops,
+ * bb->context is initialised to -1.
+ */
if (bb->context > 0)
return 0;
-
bb->context++;
+ /*
+ * We're starting with a completely new local list of contexts, so
+ * initialise it according to what we got from the parent block.
+ * That may use either the 'false' or the 'true' part of the context
+ * for the conditional_context() attribute.
+ */
FOR_EACH_PTR(ccl_in, c) {
if (in_false)
context_add(&combined, c->name, c->val_false, c->val_false);
@@ -133,76 +260,37 @@ static int check_bb_context(struct entry
context_add(&combined, c->name, c->val, c->val);
} END_FOR_EACH_PTR(c);
+ /*
+ * Now walk the instructions for this block, recursing into any
+ * instructions that have children. We need to have the right
+ * order so we cannot iterate bb->children instead.
+ */
FOR_EACH_PTR(bb->insns, insn) {
- if (!insn->bb)
- continue;
switch (insn->opcode) {
case OP_INLINED_CALL:
case OP_CALL:
- if (!insn->func || !insn->func->sym || insn->func->type != PSEUDO_SYM)
- break;
- FOR_EACH_PTR(insn->func->sym->ctype.contexts, ctx) {
- name = context_name(ctx);
- val = 0;
-
- FOR_EACH_PTR(combined, c) {
- if (strcmp(c->name, name) == 0) {
- val = c->val;
- break;
- }
- } END_FOR_EACH_PTR(c);
-
- if (ctx->exact)
- ok = ctx->in == val;
- else
- ok = ctx->in <= val;
-
- if (!ok) {
- const char *call = strdup(show_ident(insn->func->ident));
- warning(insn->pos, "context problem in '%s' - function '%s' expected different context",
- show_ident(ep->name->ident), call);
- free((void *)call);
- return -1;
- }
- } END_FOR_EACH_PTR (ctx);
+ if (handle_call(ep, bb, insn, combined))
+ return -1;
break;
case OP_CONTEXT:
- val = 0;
-
- name = unnamed_context;
- if (insn->context_expr)
- name = show_ident(insn->context_expr->symbol_name);
-
- FOR_EACH_PTR(combined, c) {
- if (strcmp(c->name, name) == 0) {
- val = c->val;
- break;
- }
- } END_FOR_EACH_PTR(c);
-
- ok = insn->required <= val;
-
- if (!ok) {
- name = strdup(name);
- imbalance(ep, insn->pos, name, "__context__ statement expected different lock context");
- free((void *)name);
+ if (handle_context(ep, bb, insn, &combined))
return -1;
- }
-
- context_add(&combined, name, insn->increment, insn->inc_false);
break;
case OP_BR:
if (insn->bb_true)
- if (check_bb_context(ep, insn->bb_true, combined, ccl_target, 0))
+ if (check_bb_context(ep, insn->bb_true,
+ combined, ccl_target, 0))
return -1;
if (insn->bb_false)
- if (check_bb_context(ep, insn->bb_false, combined, ccl_target, 1))
+ if (check_bb_context(ep, insn->bb_false,
+ combined, ccl_target, 1))
return -1;
break;
case OP_SWITCH:
case OP_COMPUTEDGOTO:
FOR_EACH_PTR(insn->multijmp_list, mj) {
- if (check_bb_context(ep, mj->target, combined, ccl_target, 0))
+ if (check_bb_context(ep, mj->target,
+ combined, ccl_target, 0))
return -1;
} END_FOR_EACH_PTR(mj);
break;
@@ -212,16 +300,10 @@ static int check_bb_context(struct entry
insn = last_instruction(bb->insns);
if (!insn)
return 0;
+
if (insn->opcode == OP_RET)
return context_list_check(ep, insn->pos, combined, ccl_target);
- FOR_EACH_PTR(bb->insns, insn) {
- if (!insn->bb)
- continue;
- switch (insn->opcode) {
- }
- } END_FOR_EACH_PTR(insn);
-
/* contents will be freed once we return out of recursion */
free_ptr_list(&combined);
--- sparse.orig/validation/context-dynamic.c 2008-04-11 12:43:44.000000000 +0200
+++ sparse/validation/context-dynamic.c 2008-04-11 12:47:44.000000000 +0200
@@ -151,11 +151,21 @@ static int good_switch(void)
r();
}
+static void bad_lock1(void)
+{
+ r();
+ a();
+}
+
/*
* check-name: Check -Wcontext with lock trylocks
*
* check-error-start
-context-dynamic.c:83:6: warning: context problem in 'bad_trylock1' - function 'r' expected different context
-context-dynamic.c:133:6: warning: context problem in 'bad_trylock2' - function 'r' expected different context
+context-dynamic.c:83:6: warning: context problem in 'bad_trylock1': 'r' expected different context
+context-dynamic.c:83:6: context 'A': wanted >= 1, got 0
+context-dynamic.c:133:6: warning: context problem in 'bad_trylock2': 'r' expected different context
+context-dynamic.c:133:6: context 'A': wanted >= 1, got 0
+context-dynamic.c:156:6: warning: context problem in 'bad_lock1': 'r' expected different context
+context-dynamic.c:156:6: context 'A': wanted >= 1, got 0
* check-error-end
*/
--- sparse.orig/validation/context-named.c 2008-04-11 12:46:36.000000000 +0200
+++ sparse/validation/context-named.c 2008-04-11 12:54:45.000000000 +0200
@@ -501,29 +501,53 @@ static void good_mixed_with_if(void)
* check-name: Check -Wcontext with lock names
*
* check-error-start
-context-named.c:86:3: warning: context imbalance in 'warn_lock1' - wrong count at exit (TEST)
-context-named.c:93:3: warning: context imbalance in 'warn_lock2' - wrong count at exit (TEST)
-context-named.c:100:3: warning: context imbalance in 'warn_lock3' - wrong count at exit (TEST)
-context-named.c:105:3: warning: context problem in 'warn_unlock1' - function 'r' expected different context
-context-named.c:112:3: warning: context problem in 'warn_unlock2' - function 'r' expected different context
-context-named.c:152:9: warning: context imbalance in 'warn_if1' - wrong count at exit (TEST)
-context-named.c:162:9: warning: context imbalance in 'warn_if2' - wrong count at exit (TEST)
-context-named.c:218:4: warning: context imbalance in 'warn_while1' - wrong count at exit (TEST)
-context-named.c:225:4: warning: context problem in 'warn_while2' - function 'r' expected different context
-context-named.c:235:4: warning: context imbalance in 'warn_while3' - wrong count at exit (TEST)
-context-named.c:295:5: warning: context imbalance in 'warn_goto1' - wrong count at exit (TEST)
-context-named.c:305:6: warning: context imbalance in 'warn_goto2' - wrong count at exit (TEST)
-context-named.c:315:6: warning: context problem in 'warn_goto3' - function 'r' expected different context
-context-named.c:321:7: warning: context imbalance in 'warn_multiple1' - wrong count at exit (TEST)
-context-named.c:327:6: warning: context imbalance in 'warn_multiple2' - wrong count at exit (TEST2)
-context-named.c:333:6: warning: context problem in 'warn_mixed1' - function 'r' expected different context
-context-named.c:343:6: warning: context problem in 'warn_mixed2' - function 'r' expected different context
-context-named.c:353:6: warning: context problem in 'warn_mixed3' - function 'r' expected different context
-context-named.c:364:6: warning: context imbalance in 'warn_mixed4' - wrong count at exit (TEST2)
-context-named.c:434:14: warning: context problem in 'warn_fn' - function 'need_lock' expected different context
-context-named.c:441:15: warning: context problem in 'warn_fn2' - function 'need_lock2' expected different context
-context-named.c:456:20: warning: context problem in 'warn_exact_fn1' - function 'need_lock_exact' expected different context
-context-named.c:464:20: warning: context problem in 'warn_exact_fn2' - function 'need_lock_exact' expected different context
-context-named.c:475:15: warning: context problem in 'warn_fn3' - function 'need_lock3' expected different context
+context-named.c:86:3: warning: context imbalance in 'warn_lock1': wrong count at exit
+context-named.c:86:3: context 'TEST': wanted 0, got 1
+context-named.c:93:3: warning: context imbalance in 'warn_lock2': wrong count at exit
+context-named.c:93:3: context 'TEST': wanted 0, got 1
+context-named.c:100:3: warning: context imbalance in 'warn_lock3': wrong count at exit
+context-named.c:100:3: context 'TEST': wanted 0, got 1
+context-named.c:105:3: warning: context problem in 'warn_unlock1': 'r' expected different context
+context-named.c:105:3: context 'TEST': wanted >= 1, got 0
+context-named.c:112:3: warning: context problem in 'warn_unlock2': 'r' expected different context
+context-named.c:112:3: context 'TEST': wanted >= 1, got 0
+context-named.c:152:9: warning: context imbalance in 'warn_if1': wrong count at exit
+context-named.c:152:9: context 'TEST': wanted 0, got 1
+context-named.c:162:9: warning: context imbalance in 'warn_if2': wrong count at exit
+context-named.c:162:9: context 'TEST': wanted 0, got 1
+context-named.c:218:4: warning: context imbalance in 'warn_while1': wrong count at exit
+context-named.c:218:4: context 'TEST': wanted 0, got 1
+context-named.c:225:4: warning: context problem in 'warn_while2': 'r' expected different context
+context-named.c:225:4: context 'TEST': wanted >= 1, got 0
+context-named.c:235:4: warning: context imbalance in 'warn_while3': wrong count at exit
+context-named.c:235:4: context 'TEST': wanted 0, got 1
+context-named.c:295:5: warning: context imbalance in 'warn_goto1': wrong count at exit
+context-named.c:295:5: context 'TEST': wanted 0, got 1
+context-named.c:305:6: warning: context imbalance in 'warn_goto2': wrong count at exit
+context-named.c:305:6: context 'TEST': wanted 0, got 1
+context-named.c:315:6: warning: context problem in 'warn_goto3': 'r' expected different context
+context-named.c:315:6: context 'TEST': wanted >= 1, got 0
+context-named.c:321:7: warning: context imbalance in 'warn_multiple1': wrong count at exit
+context-named.c:321:7: context 'TEST': wanted 0, got 1
+context-named.c:327:6: warning: context imbalance in 'warn_multiple2': wrong count at exit
+context-named.c:327:6: context 'TEST2': wanted 0, got 1
+context-named.c:333:6: warning: context problem in 'warn_mixed1': 'r' expected different context
+context-named.c:333:6: context 'TEST': wanted >= 1, got 0
+context-named.c:343:6: warning: context problem in 'warn_mixed2': 'r' expected different context
+context-named.c:343:6: context 'TEST': wanted >= 1, got 0
+context-named.c:353:6: warning: context problem in 'warn_mixed3': 'r' expected different context
+context-named.c:353:6: context 'TEST': wanted >= 1, got 0
+context-named.c:364:6: warning: context imbalance in 'warn_mixed4': wrong count at exit
+context-named.c:364:6: context 'TEST2': wanted 0, got 1
+context-named.c:434:14: warning: context problem in 'warn_fn': 'need_lock' expected different context
+context-named.c:434:14: context 'TEST': wanted >= 1, got 0
+context-named.c:441:15: warning: context problem in 'warn_fn2': 'need_lock2' expected different context
+context-named.c:441:15: context 'TEST': wanted >= 1, got 0
+context-named.c:456:20: warning: context problem in 'warn_exact_fn1': 'need_lock_exact' expected different context
+context-named.c:456:20: context 'TEST': wanted 1, got 2
+context-named.c:464:20: warning: context problem in 'warn_exact_fn2': 'need_lock_exact' expected different context
+context-named.c:464:20: context 'TEST': wanted 1, got 0
+context-named.c:475:15: warning: context problem in 'warn_fn3': 'need_lock3' expected different context
+context-named.c:475:15: context 'TEST': wanted >= 1, got 0
* check-error-end
*/
--- sparse.orig/validation/context-statement.c 2008-04-11 12:43:44.000000000 +0200
+++ sparse/validation/context-statement.c 2008-04-11 12:47:44.000000000 +0200
@@ -47,12 +47,23 @@ static void bad_macro2(void)
m();
}
+static void bad_macro3(void)
+{
+ r();
+ a();
+}
+
/*
* check-name: Check __context__ statement with required context
*
* check-error-start
-context-statement.c:16:8: warning: context imbalance in 'bad_arr' - unexpected unlock (LOCK)
-context-statement.c:38:5: warning: context imbalance in 'bad_macro1' - __context__ statement expected different lock context (LOCK)
-context-statement.c:47:5: warning: context imbalance in 'bad_macro2' - __context__ statement expected different lock context (LOCK)
+context-statement.c:16:8: warning: context imbalance in 'bad_arr': unexpected unlock
+context-statement.c:16:8: context 'LOCK': wanted 0, got -1
+context-statement.c:38:5: warning: context imbalance in 'bad_macro1': __context__ statement expected different context
+context-statement.c:38:5: context 'LOCK': wanted >= 1, got 0
+context-statement.c:47:5: warning: context imbalance in 'bad_macro2': __context__ statement expected different context
+context-statement.c:47:5: context 'LOCK': wanted >= 1, got 0
+context-statement.c:53:5: warning: context imbalance in 'bad_macro3': __context__ statement expected different context
+context-statement.c:53:5: context 'LOCK': wanted >= 0, got -1
* check-error-end
*/
--- sparse.orig/validation/context.c 2008-04-11 12:43:44.000000000 +0200
+++ sparse/validation/context.c 2008-04-11 12:47:44.000000000 +0200
@@ -325,24 +325,84 @@ static void warn_odd_looping(void)
r();
}
+static void warn_huge_switch(void)
+{
+ a();
+
+ switch(condition) {
+ case 1:
+ r();
+ break;
+ case 2:
+ r();
+ break;
+ case 3:
+ r();
+ break;
+ case 4:
+ r();
+ break;
+ case 5:
+ r();
+ break;
+ case 11:
+ r();
+ break;
+ case 12:
+ r();
+ break;
+ case 13:
+ r();
+ break;
+ case 14:
+ r();
+ case 15:
+ r();
+ break;
+ case 16:
+ r();
+ break;
+ case 17:
+ r();
+ break;
+ }
+}
+
/*
* check-name: Check -Wcontext
*
* check-error-start
-context.c:71:3: warning: context imbalance in 'warn_lock1' - wrong count at exit
-context.c:78:3: warning: context imbalance in 'warn_lock2' - wrong count at exit
-context.c:85:3: warning: context imbalance in 'warn_lock3' - wrong count at exit
-context.c:90:3: warning: context problem in 'warn_unlock1' - function 'r' expected different context
-context.c:97:3: warning: context problem in 'warn_unlock2' - function 'r' expected different context
-context.c:137:9: warning: context imbalance in 'warn_if1' - wrong count at exit
-context.c:147:9: warning: context imbalance in 'warn_if2' - wrong count at exit
-context.c:203:4: warning: context imbalance in 'warn_while1' - wrong count at exit
-context.c:210:4: warning: context problem in 'warn_while2' - function 'r' expected different context
-context.c:220:4: warning: context imbalance in 'warn_while3' - wrong count at exit
-context.c:280:5: warning: context imbalance in 'warn_goto1' - wrong count at exit
-context.c:290:6: warning: context imbalance in 'warn_goto2' - wrong count at exit
-context.c:300:6: warning: context problem in 'warn_goto3' - function 'r' expected different context
-context.c:315:6: warning: context problem in 'warn_cond_lock1' - function 'r' expected different context
-context.c:325:10: warning: context problem in 'warn_odd_looping' - function 'r' expected different context
+context.c:71:3: warning: context imbalance in 'warn_lock1': wrong count at exit
+context.c:71:3: default context: wanted 0, got 1
+context.c:78:3: warning: context imbalance in 'warn_lock2': wrong count at exit
+context.c:78:3: default context: wanted 0, got 1
+context.c:85:3: warning: context imbalance in 'warn_lock3': wrong count at exit
+context.c:85:3: default context: wanted 0, got 1
+context.c:90:3: warning: context problem in 'warn_unlock1': 'r' expected different context
+context.c:90:3: default context: wanted >= 1, got 0
+context.c:97:3: warning: context problem in 'warn_unlock2': 'r' expected different context
+context.c:97:3: default context: wanted >= 1, got 0
+context.c:137:9: warning: context imbalance in 'warn_if1': wrong count at exit
+context.c:137:9: default context: wanted 0, got 1
+context.c:147:9: warning: context imbalance in 'warn_if2': wrong count at exit
+context.c:147:9: default context: wanted 0, got 1
+context.c:203:4: warning: context imbalance in 'warn_while1': wrong count at exit
+context.c:203:4: default context: wanted 0, got 1
+context.c:210:4: warning: context problem in 'warn_while2': 'r' expected different context
+context.c:210:4: default context: wanted >= 1, got 0
+context.c:220:4: warning: context imbalance in 'warn_while3': wrong count at exit
+context.c:220:4: default context: wanted 0, got 1
+context.c:280:5: warning: context imbalance in 'warn_goto1': wrong count at exit
+context.c:280:5: default context: wanted 0, got 1
+context.c:290:6: warning: context imbalance in 'warn_goto2': wrong count at exit
+context.c:290:6: default context: wanted 0, got 1
+context.c:300:6: warning: context problem in 'warn_goto3': 'r' expected different context
+context.c:300:6: default context: wanted >= 1, got 0
+context.c:315:6: warning: context problem in 'warn_cond_lock1': 'r' expected different context
+context.c:315:6: default context: wanted >= 1, got 0
+context.c:325:10: warning: context problem in 'warn_odd_looping': 'r' expected different context
+context.c:325:10: default context: wanted >= 1, got 0
+context.c:360:10: warning: context problem in 'warn_huge_switch': 'r' expected different context
+context.c:360:10: default context: wanted >= 1, got 0
* check-error-end
*/
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 0/3] improve context handling
2008-04-10 13:25 [PATCH 0/3] improve context handling Johannes Berg
` (4 preceding siblings ...)
2008-04-11 11:08 ` [PATCH 5/3] improve -Wcontext code and messages Johannes Berg
@ 2008-04-21 18:37 ` Josh Triplett
5 siblings, 0 replies; 26+ messages in thread
From: Josh Triplett @ 2008-04-21 18:37 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-sparse
[-- Attachment #1: Type: text/plain, Size: 177 bytes --]
Johannes Berg wrote:
> Here are three patches to improve context tracking in sparse.
All five patches applied and pushed. Thanks for your great work!
- Josh Triplett
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread