On 20/07/17 13:02, Christopher Li wrote:
> On Wed, Jul 19, 2017 at 4:13 PM, Ramsay Jones
> <ramsay@xxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> The 'selfcheck' make target issues warnings about using vla's in the
>> pre-processor code, like so:
>>
>> CHECK pre-process.c
>> pre-process.c:712:25: warning: Variable length array is used.
>> pre-process.c:2019:28: warning: Variable length array is used.
>>
>> A Makefile change to pass '-Wno-vla' to sparse when processing this
>> source file (or all source files) may be a better solution than the
>> one given here.
>>
>> Replace the use of vla's with heap allocation. This has performance
>> implications (although it may me safer), due to the dynamic memory
>> allocation and the zero initialisation of the memory (using calloc).
>> I have not done any timing measurements to see if this is a problem
>> in practice.
>
> I purpose the following patch. Make the expand using stack for small
> argument numbers. That should not have much performance impact
> at all because long macro arguments are rare.
My first reaction was surprise that you didn't go for the Makefile
idea - setting '-Wno-vla' would be the simplest solution. ;-)
I can understand warning about vla usage (especially in the kernel),
but it a 'standard' supported feature. I don't use them myself, partly
because they are a 'relatively' new feature, but also because I have had
some bad experience in the past using alloca in similar circumstances.
My second thought was, have you done some timing tests (no I haven't)
and determined that this causes a noticeable slowdown?
> Incremental patch follows. If you think that is fine, I will apply the
> combined patch as yours.
>
>> + if (nargs > 0)
>> + args = calloc(nargs, sizeof(*args));
>
> Need to check alloc failed.
Yes, indeed! *blush*
>> +
>> if (sym->arglist) {
>> if (!match_op(scan_next(&token->next), '('))
>> return 1;
>
> Need to free alloc memory.
Ahem, I obviously didn't think about this patch too much! :-D
>
>
>> + if (nargs > 0)
>> + args = calloc(nargs, sizeof(*args));
>
> Same here need to check alloc failed.
>
> Chris
>
> Purposed incremental fix up follows:
Hmm, dunno.
I did, briefly, think about adding an 'array' capability to the
sparse ALLOCATOR facility (you can only allocate single instances
from the current allocators - ignoring 'string' and 'bytes').
ATB,
Ramsay Jones
>
> --- sparse.chrisl.orig/pre-process.c
> +++ sparse.chrisl/pre-process.c
> @@ -709,21 +709,30 @@ static int expand(struct token **list, s
> struct ident *expanding = token->ident;
> struct token **tail;
> int nargs = sym->arglist ? sym->arglist->count.normal : 0;
> - struct arg *args = NULL;
> +#define ARG_LIMIT 8
> + struct arg arg_array[ARG_LIMIT], *args = arg_array;
> + int err = 0;
>
> if (expanding->tainted) {
> token->pos.noexpand = 1;
> return 1;
> }
>
> - if (nargs > 0)
> + if (nargs >= ARG_LIMIT) {
> args = calloc(nargs, sizeof(*args));
> + if (!args)
> + die("calloc(%d, %lu) failed", nargs, sizeof(*args));
> + }
>
> if (sym->arglist) {
> - if (!match_op(scan_next(&token->next), '('))
> - return 1;
> - if (!collect_arguments(token->next, sym->arglist, args, token))
> - return 1;
> + if (!match_op(scan_next(&token->next), '(')) {
> + err = 1;
> + goto exit;
> + }
> + if (!collect_arguments(token->next, sym->arglist, args, token)) {
> + err = 1;
> + goto exit;
> + }
> expand_arguments(nargs, args);
> }
>
> @@ -741,9 +750,11 @@ static int expand(struct token **list, s
> (*list)->pos.whitespace = token->pos.whitespace;
> *tail = last;
>
> - free(args);
> +exit:
> + if (nargs >= ARG_LIMIT)
> + free(args);
>
> - return 0;
> + return err;
> }
>
> static const char *token_name_sequence(struct token *token, int
> endop, struct token *start)
> @@ -2024,8 +2035,11 @@ static void dump_macro(struct symbol *sy
> struct token **args = NULL;
> struct token *token;
>
> - if (nargs > 0)
> + if (nargs > 0) {
> args = calloc(nargs, sizeof(*args));
> + if (!args)
> + die("calloc %ld", nargs * sizeof(*args));
> + }
>
> printf("#define %s", show_ident(sym->ident));
> token = sym->arglist;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
