[PATCH -next] lib/vsprintf: Make function pointer_string static

[PATCH -next] lib/vsprintf: Make function pointer_string static

[Yue Haibing]

From: YueHaibing <yuehaibing@huawei.com>

Fix sparse warning:

lib/vsprintf.c:673:6: warning:
 symbol 'pointer_string' was not declared. Should it be static?

Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 lib/vsprintf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1f367f3..7b0a614 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -670,8 +670,9 @@ char *string(char *buf, char *end, const char *s,
 	return string_nocheck(buf, end, s, spec);
 }
 
-char *pointer_string(char *buf, char *end, const void *ptr,
-		     struct printf_spec spec)
+static char *pointer_string(char *buf, char *end,
+			    const void *ptr,
+			    struct printf_spec spec)
 {
 	spec.base = 16;
 	spec.flags |= SMALL;
-- 
2.7.4

Re: [PATCH -next] lib/vsprintf: Make function pointer_string static

[Steven Rostedt]

On Sat, 27 Apr 2019 00:46:30 +0800
Yue Haibing <yuehaibing@huawei.com> wrote:

> From: YueHaibing <yuehaibing@huawei.com>
> 
> Fix sparse warning:
> 
> lib/vsprintf.c:673:6: warning:
>  symbol 'pointer_string' was not declared. Should it be static?
> 
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
>  lib/vsprintf.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 1f367f3..7b0a614 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -670,8 +670,9 @@ char *string(char *buf, char *end, const char *s,
>  	return string_nocheck(buf, end, s, spec);
>  }
>  
> -char *pointer_string(char *buf, char *end, const void *ptr,
> -		     struct printf_spec spec)
> +static char *pointer_string(char *buf, char *end,

Looks like commit "vsprintf: Do not check address of well-known
strings" removed the: "static noinline_for_stack"

Does pointer_string() need that still?

Petr?

-- Steve


> +			    const void *ptr,
> +			    struct printf_spec spec)
>  {
>  	spec.base = 16;
>  	spec.flags |= SMALL;

Re: [PATCH -next] lib/vsprintf: Make function pointer_string static

[Petr Mladek]

On Fri 2019-04-26 13:02:04, Steven Rostedt wrote:
> On Sat, 27 Apr 2019 00:46:30 +0800
> Yue Haibing <yuehaibing@huawei.com> wrote:
> 
> > From: YueHaibing <yuehaibing@huawei.com>
> > 
> > Fix sparse warning:
> > 
> > lib/vsprintf.c:673:6: warning:
> >  symbol 'pointer_string' was not declared. Should it be static?
> > 
> > Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> > ---
> >  lib/vsprintf.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 1f367f3..7b0a614 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -670,8 +670,9 @@ char *string(char *buf, char *end, const char *s,
> >  	return string_nocheck(buf, end, s, spec);
> >  }
> >  
> > -char *pointer_string(char *buf, char *end, const void *ptr,
> > -		     struct printf_spec spec)
> > +static char *pointer_string(char *buf, char *end,
> 
> Looks like commit "vsprintf: Do not check address of well-known
> strings" removed the: "static noinline_for_stack"
> 
> Does pointer_string() need that still?

Heh, it was removed by mistake and well hidden in the diff.

I have pushed Yue's fix into printk.git, branch
for-5.2-vsprintf-hardening

Thanks for the patch.

Best Regards,
Petr

Re: [PATCH -next] lib/vsprintf: Make function pointer_string static

[Steven Rostedt]

On Mon, 29 Apr 2019 13:08:01 +0200
Petr Mladek <pmladek@suse.com> wrote:

> > Looks like commit "vsprintf: Do not check address of well-known
> > strings" removed the: "static noinline_for_stack"
> > 
> > Does pointer_string() need that still?  
> 
> Heh, it was removed by mistake and well hidden in the diff.
> 
> I have pushed Yue's fix into printk.git, branch
> for-5.2-vsprintf-hardening
> 
> Thanks for the patch.

But doesn't it still need the "noinline_for_stack", that doesn't look
like it changed.

-- Steve

Re: [PATCH -next] lib/vsprintf: Make function pointer_string static

[Petr Mladek]

On Mon 2019-04-29 09:13:20, Steven Rostedt wrote:
> On Mon, 29 Apr 2019 13:08:01 +0200
> Petr Mladek <pmladek@suse.com> wrote:
> 
> > > Looks like commit "vsprintf: Do not check address of well-known
> > > strings" removed the: "static noinline_for_stack"
> > > 
> > > Does pointer_string() need that still?  
> > 
> > Heh, it was removed by mistake and well hidden in the diff.
> > 
> > I have pushed Yue's fix into printk.git, branch
> > for-5.2-vsprintf-hardening
> > 
> > Thanks for the patch.
> 
> But doesn't it still need the "noinline_for_stack", that doesn't look
> like it changed.

Good question. I have just double checked it. And pointer_string() with
"noinline_for_stack" does not make any difference in the stack
usage here.


I actually played with this before:

"noinline_for_stack" is a black magic added by
the commit cf3b429b03e827c7180 ("vsprintf.c: use noinline_for_stack").

It is evidently useful in some cases. But I somehow doubt
that it really makes things better when used everywhere.
Therefore I have got a bit relaxed and omitted it in most
newly added functions that did not affect the results.

They are the same before and after the patchset:

pmladek@pathway:/prace/kernel/linux-printk> objdump -d lib/vsprintf.o | perl scripts/checkstack.pl
0x00000e12 symbol_string [vsprintf.o]:                  248
0x00000e6d symbol_string [vsprintf.o]:                  248
0x000012fb ip6_addr_string_sa [vsprintf.o]:             112
0x00001415 ip6_addr_string_sa [vsprintf.o]:             112
0x000028c6 resource_string.isra.9 [vsprintf.o]:         104
0x00002964 resource_string.isra.9 [vsprintf.o]:         104


Would you like to fix this clearly, for example, rebase and
put both "static noinline_for_stack" back or add yet
another commit or?

IMHO, it is not too important. Anyway, I am open for any
advice. I do not want to create more mess.

Best Regards,
Petr

Re: [PATCH -next] lib/vsprintf: Make function pointer_string static

[Steven Rostedt]

[ added Joe ]

On Mon, 29 Apr 2019 16:30:37 +0200
Petr Mladek <pmladek@suse.com> wrote:

> On Mon 2019-04-29 09:13:20, Steven Rostedt wrote:
> > On Mon, 29 Apr 2019 13:08:01 +0200
> > Petr Mladek <pmladek@suse.com> wrote:
> >   
> > > > Looks like commit "vsprintf: Do not check address of well-known
> > > > strings" removed the: "static noinline_for_stack"
> > > > 
> > > > Does pointer_string() need that still?    
> > > 
> > > Heh, it was removed by mistake and well hidden in the diff.
> > > 
> > > I have pushed Yue's fix into printk.git, branch
> > > for-5.2-vsprintf-hardening
> > > 
> > > Thanks for the patch.  
> > 
> > But doesn't it still need the "noinline_for_stack", that doesn't look
> > like it changed.  
> 
> Good question. I have just double checked it. And pointer_string() with
> "noinline_for_stack" does not make any difference in the stack
> usage here.
> 
> 
> I actually played with this before:
> 
> "noinline_for_stack" is a black magic added by
> the commit cf3b429b03e827c7180 ("vsprintf.c: use noinline_for_stack").

From what I understand, "noinline_for_stack" is just noinline and the
"for_stack" part is just to document that the noinline is used for
stack purposes. If the compiler doesn't inline the function without the
noinline, then it wont make any difference.

The point was to not inline the function because it can be used in
stack critical areas, and that it's better to do the call than to
increase the stack.

Although the commit you posted is back in 2010, and compilers and I
think even our stack size has changed since then. This may not be a
critical issue today, and just making it static and letting the
compiler decide might be the right solution.

Let's add Yue's patch as is for now, and perhaps revisit this as a
separate thread.

-- Steve

> 
> It is evidently useful in some cases. But I somehow doubt
> that it really makes things better when used everywhere.
> Therefore I have got a bit relaxed and omitted it in most
> newly added functions that did not affect the results.
> 
> They are the same before and after the patchset:
> 
> pmladek@pathway:/prace/kernel/linux-printk> objdump -d lib/vsprintf.o | perl scripts/checkstack.pl
> 0x00000e12 symbol_string [vsprintf.o]:                  248
> 0x00000e6d symbol_string [vsprintf.o]:                  248
> 0x000012fb ip6_addr_string_sa [vsprintf.o]:             112
> 0x00001415 ip6_addr_string_sa [vsprintf.o]:             112
> 0x000028c6 resource_string.isra.9 [vsprintf.o]:         104
> 0x00002964 resource_string.isra.9 [vsprintf.o]:         104
> 
> 
> Would you like to fix this clearly, for example, rebase and
> put both "static noinline_for_stack" back or add yet
> another commit or?
> 
> IMHO, it is not too important. Anyway, I am open for any
> advice. I do not want to create more mess.
> 
> Best Regards,
> Petr

Re: [PATCH -next] lib/vsprintf: Make function pointer_string static

[Joe Perches]

On Mon, 2019-04-29 at 10:39 -0400, Steven Rostedt wrote:
> [ added Joe ]
> > Good question. I have just double checked it. And pointer_string() with
> > "noinline_for_stack" does not make any difference in the stack
> > usage here.
> > 
> > I actually played with this before:
> > 
> > "noinline_for_stack" is a black magic added by
> > the commit cf3b429b03e827c7180 ("vsprintf.c: use noinline_for_stack").
> 
> From what I understand, "noinline_for_stack" is just noinline and the
> "for_stack" part is just to document that the noinline is used for
> stack purposes. If the compiler doesn't inline the function without the
> noinline, then it wont make any difference.
> 
> The point was to not inline the function because it can be used in
> stack critical areas, and that it's better to do the call than to
> increase the stack.

It was added because of %pV is recursive and recursive
functions can eat
a lot of stack.

Using noinline_for_stack was just a bit more self-documenting.

I do still think it's a useful notation.

Re: [PATCH -next] lib/vsprintf: Make function pointer_string static

[Petr Mladek]

On Mon 2019-04-29 09:42:30, Joe Perches wrote:
> On Mon, 2019-04-29 at 10:39 -0400, Steven Rostedt wrote:
> > [ added Joe ]
> > > Good question. I have just double checked it. And pointer_string() with
> > > "noinline_for_stack" does not make any difference in the stack
> > > usage here.
> > > 
> > > I actually played with this before:
> > > 
> > > "noinline_for_stack" is a black magic added by
> > > the commit cf3b429b03e827c7180 ("vsprintf.c: use noinline_for_stack").
> > 
> > From what I understand, "noinline_for_stack" is just noinline and the
> > "for_stack" part is just to document that the noinline is used for
> > stack purposes. If the compiler doesn't inline the function without the
> > noinline, then it wont make any difference.
> > 
> > The point was to not inline the function because it can be used in
> > stack critical areas, and that it's better to do the call than to
> > increase the stack.
> 
> It was added because of %pV is recursive and recursive
> functions can eat
> a lot of stack.
> 
> Using noinline_for_stack was just a bit more self-documenting.
> 
> I do still think it's a useful notation.

I understand the problem and noinline_for_stack improved some
paths definitely.

On the other hand, the call instruction uses the stack as well.
Note that many of the annotated functions have 5 parameters.

I believe that some of the annotated functions might get inlined
with a lower stack usage in the caller than what is needed
by the call.

The problem is that it depends on the used compiler, optimization,
and architecture. I personally do not want to invest much time
into optimizing this unless people report real life problems.

Best Regards,
Petr