| Message ID | CANeU7QkmtVVPhhanR2M7=3P9cCse0B9rXJSfx8c1NCC4aHZ-qw@mail.gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On 20/07/17 13:02, Christopher Li wrote: > On Wed, Jul 19, 2017 at 4:13 PM, Ramsay Jones > <ramsay@ramsayjones.plus.com> 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@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 20, 2017 at 6:44 PM, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > > > On 20/07/17 13:02, Christopher Li wrote: >> On Wed, Jul 19, 2017 at 4:13 PM, Ramsay Jones >> <ramsay@ramsayjones.plus.com> 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. I second this opinion. In the kernel, stacks are quite small and it's very natural to: - limit stack use to the minimal - control stack usage and so VLAs are avoided (but not banned). But in userspace this limit doesn't exist so why make the code more complicated? > My second thought was, have you done some timing tests (no I haven't) > and determined that this causes a noticeable slowdown? Also, this is not IMO -rc4+ material. -- Luc -- 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
On Sat, Jul 29, 2017 at 9:17 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: >> My first reaction was surprise that you didn't go for the Makefile >> idea - setting '-Wno-vla' would be the simplest solution. ;-) Yes, I am convince too. I haven't thought of that. You can submit a patch for '-Wno-vla'. I will apply it, but most likely after this release. >> My second thought was, have you done some timing tests (no I haven't) >> and determined that this causes a noticeable slowdown? I did some limited test it is within std on kernel compile. But I am not very happy about the stack usage per macro will expand to 8 argument structs regardless how few arguments used myself. The -Wno-vla is better idea. > Also, this is not IMO -rc4+ material. Agree, because it changes the code behavior. Do you mind have the '-Wno-vla' in -rc5? That should not change any code it generate. I am fine either way. 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
On Sat, Jul 29, 2017 at 12:16:53PM -0400, Christopher Li wrote: > On Sat, Jul 29, 2017 at 9:17 AM, Luc Van Oostenryck > > Also, this is not IMO -rc4+ material. > > Agree, because it changes the code behavior. Do you mind have the '-Wno-vla' > in -rc5? That should not change any code it generate. I am fine either way. I don't mind. -- Luc -- 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
--- 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