[PATCH 0/6] cleanup related to inlining of variadic functions

[PATCH 0/6] cleanup related to inlining of variadic functions

[Luc Van Oostenryck]

From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

Sparse's tree-inline contains or needs some special handling
if the inlined function is variadic.
This series contains some cleanup related to these.

Luc Van Oostenryck (6):
  inline: add testcases for inlining of variadics
  inline: comment about creating node of node on variadics
  inline: declaration of the variadic vars is useless
  inline: avoid needless intermediate vars
  inline: allocate statement after guards
  inline: free symbol list after use

 inline.c                            | 23 +++++++++++++----------
 validation/inline-early/variadic0.c | 13 +++++++++++++
 2 files changed, 26 insertions(+), 10 deletions(-)
 create mode 100644 validation/inline-early/variadic0.c

-- 
2.36.1

[PATCH 1/6] inline: add testcases for inlining of variadics

[Luc Van Oostenryck]

From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

Inlining of variadic functions needs some special cases.
Add some testcases for this.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/inline-early/variadic0.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644 validation/inline-early/variadic0.c

diff --git a/validation/inline-early/variadic0.c b/validation/inline-early/variadic0.c
new file mode 100644
index 000000000000..566e129f029d
--- /dev/null
+++ b/validation/inline-early/variadic0.c
@@ -0,0 +1,13 @@
+static inline void fun(const char *fmt, ...)
+{
+}
+
+void main(void)
+{
+	fun("abc", 0);			// will be a SYM_BASETYPE
+	fun("ijk", (const int)1);	// will be a SYM_NODE
+}
+
+/*
+ * check-name: variadic0
+ */
-- 
2.36.1

[PATCH 2/6] inline: comment about creating node of node on variadics

[Luc Van Oostenryck]

From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

When inlining a variadic function (unsupported in general by
sparse but OK when the arguments are unused and occurs as such
in the kernel), the extra arguments are added in the declaration
list as SYM_NODE.

But these arguments can already be SYM_NODEs. Sparse doesn't
support everywhere such nested nodes (they must be merged) but
in this case it's fine as the node will be merged when evaluated.

Add a comment telling the situation is fine.
Also, move the code to where the variadic arguments are handled
since the fixed one will be anyway directly overwritten.

Note: Sparse doesn't really support inlining of variadic functions
      but is fine when the arguments are not used (and such cases
      occur in the kernel).

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 inline.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/inline.c b/inline.c
index 0097e4bf620a..d031c9b19128 100644
--- a/inline.c
+++ b/inline.c
@@ -542,11 +542,15 @@ int inline_function(struct expression *expr, struct symbol *sym)
 	FOR_EACH_PTR(arg_list, arg) {
 		struct symbol *a = alloc_symbol(arg->pos, SYM_NODE);
 
-		a->ctype.base_type = arg->ctype;
 		if (name) {
 			*a = *name;
 			set_replace(name, a);
 			add_symbol(&fn_symbol_list, a);
+		} else {
+			// This may create a node of a node but it will
+			// be resolved later when the corresponding
+			// STMT_DECLARATION will be evaluated.
+			a->ctype.base_type = arg->ctype;
 		}
 		a->initializer = arg;
 		add_symbol(&arg_decl, a);
-- 
2.36.1

[PATCH 3/6] inline: declaration of the variadic vars is useless

[Luc Van Oostenryck]

From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

When inlining a function call, the arguments of this call must
somehow be assigned to the names used in the function definition.
This is done via a STMT_DECLARATION associated to the top
STMT_COMPOUND which now correspond to the inlined code.

This is perfectly fine for the normal case of non-variadic function
but when inlining a variadic function there is no corresponding name
to assign the non-fixed arguments to (such arguments must either be
not used at all or copied via __builtin_va_arg_pack()). What's then
happening is essentially that these variables are self-assigned.
Not Good.

This seems to be relatively harmless but is confusing.
So only put the fixed/named arguments in the declaration list.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 inline.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/inline.c b/inline.c
index d031c9b19128..4696eb509f9a 100644
--- a/inline.c
+++ b/inline.c
@@ -546,14 +546,14 @@ int inline_function(struct expression *expr, struct symbol *sym)
 			*a = *name;
 			set_replace(name, a);
 			add_symbol(&fn_symbol_list, a);
+			a->initializer = arg;
+			add_symbol(&arg_decl, a);
 		} else {
 			// This may create a node of a node but it will
 			// be resolved later when the corresponding
 			// STMT_DECLARATION will be evaluated.
 			a->ctype.base_type = arg->ctype;
 		}
-		a->initializer = arg;
-		add_symbol(&arg_decl, a);
 
 		NEXT_PTR_LIST(name);
 	} END_FOR_EACH_PTR(arg);
-- 
2.36.1

[PATCH 4/6] inline: avoid needless intermediate vars

[Luc Van Oostenryck]

From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

In inline_function(), the we need to iterate over the parameters
and the (effective) arguments. An itermediate variable is used for
each: "name_list" and "arg_list".

These confuse me a lot (especially "name_list", "param_list" would
be much more OK) and are just used once.

So, avoid using an intermediate variable for these.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 inline.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/inline.c b/inline.c
index 4696eb509f9a..a6f9252ab0ff 100644
--- a/inline.c
+++ b/inline.c
@@ -516,9 +516,8 @@ int inline_function(struct expression *expr, struct symbol *sym)
 {
 	struct symbol_list * fn_symbol_list;
 	struct symbol *fn = sym->ctype.base_type;
-	struct expression_list *arg_list = expr->args;
 	struct statement *stmt = alloc_statement(expr->pos, STMT_COMPOUND);
-	struct symbol_list *name_list, *arg_decl;
+	struct symbol_list *arg_decl;
 	struct symbol *name;
 	struct expression *arg;
 
@@ -529,8 +528,6 @@ int inline_function(struct expression *expr, struct symbol *sym)
 	if (fn->expanding)
 		return 0;
 
-	name_list = fn->arguments;
-
 	expr->type = EXPR_STATEMENT;
 	expr->statement = stmt;
 	expr->ctype = fn->ctype.base_type;
@@ -538,8 +535,8 @@ int inline_function(struct expression *expr, struct symbol *sym)
 	fn_symbol_list = create_symbol_list(sym->inline_symbol_list);
 
 	arg_decl = NULL;
-	PREPARE_PTR_LIST(name_list, name);
-	FOR_EACH_PTR(arg_list, arg) {
+	PREPARE_PTR_LIST(fn->arguments, name);
+	FOR_EACH_PTR(expr->args, arg) {
 		struct symbol *a = alloc_symbol(arg->pos, SYM_NODE);
 
 		if (name) {
-- 
2.36.1

[PATCH 5/6] inline: allocate statement after guards

[Luc Van Oostenryck]

From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

In inline_function(), the statement that will correspond to the
inlined code is allocated in the function declaration but then
it's checked if the function can be allocated or not.

This is not much memory and the checks should succeed most of the time
but it's clearer if the statement is allocated after the checks.

So, move the allocation after the checks.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 inline.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/inline.c b/inline.c
index a6f9252ab0ff..68f235c21945 100644
--- a/inline.c
+++ b/inline.c
@@ -516,7 +516,7 @@ int inline_function(struct expression *expr, struct symbol *sym)
 {
 	struct symbol_list * fn_symbol_list;
 	struct symbol *fn = sym->ctype.base_type;
-	struct statement *stmt = alloc_statement(expr->pos, STMT_COMPOUND);
+	struct statement *stmt;
 	struct symbol_list *arg_decl;
 	struct symbol *name;
 	struct expression *arg;
@@ -528,6 +528,7 @@ int inline_function(struct expression *expr, struct symbol *sym)
 	if (fn->expanding)
 		return 0;
 
+	stmt = alloc_statement(expr->pos, STMT_COMPOUND);
 	expr->type = EXPR_STATEMENT;
 	expr->statement = stmt;
 	expr->ctype = fn->ctype.base_type;
-- 
2.36.1

[PATCH 6/6] inline: free symbol list after use

[Luc Van Oostenryck]

From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

We usually don't free allocated memory because it's not known
when the allocated objects aren't used anymore.

But here it's pretty obvious, so free this symbol list.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 inline.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/inline.c b/inline.c
index 68f235c21945..a7ab73d37492 100644
--- a/inline.c
+++ b/inline.c
@@ -567,6 +567,7 @@ int inline_function(struct expression *expr, struct symbol *sym)
 	stmt->inline_fn = sym;
 
 	unset_replace_list(fn_symbol_list);
+	free_ptr_list(&fn_symbol_list);
 
 	return 1;
 }
-- 
2.36.1

Re: [PATCH 2/6] inline: comment about creating node of node on variadics

[Ramsay Jones]

On 26/06/2022 14:07, Luc Van Oostenryck wrote:
> From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> 
> When inlining a variadic function (unsupported in general by
> sparse but OK when the arguments are unused and occurs as such
> in the kernel), the extra arguments are added in the declaration
> list as SYM_NODE.
> 
> But these arguments can already be SYM_NODEs. Sparse doesn't
> support everywhere such nested nodes (they must be merged) but
> in this case it's fine as the node will be merged when evaluated.
> 
> Add a comment telling the situation is fine.
> Also, move the code to where the variadic arguments are handled
> since the fixed one will be anyway directly overwritten.
> 
> Note: Sparse doesn't really support inlining of variadic functions
>       but is fine when the arguments are not used (and such cases
>       occur in the kernel).

This note prompted a feeling of deja-vu :) It is simply repeating
(in slightly different words) the content of the first paragraph.

ATB,
Ramsay Jones

> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  inline.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/inline.c b/inline.c
> index 0097e4bf620a..d031c9b19128 100644
> --- a/inline.c
> +++ b/inline.c
> @@ -542,11 +542,15 @@ int inline_function(struct expression *expr, struct symbol *sym)
>  	FOR_EACH_PTR(arg_list, arg) {
>  		struct symbol *a = alloc_symbol(arg->pos, SYM_NODE);
>  
> -		a->ctype.base_type = arg->ctype;
>  		if (name) {
>  			*a = *name;
>  			set_replace(name, a);
>  			add_symbol(&fn_symbol_list, a);
> +		} else {
> +			// This may create a node of a node but it will
> +			// be resolved later when the corresponding
> +			// STMT_DECLARATION will be evaluated.
> +			a->ctype.base_type = arg->ctype;
>  		}
>  		a->initializer = arg;
>  		add_symbol(&arg_decl, a);

Re: [PATCH 4/6] inline: avoid needless intermediate vars

[Ramsay Jones]

On 26/06/2022 14:07, Luc Van Oostenryck wrote:
> From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> 
> In inline_function(), the we need to iterate over the parameters

s/the we/we/


ATB,
Ramsay Jones

> and the (effective) arguments. An itermediate variable is used for
> each: "name_list" and "arg_list".
> 
> These confuse me a lot (especially "name_list", "param_list" would
> be much more OK) and are just used once.
> 
> So, avoid using an intermediate variable for these.
> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  inline.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/inline.c b/inline.c
> index 4696eb509f9a..a6f9252ab0ff 100644
> --- a/inline.c
> +++ b/inline.c
> @@ -516,9 +516,8 @@ int inline_function(struct expression *expr, struct symbol *sym)
>  {
>  	struct symbol_list * fn_symbol_list;
>  	struct symbol *fn = sym->ctype.base_type;
> -	struct expression_list *arg_list = expr->args;
>  	struct statement *stmt = alloc_statement(expr->pos, STMT_COMPOUND);
> -	struct symbol_list *name_list, *arg_decl;
> +	struct symbol_list *arg_decl;
>  	struct symbol *name;
>  	struct expression *arg;
>  
> @@ -529,8 +528,6 @@ int inline_function(struct expression *expr, struct symbol *sym)
>  	if (fn->expanding)
>  		return 0;
>  
> -	name_list = fn->arguments;
> -
>  	expr->type = EXPR_STATEMENT;
>  	expr->statement = stmt;
>  	expr->ctype = fn->ctype.base_type;
> @@ -538,8 +535,8 @@ int inline_function(struct expression *expr, struct symbol *sym)
>  	fn_symbol_list = create_symbol_list(sym->inline_symbol_list);
>  
>  	arg_decl = NULL;
> -	PREPARE_PTR_LIST(name_list, name);
> -	FOR_EACH_PTR(arg_list, arg) {
> +	PREPARE_PTR_LIST(fn->arguments, name);
> +	FOR_EACH_PTR(expr->args, arg) {
>  		struct symbol *a = alloc_symbol(arg->pos, SYM_NODE);
>  
>  		if (name) {

Re: [PATCH 2/6] inline: comment about creating node of node on variadics

[Luc Van Oostenryck]

On Sun, Jun 26, 2022 at 03:42:30PM +0100, Ramsay Jones wrote:
> 
> 
> On 26/06/2022 14:07, Luc Van Oostenryck wrote:
> > From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> > 
> > When inlining a variadic function (unsupported in general by
> > sparse but OK when the arguments are unused and occurs as such
> > in the kernel), the extra arguments are added in the declaration
> > list as SYM_NODE.
> > 
> > But these arguments can already be SYM_NODEs. Sparse doesn't
> > support everywhere such nested nodes (they must be merged) but
> > in this case it's fine as the node will be merged when evaluated.
> > 
> > Add a comment telling the situation is fine.
> > Also, move the code to where the variadic arguments are handled
> > since the fixed one will be anyway directly overwritten.
> > 
> > Note: Sparse doesn't really support inlining of variadic functions
> >       but is fine when the arguments are not used (and such cases
> >       occur in the kernel).
> 
> This note prompted a feeling of deja-vu :) It is simply repeating
> (in slightly different words) the content of the first paragraph.

Hehe, indeed. I'm really bad at rereading myself.

Thanks for noticing.
-- Luc