[PATCH 2/2] pre-process: replace use of vla's with heap allocation

[PATCH 2/2] pre-process: replace use of vla's with heap allocation

[Ramsay Jones]

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.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Chris,

This is the 'obvious' fix-up, with potential performance problems,
that I mentioned yesterday.

With these two patches on top of today's sparse-next (@f976ce2), the
'make selfcheck' is clean for me on Linux (x86_64).

BTW, I have tested today's 'sparse-next' branch on x86_64 Linux and
cygwin (without problems), but not yet i686 Linux.

ATB,
Ramsay Jones

 
 pre-process.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/pre-process.c b/pre-process.c
index 74414df..0063f8b 100644
--- a/pre-process.c
+++ b/pre-process.c
@@ -709,13 +709,16 @@ static int expand(struct token **list, struct symbol *sym)
 	struct ident *expanding = token->ident;
 	struct token **tail;
 	int nargs = sym->arglist ? sym->arglist->count.normal : 0;
-	struct arg args[nargs];
+	struct arg *args = NULL;
 
 	if (expanding->tainted) {
 		token->pos.noexpand = 1;
 		return 1;
 	}
 
+	if (nargs > 0)
+		args = calloc(nargs, sizeof(*args));
+
 	if (sym->arglist) {
 		if (!match_op(scan_next(&token->next), '('))
 			return 1;
@@ -738,6 +741,8 @@ static int expand(struct token **list, struct symbol *sym)
 	(*list)->pos.whitespace = token->pos.whitespace;
 	*tail = last;
 
+	free(args);
+
 	return 0;
 }
 
@@ -2016,9 +2021,12 @@ struct token * preprocess(struct token *token)
 static void dump_macro(struct symbol *sym)
 {
 	int nargs = sym->arglist ? sym->arglist->count.normal : 0;
-	struct token *args[nargs];
+	struct token **args = NULL;
 	struct token *token;
 
+	if (nargs > 0)
+		args = calloc(nargs, sizeof(*args));
+
 	printf("#define %s", show_ident(sym->ident));
 	token = sym->arglist;
 	if (token) {
@@ -2053,6 +2061,8 @@ static void dump_macro(struct symbol *sym)
 		token = next;
 	}
 	putchar('\n');
+
+	free(args);
 }
 
 void dump_macro_definitions(void)
-- 
2.13.0

Re: [PATCH 2/2] pre-process: replace use of vla's with heap allocation

[Christopher Li]

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.

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.

> +
>         if (sym->arglist) {
>                 if (!match_op(scan_next(&token->next), '('))
>                         return 1;

Need to free alloc memory.


> +       if (nargs > 0)
> +               args = calloc(nargs, sizeof(*args));

Same here need to check alloc failed.

Chris

Purposed incremental fix up follows:

--- 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;

Re: [PATCH 2/2] pre-process: replace use of vla's with heap allocation

[Ramsay Jones]

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
> 

Re: [PATCH 2/2] pre-process: replace use of vla's with heap allocation

[Luc Van Oostenryck]

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

Re: [PATCH 2/2] pre-process: replace use of vla's with heap allocation

[Christopher Li]

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

Re: [PATCH 2/2] pre-process: replace use of vla's with heap allocation

[Luc Van Oostenryck]

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