On Wed, Apr 03, 2019 at 04:35:48PM +0100, Ben Dooks wrote:
> Add code to parse the __attribute__((format)) used to indicate that
> a variadic function takes a printf-style format string and where
> those are. Save the data in ctype ready for checking when such an
> function is encoutered.
s/.../encountered/
> Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
> ---
> Fixes since v1:
> - moved to using ctype in base_type to store infromation
> - fixed formatting issues
> - updated check for bad format arguments
> - reduced the line count to unsigned short to save space
>
> Fixes since v2:
> - correctly use the decl->ctype to store printf information
> - fixed issues with checking format positions for va_list code
> - parse but ignore scanf formatting for now
>
> Fixes since v4:
> - deal with function pointers losing format info
> ---
> parse.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> symbol.h | 2 ++
> 2 files changed, 103 insertions(+), 1 deletion(-)
>
> diff --git a/parse.c b/parse.c
> index f291e24..42ba457 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -136,6 +136,10 @@ static void asm_modifier_inline(struct token *token, unsigned long *mods)
> asm_modifier(token, mods, MOD_INLINE);
> }
>
> +enum {
> + FmtPrintf = 0, FmtScanf,
It would be good to add a small comment explaining the purpose.
Something like: "for the different kinds of 'format' attributes".
> @@ -1172,6 +1194,74 @@ static struct token *attribute_address_space(struct token *token, struct symbol
> return token;
> }
>
> +static int invalid_printf_format_args(long long start, long long at)
> +{
> + return start < 0 || at < 0 || (start == at && start > 0) ||
> + (start == 0 && at == 0);
> +}
> +
> +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->type != KW_FORMAT) {
> + sparse_error(token->pos,
> + "unknown format type '%s'\n",
> + show_ident(token->ident));
For now, it's better to just ignore unknown format type.
> + 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, "incorrect format attribute");
Since there is no optional argument, I think it would be clearer
to not use a loop but instead do something like:
token = expect(token, '(', "after format attribute");
... check for the KW_FORMAT ...
token = expect(token, ',', ...
token = conditional_expression(token, &expr1);
token = expect(token, ',', ...
token = conditional_expression(token, &expr2);
token = expect(token, ')', ...
> @@ -2981,8 +3071,18 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
>
> if (!(decl->ctype.modifiers & MOD_STATIC))
> decl->ctype.modifiers |= MOD_EXTERN;
> +
> + base_type->ctype.printf_msg = decl->ctype.printf_msg;
> + base_type->ctype.printf_va_start = decl->ctype.printf_va_start;
> } else if (base_type == &void_ctype && !(decl->ctype.modifiers & MOD_EXTERN)) {
> sparse_error(token->pos, "void declaration");
> + } else if (base_type && base_type->type == SYM_PTR) {
> + // think this is correct to do //
> + struct symbol *ptr_base = get_base_type(base_type);
> + if (ptr_base) {
> + ptr_base->ctype.printf_msg = decl->ctype.printf_msg;
> + ptr_base->ctype.printf_va_start = decl->ctype.printf_va_start;
> + }
I'll need to check this.
What situation is this supposed to cover?
> diff --git a/symbol.h b/symbol.h
> index ac43b31..7bb6f29 100644
> --- a/symbol.h
> +++ b/symbol.h
> @@ -103,6 +104,7 @@ struct ctype {
> struct context_list *contexts;
> struct ident *as;
> struct symbol *base_type;
> + unsigned short printf_va_start, printf_msg;
> };
Note (mainly to myself): it's unfortunate that this will make
struct ctype bigger but without some generic solution for attributes'
paramaters, this is OK.
-- Luc
