barrier macro

barrier macro

[Randy Dunlap]

(using sparse 0.3)

./compiler-gcc.h:10:#define barrier() __asm__ __volatile__("": : :"memory")

causes this output:

net/sunrpc/xprtsock.c:640:2: error: Expected ( after asm
net/sunrpc/xprtsock.c:640:2: error: got __volatile__
net/sunrpc/xprtsock.c:640:2: error: typename in expression
net/sunrpc/xprtsock.c:640:2: error: Expected ) in function call
net/sunrpc/xprtsock.c:640:2: error: got :


Maybe sparse could allow modifiers between asm|__asm__ and the
(...) ?

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: barrier macro

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

Randy Dunlap wrote:
> (using sparse 0.3)
> 
> ./compiler-gcc.h:10:#define barrier() __asm__ __volatile__("": : :"memory")
> 
> causes this output:
> 
> net/sunrpc/xprtsock.c:640:2: error: Expected ( after asm
> net/sunrpc/xprtsock.c:640:2: error: got __volatile__
> net/sunrpc/xprtsock.c:640:2: error: typename in expression
> net/sunrpc/xprtsock.c:640:2: error: Expected ) in function call
> net/sunrpc/xprtsock.c:640:2: error: got :
> 
> 
> Maybe sparse could allow modifiers between asm|__asm__ and the
> (...) ?

Sparse specifically allows volatile, and double-underscore variants, between the asm keyword and the open parenthesis:

static struct token *parse_asm_statement(struct token *token, struct statement *stmt)
{
        token = token->next;
        stmt->type = STMT_ASM;
        if (match_idents(token, &__volatile___ident, &__volatile_ident, &volatile_ident, NULL)) {
                token = token->next;
        }
        token = expect(token, '(', "after asm");
[...]


I cannot reproduce your report with the following test case (just added to git
as validation/asm-volatile.c):

#define barrier() __asm__ __volatile__("": : :"memory")

static void f(void)
{
        barrier();
}


Perhaps something else has caused the problem.  Could you please generate a
preprocessed file with "make net/sunrpc/xprtsock.i", and strip it down to a
minimal test case that still generates the Sparse warning?

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: barrier macro

[Randy Dunlap]

On Tue, 01 May 2007 20:17:53 -0700 Josh Triplett wrote:

> Randy Dunlap wrote:
> > (using sparse 0.3)
> > 
> > ./compiler-gcc.h:10:#define barrier() __asm__ __volatile__("": : :"memory")
> > 
> > causes this output:
> > 
> > net/sunrpc/xprtsock.c:640:2: error: Expected ( after asm
> > net/sunrpc/xprtsock.c:640:2: error: got __volatile__
> > net/sunrpc/xprtsock.c:640:2: error: typename in expression
> > net/sunrpc/xprtsock.c:640:2: error: Expected ) in function call
> > net/sunrpc/xprtsock.c:640:2: error: got :
> > 
> > 
> > Maybe sparse could allow modifiers between asm|__asm__ and the
> > (...) ?
> 
> Sparse specifically allows volatile, and double-underscore variants, between the asm keyword and the open parenthesis:
> 
> static struct token *parse_asm_statement(struct token *token, struct statement *stmt)
> {
>         token = token->next;
>         stmt->type = STMT_ASM;
>         if (match_idents(token, &__volatile___ident, &__volatile_ident, &volatile_ident, NULL)) {
>                 token = token->next;
>         }
>         token = expect(token, '(', "after asm");
> [...]
> 
> 
> I cannot reproduce your report with the following test case (just added to git
> as validation/asm-volatile.c):

I'm doing this on i386 (x86_32).  Maybe that would help you.
It's trivial to reproduce.


> #define barrier() __asm__ __volatile__("": : :"memory")
> 
> static void f(void)
> {
>         barrier();
> }
> 
> 
> Perhaps something else has caused the problem.  Could you please generate a
> preprocessed file with "make net/sunrpc/xprtsock.i", and strip it down to a
> minimal test case that still generates the Sparse warning?

Sure, I'll trim the 35000 lines down to a test case and get back
to you.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: barrier macro

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 2008 bytes --]

Randy Dunlap wrote:
> On Tue, 01 May 2007 20:17:53 -0700 Josh Triplett wrote:
> 
>> Randy Dunlap wrote:
>>> (using sparse 0.3)
>>>
>>> ./compiler-gcc.h:10:#define barrier() __asm__ __volatile__("": : :"memory")
>>>
>>> causes this output:
>>>
>>> net/sunrpc/xprtsock.c:640:2: error: Expected ( after asm
>>> net/sunrpc/xprtsock.c:640:2: error: got __volatile__
>>> net/sunrpc/xprtsock.c:640:2: error: typename in expression
>>> net/sunrpc/xprtsock.c:640:2: error: Expected ) in function call
>>> net/sunrpc/xprtsock.c:640:2: error: got :
>>>
>>>
>>> Maybe sparse could allow modifiers between asm|__asm__ and the
>>> (...) ?
>> Sparse specifically allows volatile, and double-underscore variants, between the asm keyword and the open parenthesis:
>>
>> static struct token *parse_asm_statement(struct token *token, struct statement *stmt)
>> {
>>         token = token->next;
>>         stmt->type = STMT_ASM;
>>         if (match_idents(token, &__volatile___ident, &__volatile_ident, &volatile_ident, NULL)) {
>>                 token = token->next;
>>         }
>>         token = expect(token, '(', "after asm");
>> [...]
>>
>>
>> I cannot reproduce your report with the following test case (just added to git
>> as validation/asm-volatile.c):
> 
> I'm doing this on i386 (x86_32).  Maybe that would help you.

Same here.

> It's trivial to reproduce.

I've managed to reproduce it using a current Linux tree.  Let me look into it.

>> #define barrier() __asm__ __volatile__("": : :"memory")
>>
>> static void f(void)
>> {
>>         barrier();
>> }
>>
>>
>> Perhaps something else has caused the problem.  Could you please generate a
>> preprocessed file with "make net/sunrpc/xprtsock.i", and strip it down to a
>> minimal test case that still generates the Sparse warning?
> 
> Sure, I'll trim the 35000 lines down to a test case and get back
> to you.

Ouch.  Nevermind, now that I've reproduced it I'll take care of it.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: barrier macro

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

After analyzing net/sunrpc/xprtsock.i, I managed to reproduce the problem with
the following test case:

#define barrier() __asm__ __volatile__("": : :"memory")

static void f(void)
{
	barrier();
l:
	barrier();
}


Apparently sparse doesn't like __asm__ __volatile__ after a label.  Looks like
the change to enable attributes on labels makes Sparse interpret the __asm__
as an attribute on the label, not as a statement.  If I locally revert the
label attributes change, aec53c938c34c47cdbdd6824552e0f2a5104b1cb, this test
case compiles without warning, as does net/sunrpc/xprtsock.c.

The label attributes change needs some additional work, to make it only handle
attribute and __attribute__, and nothing else.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: barrier macro

[Randy Dunlap]

On Tue, 01 May 2007 21:24:09 -0700 Josh Triplett wrote:

> After analyzing net/sunrpc/xprtsock.i, I managed to reproduce the problem with
> the following test case:
> 
> #define barrier() __asm__ __volatile__("": : :"memory")
> 
> static void f(void)
> {
> 	barrier();
> l:
> 	barrier();
> }
> 
> 
> Apparently sparse doesn't like __asm__ __volatile__ after a label.  Looks like
> the change to enable attributes on labels makes Sparse interpret the __asm__
> as an attribute on the label, not as a statement.  If I locally revert the
> label attributes change, aec53c938c34c47cdbdd6824552e0f2a5104b1cb, this test
> case compiles without warning, as does net/sunrpc/xprtsock.c.
> 
> The label attributes change needs some additional work, to make it only handle
> attribute and __attribute__, and nothing else.

OK, thanks for the analysis (and not making me cut down 35000 lines :).

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: barrier macro

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

Josh Triplett wrote:
> After analyzing net/sunrpc/xprtsock.i, I managed to reproduce the problem with
> the following test case:
> 
> #define barrier() __asm__ __volatile__("": : :"memory")
> 
> static void f(void)
> {
> 	barrier();
> l:
> 	barrier();
> }
> 
> 
> Apparently sparse doesn't like __asm__ __volatile__ after a label.  Looks like
> the change to enable attributes on labels makes Sparse interpret the __asm__
> as an attribute on the label, not as a statement.  If I locally revert the
> label attributes change, aec53c938c34c47cdbdd6824552e0f2a5104b1cb, this test
> case compiles without warning, as does net/sunrpc/xprtsock.c.
> 
> The label attributes change needs some additional work, to make it only handle
> attribute and __attribute__, and nothing else.

Fixed in current Git.

- Josh Triplett



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: barrier macro

[Christopher Li]

On Tue, May 01, 2007 at 10:37:11PM -0700, Josh Triplett wrote:
> 
> Fixed in current Git.
> 

May I purpose a slightly different fix?

The reason I use bit mask in the keyword so that it can allow
select a sub set of keywords. If we want, We can fine tune exactly
what keyword is allowed. It also makes the caller of handle_attributes
show exactly what kind of attribute it takes.

Signed-Off-By: Christopher Li <sparse@chrisli.org>

Index: sparse/parse.c
===================================================================
--- sparse.orig/parse.c	2007-05-02 00:41:30.000000000 -0700
+++ sparse/parse.c	2007-05-02 00:53:09.000000000 -0700
@@ -35,7 +35,7 @@ struct symbol_list *function_computed_ta
 struct statement_list *function_computed_goto_list;
 
 static struct token *statement(struct token *token, struct statement **tree);
-static struct token *handle_attributes(struct token *token, struct ctype *ctype, int allow_asm);
+static struct token *handle_attributes(struct token *token, struct ctype *ctype, enum keyword keywords);
 
 static struct token *struct_specifier(struct token *token, struct ctype *ctype);
 static struct token *union_specifier(struct token *token, struct ctype *ctype);
@@ -458,7 +458,7 @@ static struct token *struct_union_enum_s
 	struct position *repos;
 
 	ctype->modifiers = 0;
-	token = handle_attributes(token, ctype, 1);
+	token = handle_attributes(token, ctype, KW_ATTRIBUTE | KW_ASM);
 	if (token_type(token) == TOKEN_IDENT) {
 		sym = lookup_symbol(token->ident, NS_STRUCT);
 		if (!sym ||
@@ -1094,7 +1094,7 @@ static struct token *abstract_array_decl
 static struct token *parameter_type_list(struct token *, struct symbol *, struct ident **p);
 static struct token *declarator(struct token *token, struct symbol *sym, struct ident **p);
 
-static struct token *handle_attributes(struct token *token, struct ctype *ctype, int allow_asm)
+static struct token *handle_attributes(struct token *token, struct ctype *ctype, enum keyword keywords)
 {
 	struct symbol *keyword;
 	for (;;) {
@@ -1104,7 +1104,7 @@ static struct token *handle_attributes(s
 		keyword = lookup_keyword(token->ident, NS_KEYWORD | NS_TYPEDEF);
 		if (!keyword || keyword->type != SYM_KEYWORD)
 			break;
-		if (!(keyword->op->type & (KW_ATTRIBUTE | (allow_asm ? KW_ASM : 0))))
+		if (!(keyword->op->type & keywords))
 			break;
 		token = keyword->op->declarator(token->next, &thistype);
 		apply_ctype(token->pos, &thistype, ctype);
@@ -1122,7 +1122,7 @@ static struct token *direct_declarator(s
 	}
 
 	for (;;) {
-		token = handle_attributes(token, ctype, 1);
+		token = handle_attributes(token, ctype, KW_ATTRIBUTE | KW_ASM);
 
 		if (token_type(token) != TOKEN_SPECIAL)
 			return token;
@@ -1261,7 +1261,7 @@ static struct token *declaration_list(st
 		decl->ident = ident;
 		if (match_op(token, ':')) {
 			token = handle_bitfield(token, decl);
-			token = handle_attributes(token, &decl->ctype, 1);
+			token = handle_attributes(token, &decl->ctype, KW_ATTRIBUTE | KW_ASM);
 		}
 		apply_modifiers(token->pos, &decl->ctype);
 		add_symbol(list, decl);
@@ -1702,7 +1702,7 @@ static struct token *statement(struct to
 		if (match_op(token->next, ':')) {
 			stmt->type = STMT_LABEL;
 			stmt->label_identifier = label_symbol(token);
-			token = handle_attributes(token->next->next, &stmt->label_identifier->ctype, 0);
+			token = handle_attributes(token->next->next, &stmt->label_identifier->ctype, KW_ATTRIBUTE);
 			return statement(token, &stmt->label_statement);
 		}
 	}

Re: barrier macro

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

Christopher Li wrote:
> On Tue, May 01, 2007 at 10:37:11PM -0700, Josh Triplett wrote:
>> Fixed in current Git.
> 
> May I purpose a slightly different fix?
> 
> The reason I use bit mask in the keyword so that it can allow
> select a sub set of keywords. If we want, We can fine tune exactly
> what keyword is allowed. It also makes the caller of handle_attributes
> show exactly what kind of attribute it takes.

Applied, with a minor fix: a bitmask of values from "enum keyword" does not
have type "enum keyword", so I made the argument an unsigned int.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]