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

[Christopher Li <sparse@chrisli.org>]

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