On Wed, Sep 25, 2019 at 11:00:13AM +0100, Ben Dooks wrote:
> diff --git a/evaluate.c b/evaluate.c
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -2319,13 +2319,452 @@ static struct symbol *evaluate_alignof(struct expression *expr)
> return size_t_ctype;
> }
>
> +struct format_type {
> + const char *format;
> + int (*test)(struct format_type *fmt, struct expression **expr, struct symbol *ctype, struct symbol **target, const char **typediff);
> + struct symbol *data;
> +};
> +
> +struct format_state {
> + struct expression *expr;
> + unsigned int va_start;
the prefix 'va_' is useless.
> +static int printf_fmt_print_pointer(struct format_type *fmt, struct expression **expr, struct symbol *ctype, struct symbol **target, const char **typediff)
> +{
> + int ret;
> + *target = &ptr_ctype;
> + ret =check_assignment_types(*target, expr, typediff);
> + if (ret == 0) {
> + /* if just printing, ignore address-space mismatches */
> + if (strcmp(*typediff, "different address spaces") == 0)
That's terrible.
It would be better to copy the ctype and then mask out the address spaces.
> + ret = 1;
> + }
> + return ret;
> +}
> +
> +static struct format_type printf_fmt_ptr_ref = { "p", .test = printf_fmt_pointer, };
Please use a designator for all members:
.format = "p",
.test = ....
> +static struct expression *get_expression_n(struct expression_list *args, int nr)
get_nth_expression() ?
> +static void parse_format_printf_checkpos(struct format_state *state, const char *which)
> +{
> + if (state->used_position)
> + warning(state->expr->pos,
> + "format %d: %s: no position specified",
> + state->arg_index-1, which);
Please, use braces when the body doesn't hold on a single line.
> +static int parse_format_printf_argfield(const char **fmtptr, struct format_state *state, struct expression_list *args, int *pos, const char *which)
> +{
...
> + /* check the vale we got was int/uint type */
typo: value
> + ctype = evaluate_expression(expr);
> + if (ctype) {
> + struct symbol *source, *target = &int_ctype;
> +
> + source = degenerate(expr);
> +
> + if (source != &int_ctype && source != &uint_ctype) {
Is there a good reason to allow uint (the standard only allow a plain int)?
> +/*
> + * printf format parsing code
> + *
> + * this code currently does not:
> + * - check castable types (such as int vs long vs long long)
> + * - validate all arguments specified are also used...
> + */
> +static int parse_format_printf(const char **fmtstring,
> + struct format_state *state,
> + struct expression_list *args)
> +{
> + struct format_type ftype;
> + struct format_type *type;
> + struct expression *expr;
> + const char *fmt = *fmtstring;
> + const char *fmtpost = NULL;
> + int pos = state->arg_index;
> + int error = 0;
> + int ret;
It would be nice to have some comments about the variables, for example
what is the purpose of fmtpost.
> + source = degenerate(expr);
> + ret = (type->test)(type, &expr, ctype, &target, &typediff);
The first set of parentheses are unneeded. Please remove them.
> + } else {
> + /* try and find the end of this */
> + fmtpost = *fmtstring;
> + while (*fmtpost > ' ')
This merits a comment. Also, what is 'this'?
> +static void evaluate_format_printf(const char *fmt_string, struct symbol *fn, struct expression_list *head)
> +{
> + if (fail > 0)
> + /* format string may have '\n' etc embedded in it */
> + warning(expr->pos, "cannot evaluate format string");
If fail > 0, a specific warnings has already been issued, right?
then this warning should be removed.
> static int evaluate_arguments(struct symbol *fn, struct expression_list *head)
> {
> struct expression *expr;
> struct symbol_list *argument_types = fn->arguments;
> + const char *fmt_string = NULL;
> struct symbol *argtype;
> int i = 1;
>
> + /*
> + * do this first, otherwise the arugment info may get lost or changed
typo: argument.
Maybe change the message to something like:
Do first part of (printf) format checking. This need to be done
here, ...
-- Luc
