Re: [PATCH 1/1] parser: add support for asm goto

Re: [PATCH 1/1] parser: add support for asm goto

[Christopher Li]

Hi Jirl,

Thank you for the patch. Over all looks good.

Can you add a validations test case for me? It would be great to see
sparse fail on the test case then your patch makes it work.

Some minor nitpick follows. If you are busy, just give me the test case,
I can do the rest of the change for you,


On Thu, Jun 10, 2010 at 1:33 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> As of gcc 4.5, asm goto("jmp %l[label]" : OUT : IN : CLOB : LABELS) is
> supported. Add this support to the parser so that it won't choke on
> the newest Linux kernel when compiling with gcc 4.5.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  evaluate.c |    8 ++++++++
>  parse.c    |   23 +++++++++++++++++++++++
>  parse.h    |    1 +
>  3 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/evaluate.c b/evaluate.c
> index 28bfd7c..5bb26df 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -3149,6 +3149,7 @@ static void verify_input_constraint(struct expression *expr, const char *constra
>  static void evaluate_asm_statement(struct statement *stmt)
>  {
>        struct expression *expr;
> +       struct symbol *sym;
>        int state;
>
>        expr = stmt->asm_string;
> @@ -3225,6 +3226,13 @@ static void evaluate_asm_statement(struct statement *stmt)
>                        continue;
>                expression_error(expr, "asm clobber is not a string");
>        } END_FOR_EACH_PTR(expr);
> +
> +       FOR_EACH_PTR(stmt->asm_labels, sym) {
> +               if (!sym || sym->type != SYM_LABEL) {
> +                       sparse_error(stmt->pos, "bad asm label");
> +                       return;
> +               }
> +       } END_FOR_EACH_PTR(sym);
>  }
>
>  static void evaluate_case_statement(struct statement *stmt)
> diff --git a/parse.c b/parse.c
> index f81b19f..e292570 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -1889,12 +1889,33 @@ static struct token *parse_asm_clobbers(struct token *token, struct statement *s
>        return token;
>  }
>
> +static struct token *parse_asm_labels(struct token *token, struct statement *stmt,
> +                       struct symbol_list **labels)
> +{
> +       struct symbol *label;

What is the stmt argument doing here?

> +
> +       do {
> +               token = token->next; /* skip ':' and ',' */

I think it would be better to enter parse_asm_labels with ':' already
skipped. You can modify the caller do:
parse_asm_lablels(token->next).

It is tempting to make the function name as parse_label_list() because
without ':', there is nothing specific about asm any more. It is your
call.

The while loop can write as follows, with fewer break outs
and read better for me.

while (token_type(token) == TOKEN_IDENT) {
          struct symbol *label = label_symbol(token);
          add_symbol(lables, label);
          token = token->next;
          if (!match_op(token, ',')
                      break;
          token = token->next;
}



> +               if (token_type(token) != TOKEN_IDENT)
> +                       return token;
> +               label = label_symbol(token);
> +               add_symbol(labels, label);
> +               token = token->next;
> +       } while (match_op(token, ','));
> +       return token;
> +}
> +
>  static struct token *parse_asm_statement(struct token *token, struct statement *stmt)
>  {
> +       int is_goto = 0;
> +
>        token = token->next;
>        stmt->type = STMT_ASM;
>        if (match_idents(token, &__volatile___ident, &__volatile_ident, &volatile_ident, NULL)) {
>                token = token->next;
> +       } else if (match_idents(token, &goto_ident, NULL)) {

match_idents is over kill here. You just need to do:
else if (token->ident == &goto_ident) {


> +               is_goto = 1;
> +               token = token->next;
>        }
>        token = expect(token, '(', "after asm");
>        token = parse_expression(token, &stmt->asm_string);
> @@ -1904,6 +1925,8 @@ static struct token *parse_asm_statement(struct token *token, struct statement *
>                token = parse_asm_operands(token, stmt, &stmt->asm_inputs);
>        if (match_op(token, ':'))
>                token = parse_asm_clobbers(token, stmt, &stmt->asm_clobbers);
> +       if (is_goto && match_op(token, ':'))
> +               token = parse_asm_labels(token, stmt, &stmt->asm_labels);

parse_asm_labels(token->next ....) to match previous change.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] parser: add support for asm goto

[Jiri Slaby]

On 06/10/2010 09:18 PM, Christopher Li wrote:
> Hi Jirl,

Hi, (the last letter in my name should be small I :))

> Can you add a validations test case for me? It would be great to see
> sparse fail on the test case then your patch makes it work.

static inline int __static_cpu_has(unsigned char bit)
{
        asm goto("1: jmp %l[t_no]\n"
                 "2:\n"
                 ".section .altinstructions,\"a\"\n"
                 "\n"
                 "1b\n"
                 "0\n"         /* no replacement */
                 " .byte %P0\n"         /* feature bit */
                 " .byte 2b - 1b\n"     /* source len */
                 " .byte 0\n"           /* replacement len */
                 " .byte 0xff + 0 - (2b-1b)\n"  /* padding */
                 ".previous\n"
                 : : "i" (bit) : : t_no, ble);
        return 1;
t_no:
        return 0;
}

> Some minor nitpick follows. If you are busy, just give me the test case,
> I can do the rest of the change for you,

If you could do it, I would appreciate that.

thanks,
-- 
js
suse labs

Re: [PATCH 1/1] parser: add support for asm goto

[Christopher Li]

On Fri, Jun 11, 2010 at 8:05 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> On 06/10/2010 09:18 PM, Christopher Li wrote:
>> Hi Jirl,
>
> Hi, (the last letter in my name should be small I :))

Ah, sorry about that.

>
> static inline int __static_cpu_has(unsigned char bit)
> {
>        asm goto("1: jmp %l[t_no]\n"
>                 "2:\n"
>                 ".section .altinstructions,\"a\"\n"
>                 "\n"
>                 "1b\n"
>                 "0\n"         /* no replacement */
>                 " .byte %P0\n"         /* feature bit */
>                 " .byte 2b - 1b\n"     /* source len */
>                 " .byte 0\n"           /* replacement len */
>                 " .byte 0xff + 0 - (2b-1b)\n"  /* padding */
>                 ".previous\n"
>                 : : "i" (bit) : : t_no, ble);
>        return 1;
> t_no:
>        return 0;
> }
>

I will add that as test case.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] parser: add support for asm goto

[Christopher Li]

On Fri, Jun 11, 2010 at 1:21 PM, Christopher Li <sparse@chrisli.org> wrote:
>> static inline int __static_cpu_has(unsigned char bit)
>> {
>>        asm goto("1: jmp %l[t_no]\n"
>>                 "2:\n"
>>                 ".section .altinstructions,\"a\"\n"
>>                 "\n"
>>                 "1b\n"
>>                 "0\n"         /* no replacement */
>>                 " .byte %P0\n"         /* feature bit */
>>                 " .byte 2b - 1b\n"     /* source len */
>>                 " .byte 0\n"           /* replacement len */
>>                 " .byte 0xff + 0 - (2b-1b)\n"  /* padding */
>>                 ".previous\n"
>>                 : : "i" (bit) : : t_no, ble);
>>        return 1;
>> t_no:
>>        return 0;
>> }
>>

Hi, I update the chrisl branch with your change and the test case.

Please verify it. I hope I did not do some thing stupid there.

It just hit me, is "asm volatile goto" or "asm goto volatile" valid?
If so, the asm goto code needs some change.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] parser: add support for asm goto

[Jiri Slaby]

On 06/18/2010 02:35 AM, Christopher Li wrote:
> On Fri, Jun 11, 2010 at 1:21 PM, Christopher Li <sparse@chrisli.org> wrote:
>>> static inline int __static_cpu_has(unsigned char bit)
>>> {
>>>        asm goto("1: jmp %l[t_no]\n"
>>>                 "2:\n"
>>>                 ".section .altinstructions,\"a\"\n"
>>>                 "\n"
>>>                 "1b\n"
>>>                 "0\n"         /* no replacement */
>>>                 " .byte %P0\n"         /* feature bit */
>>>                 " .byte 2b - 1b\n"     /* source len */
>>>                 " .byte 0\n"           /* replacement len */
>>>                 " .byte 0xff + 0 - (2b-1b)\n"  /* padding */
>>>                 ".previous\n"
>>>                 : : "i" (bit) : : t_no, ble);
>>>        return 1;
>>> t_no:
>>>        return 0;
>>> }
>>>
> 
> Hi, I update the chrisl branch with your change and the test case.
> 
> Please verify it. I hope I did not do some thing stupid there.

Where can I find it?

> It just hit me, is "asm volatile goto" or "asm goto volatile" valid?
> If so, the asm goto code needs some change.

Holy crap, in docco, there is nothing like that, but I checked gcc
parser sources to be sure, and there indeed is:
     asm type-qualifier[opt] ( asm-argument ) ;
     asm type-qualifier[opt] goto ( asm-goto-argument ) ;
So yes, asm volatile goto is valid. Would you take care of that?

-- 
js
suse labs

Re: [PATCH 1/1] parser: add support for asm goto

[Jiri Slaby]

On 06/18/2010 10:21 AM, Jiri Slaby wrote:
> On 06/18/2010 02:35 AM, Christopher Li wrote:
>> Hi, I update the chrisl branch with your change and the test case.
>>
>> Please verify it. I hope I did not do some thing stupid there.
> 
> Where can I find it?

Ah I see now, not chrisl branch, but chrisl repo.

-- 
js
suse labs

Re: [PATCH 1/1] parser: add support for asm goto

[Jiri Slaby]

On 06/18/2010 10:21 AM, Jiri Slaby wrote:
> Holy crap, in docco, there is nothing like that, but I checked gcc
> parser sources to be sure, and there indeed is:
>      asm type-qualifier[opt] ( asm-argument ) ;
>      asm type-qualifier[opt] goto ( asm-goto-argument ) ;
> So yes, asm volatile goto is valid. Would you take care of that?

It is as easy as:
diff --git a/parse.c b/parse.c
index caf10b9..1d20658 100644
--- a/parse.c
+++ b/parse.c
@@ -1915,7 +1915,8 @@ static struct token *parse_asm_statement(struct
token *token, struct statement *
        stmt->type = STMT_ASM;
        if (match_idents(token, &__volatile___ident, &__volatile_ident,
&volatile_ident, NULL)) {
                token = token->next;
-       } else if (match_idents(token, &goto_ident, NULL)) {
+       }
+       if (match_idents(token, &goto_ident, NULL)) {
                is_goto = 1;
                token = token->next;
        }

thanks,
-- 
js
suse labs

Re: [PATCH 1/1] parser: add support for asm goto

[Christopher Li]

On Fri, Jun 18, 2010 at 1:29 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> On 06/18/2010 10:21 AM, Jiri Slaby wrote:
>> Holy crap, in docco, there is nothing like that, but I checked gcc
>> parser sources to be sure, and there indeed is:
>>      asm type-qualifier[opt] ( asm-argument ) ;
>>      asm type-qualifier[opt] goto ( asm-goto-argument ) ;
>> So yes, asm volatile goto is valid. Would you take care of that?
>
> It is as easy as:
> diff --git a/parse.c b/parse.c
> index caf10b9..1d20658 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -1915,7 +1915,8 @@ static struct token *parse_asm_statement(struct
> token *token, struct statement *
>        stmt->type = STMT_ASM;
>        if (match_idents(token, &__volatile___ident, &__volatile_ident,
> &volatile_ident, NULL)) {
>                token = token->next;
> -       } else if (match_idents(token, &goto_ident, NULL)) {
> +       }
> +       if (match_idents(token, &goto_ident, NULL)) {
>                is_goto = 1;
>                token = token->next;
>        }

Looks good to me.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html