On Fri, Oct 26, 2018 at 04:26:30PM +0100, Ben Dooks wrote:
> ---
> parse.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> symbol.h | 2 ++
> 2 files changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/parse.c b/parse.c
> index 02a55a7..bb0545c 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -1051,6 +1061,69 @@ static struct token *attribute_address_space(struct token *token, struct symbol
> return token;
> }
>
> +static struct token *attribute_format(struct token *token, struct symbol *attr, struct decl_state *ctx)
> +{
> + struct expression *args[3];
> + struct symbol *fmt_sym = NULL;
> + int argc = 0;
> +
> + /* expecting format ( type, start, va_args at) */
> +
> + token = expect(token, '(', "after format attribute");
> + while (!match_op(token, ')')) {
> + struct expression *expr = NULL;
> +
> + if (argc == 0) {
> + if (token_type(token) == TOKEN_IDENT)
> + fmt_sym = lookup_keyword(token->ident, NS_KEYWORD);
> +
> + if (!fmt_sym || !fmt_sym->op ||
> + fmt_sym->op != &attr_printf_op) {
Better to check for op->type == KW_FORMAT so it can later be easily extended.
> + sparse_error(token->pos,
> + "unknown format type '%s'\n",
> + show_ident(token->ident));
> + fmt_sym = NULL;
> + }
> + }
> +
> + token = conditional_expression(token, &expr);
> + if (!expr)
> + break;
> + if (argc < 3)
> + args[argc++] = expr;
> + if (!match_op(token, ','))
> + break;
> + token = token->next;
> + }
> +
> + if (argc != 3 || !fmt_sym)
> + warning(token->pos,
> + "expected format type and start/position values");
> + else {
> + struct symbol *base_type = ctx->ctype.base_type;
> + long long start, at;
> +
> + start = get_expression_value(args[2]);
> + at = get_expression_value(args[1]);
> +
> + if (start <= 0 || at <= 0)
> + warning(token->pos, "bad format positions");
> + else if (!base_type)
> + sparse_error(token->pos, "no base context for format");
> + else if (base_type->type != SYM_FN)
> + warning(token->pos, "attribute format can only be used on functions");
> + else if (!base_type->variadic)
> + warning(token->pos, "attribute format used on non variadic function");
> + else {
> + base_type->printf_va_start = start;
> + base_type->printf_msg = at;
> + }
Just a stylistic nit and as such not really imprtant but I would prefer
in the next version that chained ifs either:
* all of them take only a single line and then doesn't take braces
* otherwise that they all take the braces
This is the same as done in the kernel.
> + }
> +
> + token = expect(token, ')', "after format attribute");
> + return token;
> +}
> +
> static struct symbol *to_QI_mode(struct symbol *ctype)
> {
> if (ctype->ctype.base_type != &int_type)
> diff --git a/symbol.h b/symbol.h
> index 3274496..5f1b85d 100644
> --- a/symbol.h
> +++ b/symbol.h
> @@ -86,6 +86,7 @@ enum keyword {
> KW_SHORT = 1 << 7,
> KW_LONG = 1 << 8,
> KW_EXACT = 1 << 9,
> + KW_FORMAT = 1 << 10,
> };
If not using KW_FORMAT in the test here above, it's not really used and needed..
> struct context {
> @@ -185,6 +186,7 @@ struct symbol {
> struct entrypoint *ep;
> long long value; /* Initial value */
> struct symbol *definition;
> + long long printf_va_start, printf_msg;
This is annoying. This struct is the most used one and is already
quite large and thus uses a significant amount of the allocated memory
which represent a non-negligeable amount of the processing time.
There is several options:
* a long long is certainly not needed for these, 32 bits 16 or even 8
should be enough
* they could be 'unionized' with some of the other fields
* it could be moved elsewhere ...
More fundamentally these two fields should not belong to struct symbol
but to struct ctype (for example next to 'as' make thme each 8 bit witdh).
Thanks,
-- Luc
