[PATCH] printk-formats.txt: Add examples for %pS and %pF

[PATCH] printk-formats.txt: Add examples for %pS and %pF

[Helge Deller]

Sometimes people seems unclear when to use the %pS or %pF printk format.
Adding some examples may help to avoid such mistakes.

See for example commit 51d96dc2e2dc ("random: fix warning message on ia64 and
parisc") which fixed such a wrong format string.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 65ea591..be8c05b 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -73,6 +73,12 @@ actually function descriptors which must first be resolved. The ``F`` and
 ``f`` specifiers perform this resolution and then provide the same
 functionality as the ``S`` and ``s`` specifiers.
 
+Examples::
+
+	printk("Called from %pS.\n", __builtin_return_address(0));
+	printk("Called from %pS.\n", (void *)regs->ip);
+	printk("Called from %pF.\n", &gettimeofday);
+
 Kernel Pointers
 ===============
 

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Sergey Senozhatsky]

On (08/10/17 19:35), Helge Deller wrote:
> Sometimes people seems unclear when to use the %pS or %pF printk format.
> Adding some examples may help to avoid such mistakes.
> 
> See for example commit 51d96dc2e2dc ("random: fix warning message on ia64 and
> parisc") which fixed such a wrong format string.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 65ea591..be8c05b 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -73,6 +73,12 @@ actually function descriptors which must first be resolved. The ``F`` and
>  ``f`` specifiers perform this resolution and then provide the same
>  functionality as the ``S`` and ``s`` specifiers.
>  
> +Examples::
> +
> +	printk("Called from %pS.\n", __builtin_return_address(0));
> +	printk("Called from %pS.\n", (void *)regs->ip);
> +	printk("Called from %pF.\n", &gettimeofday);

sorry, but how does it help?


there is this paragraph

: On ia64, ppc64 and parisc64 architectures function pointers are
: actually function descriptors which must first be resolved. The ``F`` and
: ``f`` specifiers perform this resolution and then provide the same
: functionality as the ``S`` and ``s`` specifiers.

which supposed to explain everything in details. the examples
don't make it any `clearer', IMHO.


*may be* on "ia64, ppc64 and parisc64" we can somehow check
that the pointer, which we pass as %pS, belongs to .text and
print some build-time warnings. well, if it's actually a
problem. dunno.

	-ss

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Helge Deller]

On 11.08.2017 02:15, Sergey Senozhatsky wrote:
> On (08/10/17 19:35), Helge Deller wrote:
>> Sometimes people seems unclear when to use the %pS or %pF printk format.
>> Adding some examples may help to avoid such mistakes.
>>
>> See for example commit 51d96dc2e2dc ("random: fix warning message on ia64 and
>> parisc") which fixed such a wrong format string.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>> index 65ea591..be8c05b 100644
>> --- a/Documentation/printk-formats.txt
>> +++ b/Documentation/printk-formats.txt
>> @@ -73,6 +73,12 @@ actually function descriptors which must first be resolved. The ``F`` and
>>  ``f`` specifiers perform this resolution and then provide the same
>>  functionality as the ``S`` and ``s`` specifiers.
>>  
>> +Examples::
>> +
>> +	printk("Called from %pS.\n", __builtin_return_address(0));
>> +	printk("Called from %pS.\n", (void *)regs->ip);
>> +	printk("Called from %pF.\n", &gettimeofday);
> 
> sorry, but how does it help?
> 
> 
> there is this paragraph
> 
> : On ia64, ppc64 and parisc64 architectures function pointers are
> : actually function descriptors which must first be resolved. The ``F`` and
> : ``f`` specifiers perform this resolution and then provide the same
> : functionality as the ``S`` and ``s`` specifiers.
> 
> which supposed to explain everything in details. the examples
> don't make it any `clearer', IMHO.

Experts surely do know what function descriptors are.
Nevertheless even those often get it wrong as can be seen in
various commits.

The hope with this patch is to show widely-used examples
and avoid additional commits afterwards to fix it up.

This patch was meant to be RFC.
If you decide not to take it, I'm fine as well.

> *may be* on "ia64, ppc64 and parisc64" we can somehow check
> that the pointer, which we pass as %pS, belongs to .text and
> print some build-time warnings. well, if it's actually a
> problem. dunno.

I think it's not needed. Those bugs will be seen and fixed.

Helge

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Petr Mladek]

On Fri 2017-08-11 09:31:28, Helge Deller wrote:
> On 11.08.2017 02:15, Sergey Senozhatsky wrote:
> > On (08/10/17 19:35), Helge Deller wrote:
> >> Sometimes people seems unclear when to use the %pS or %pF printk format.
> >> Adding some examples may help to avoid such mistakes.
> >>
> >> See for example commit 51d96dc2e2dc ("random: fix warning message on ia64 and
> >> parisc") which fixed such a wrong format string.
> >>
> >> Signed-off-by: Helge Deller <deller@gmx.de>
> >>
> >> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> >> index 65ea591..be8c05b 100644
> >> --- a/Documentation/printk-formats.txt
> >> +++ b/Documentation/printk-formats.txt
> >> @@ -73,6 +73,12 @@ actually function descriptors which must first be resolved. The ``F`` and
> >>  ``f`` specifiers perform this resolution and then provide the same
> >>  functionality as the ``S`` and ``s`` specifiers.
> >>  
> >> +Examples::
> >> +
> >> +	printk("Called from %pS.\n", __builtin_return_address(0));
> >> +	printk("Called from %pS.\n", (void *)regs->ip);
> >> +	printk("Called from %pF.\n", &gettimeofday);
> > 
> > there is this paragraph
> > 
> > : On ia64, ppc64 and parisc64 architectures function pointers are
> > : actually function descriptors which must first be resolved. The ``F`` and
> > : ``f`` specifiers perform this resolution and then provide the same
> > : functionality as the ``S`` and ``s`` specifiers.
> > 
> > which supposed to explain everything in details. the examples
> > don't make it any `clearer', IMHO.
> 
> Experts surely do know what function descriptors are.
> Nevertheless even those often get it wrong as can be seen in
> various commits.

It seems that these specifiers are used the wrong way on many
locations. They might be worth fixing but I cannot test it
easily.

Hmm, using %pF might actually cause a crash when used
on direct function address.


> The hope with this patch is to show widely-used examples
> and avoid additional commits afterwards to fix it up.

IMHO, one problem is that the meaning of ''F'' and ''f''
is hidden at the end of the section. Also the first line

  'For printing symbols and function pointers. The ``S`` and ``s`` '

kind of invites to use ``S`` and ``s`` even for function pointers.
I suggest to switch the order, slightly retranslate, add the
examples, see below.


> This patch was meant to be RFC.
> If you decide not to take it, I'm fine as well.
> 
> > *may be* on "ia64, ppc64 and parisc64" we can somehow check
> > that the pointer, which we pass as %pS, belongs to .text and
> > print some build-time warnings. well, if it's actually a
> > problem. dunno.

I think that it would need to be a runtime check because many/most
printed addresses are not statically defined.


> I think it's not needed. Those bugs will be seen and fixed.

I am not sure how many people are familiar with this problem.
I might help to avoid some headaches when debugging.

If we add the warning, it should be ratelimited to reduce messing
of the original message.

I do not have strong opinion about it.


Here is the updated patch with my proposed changes.
Feel free to update it:


>From ef983c65095cada994c1fe531e2b98e936c943bf Mon Sep 17 00:00:00 2001
From: Helge Deller <deller@gmx.de>
Date: Tue, 15 Aug 2017 11:34:19 +0200
Subject: [PATCH] printk-formats.txt: Better describe the difference between
 %pS and %pF

Sometimes people seems unclear when to use the %pS or %pF printk format.
For example, see commit 51d96dc2e2dc ("random: fix warning message on ia64
and parisc") which fixed such a wrong format string.

The documentation should be more clear about the difference.

Signed-off-by: Helge Deller <deller@gmx.de>
[pmladek@suse.com: Restructure the entire section]
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/printk-formats.txt | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 65ea5915178b..074670b98bac 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -58,20 +58,23 @@ Symbols/Function Pointers
 	%ps	versatile_init
 	%pB	prev_fn_of_versatile_init+0x88/0x88
 
-For printing symbols and function pointers. The ``S`` and ``s`` specifiers
-result in the symbol name with (``S``) or without (``s``) offsets. Where
-this is used on a kernel without KALLSYMS - the symbol address is
-printed instead.
+The ``F`` and ``f`` specifiers are for printing function pointers,
+for example, f->func, &gettimeofday. They have the same result as
+``S`` and ``s`` specifiers. But they do an extra conversion on
+ia64, ppc64 and parisc64 architectures where the function pointers
+are actually function descriptors.
+
+The ``S`` and ``s`` specifiers can be used for printing symbols
+from direct addresses, for example, __builtin_return_address(0),
+(void *)regs->ip. They result in the symbol name with (``S``) or
+without (``s``) offsets. If KALLSYMS are disabled then the symbol
+address is printed instead.
 
 The ``B`` specifier results in the symbol name with offsets and should be
 used when printing stack backtraces. The specifier takes into
 consideration the effect of compiler optimisations which may occur
 when tail-call``s are used and marked with the noreturn GCC attribute.
 
-On ia64, ppc64 and parisc64 architectures function pointers are
-actually function descriptors which must first be resolved. The ``F`` and
-``f`` specifiers perform this resolution and then provide the same
-functionality as the ``S`` and ``s`` specifiers.
 
 Kernel Pointers
 ===============
-- 
1.8.5.6

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Helge Deller]

On 15.08.2017 13:36, Petr Mladek wrote:
> On Fri 2017-08-11 09:31:28, Helge Deller wrote:
>> On 11.08.2017 02:15, Sergey Senozhatsky wrote:
>>> On (08/10/17 19:35), Helge Deller wrote:
>>>> Sometimes people seems unclear when to use the %pS or %pF printk format.
>>>> Adding some examples may help to avoid such mistakes.
>>>>
>>>> See for example commit 51d96dc2e2dc ("random: fix warning message on ia64 and
>>>> parisc") which fixed such a wrong format string.
>>>>
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>
>>>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>>>> index 65ea591..be8c05b 100644
>>>> --- a/Documentation/printk-formats.txt
>>>> +++ b/Documentation/printk-formats.txt
>>>> @@ -73,6 +73,12 @@ actually function descriptors which must first be resolved. The ``F`` and
>>>>  ``f`` specifiers perform this resolution and then provide the same
>>>>  functionality as the ``S`` and ``s`` specifiers.
>>>>  
>>>> +Examples::
>>>> +
>>>> +	printk("Called from %pS.\n", __builtin_return_address(0));
>>>> +	printk("Called from %pS.\n", (void *)regs->ip);
>>>> +	printk("Called from %pF.\n", &gettimeofday);
>>>
>>> there is this paragraph
>>>
>>> : On ia64, ppc64 and parisc64 architectures function pointers are
>>> : actually function descriptors which must first be resolved. The ``F`` and
>>> : ``f`` specifiers perform this resolution and then provide the same
>>> : functionality as the ``S`` and ``s`` specifiers.
>>>
>>> which supposed to explain everything in details. the examples
>>> don't make it any `clearer', IMHO.
>>
>> Experts surely do know what function descriptors are.
>> Nevertheless even those often get it wrong as can be seen in
>> various commits.
> 
> It seems that these specifiers are used the wrong way on many
> locations. 

Yes. %pF usage in mm/memblock.c is just one example.

> They might be worth fixing but I cannot test it easily.

I can check and send patches at some point.
 
> Hmm, using %pF might actually cause a crash when used
> on direct function address.

Probably won't happen on parisc, but basically you are right.


>> The hope with this patch is to show widely-used examples
>> and avoid additional commits afterwards to fix it up.
> 
> IMHO, one problem is that the meaning of ''F'' and ''f''
> is hidden at the end of the section. Also the first line
> 
>   'For printing symbols and function pointers. The ``S`` and ``s`` '
> 
> kind of invites to use ``S`` and ``s`` even for function pointers.
> I suggest to switch the order, slightly retranslate, add the
> examples, see below.
> 
> 
>> This patch was meant to be RFC.
>> If you decide not to take it, I'm fine as well.
>>
>>> *may be* on "ia64, ppc64 and parisc64" we can somehow check
>>> that the pointer, which we pass as %pS, belongs to .text and
>>> print some build-time warnings. well, if it's actually a
>>> problem. dunno.
> 
> I think that it would need to be a runtime check because many/most
> printed addresses are not statically defined.
> 
> 
>> I think it's not needed. Those bugs will be seen and fixed.
> 
> I am not sure how many people are familiar with this problem.
> I might help to avoid some headaches when debugging.
> 
> If we add the warning, it should be ratelimited to reduce messing
> of the original message.
> 
> I do not have strong opinion about it.
> 
> 
> Here is the updated patch with my proposed changes.
> Feel free to update it:

Much better!
Thanks a lot.

Maybe we should mention usage of __func__ with '%s' (see other thread).

And _RET_IP_ is worth mentioning beside __builtin_return_address(0) too,
because it's used quite often wrongly.
   
Helge

> From ef983c65095cada994c1fe531e2b98e936c943bf Mon Sep 17 00:00:00 2001
> From: Helge Deller <deller@gmx.de>
> Date: Tue, 15 Aug 2017 11:34:19 +0200
> Subject: [PATCH] printk-formats.txt: Better describe the difference between
>  %pS and %pF
> 
> Sometimes people seems unclear when to use the %pS or %pF printk format.
> For example, see commit 51d96dc2e2dc ("random: fix warning message on ia64
> and parisc") which fixed such a wrong format string.
> 
> The documentation should be more clear about the difference.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> [pmladek@suse.com: Restructure the entire section]
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  Documentation/printk-formats.txt | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 65ea5915178b..074670b98bac 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -58,20 +58,23 @@ Symbols/Function Pointers
>  	%ps	versatile_init
>  	%pB	prev_fn_of_versatile_init+0x88/0x88
>  
> -For printing symbols and function pointers. The ``S`` and ``s`` specifiers
> -result in the symbol name with (``S``) or without (``s``) offsets. Where
> -this is used on a kernel without KALLSYMS - the symbol address is
> -printed instead.
> +The ``F`` and ``f`` specifiers are for printing function pointers,
> +for example, f->func, &gettimeofday. They have the same result as
> +``S`` and ``s`` specifiers. But they do an extra conversion on
> +ia64, ppc64 and parisc64 architectures where the function pointers
> +are actually function descriptors.
> +
> +The ``S`` and ``s`` specifiers can be used for printing symbols
> +from direct addresses, for example, __builtin_return_address(0),
> +(void *)regs->ip. They result in the symbol name with (``S``) or
> +without (``s``) offsets. If KALLSYMS are disabled then the symbol
> +address is printed instead.
>  
>  The ``B`` specifier results in the symbol name with offsets and should be
>  used when printing stack backtraces. The specifier takes into
>  consideration the effect of compiler optimisations which may occur
>  when tail-call``s are used and marked with the noreturn GCC attribute.
>  
> -On ia64, ppc64 and parisc64 architectures function pointers are
> -actually function descriptors which must first be resolved. The ``F`` and
> -``f`` specifiers perform this resolution and then provide the same
> -functionality as the ``S`` and ``s`` specifiers.
>  
>  Kernel Pointers
>  ===============
> 

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Petr Mladek]

On Tue 2017-08-15 21:58:33, Helge Deller wrote:
> On 15.08.2017 13:36, Petr Mladek wrote:
> > Here is the updated patch with my proposed changes.
> > Feel free to update it:
> 
> Much better!
> Thanks a lot.
> 
> Maybe we should mention usage of __func__ with '%s' (see other thread).
> 
> And _RET_IP_ is worth mentioning beside __builtin_return_address(0) too,
> because it's used quite often wrongly.

Yup, it seems that all this causes confusion. I did not find a good
way to explain this in the text. I think that we need the examples
after all.

Here is an updated version:


>From 818511a3be494b76b8f5ba1c2c6bb38095a9c640 Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Tue, 15 Aug 2017 11:34:19 +0200
Subject: [PATCH] printk-formats.txt: Better describe the difference between
 %pS and %pF

Sometimes people seems unclear when to use the %pS or %pF printk format.
For example, see commit 51d96dc2e2dc ("random: fix warning message on ia64
and parisc") which fixed such a wrong format string.

The documentation should be more clear about the difference.
Also examples might help to avoid some typical mistakes.

Signed-off-by: Helge Deller <deller@gmx.de>
[pmladek@suse.com: Restructure the entire section]
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/printk-formats.txt | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 65ea5915178b..d503080e5174 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -58,20 +58,31 @@ Symbols/Function Pointers
 	%ps	versatile_init
 	%pB	prev_fn_of_versatile_init+0x88/0x88
 
-For printing symbols and function pointers. The ``S`` and ``s`` specifiers
-result in the symbol name with (``S``) or without (``s``) offsets. Where
-this is used on a kernel without KALLSYMS - the symbol address is
-printed instead.
+The ``F`` and ``f`` specifiers are for printing function pointers,
+for example, f->func, &gettimeofday. They have the same result as
+``S`` and ``s`` specifiers. But they do an extra conversion on
+ia64, ppc64 and parisc64 architectures where the function pointers
+are actually function descriptors.
+
+The ``S`` and ``s`` specifiers can be used for printing symbols
+from direct addresses, for example, __builtin_return_address(0),
+(void *)regs->ip. They result in the symbol name with (``S``) or
+without (``s``) offsets. If KALLSYMS are disabled then the symbol
+address is printed instead.
 
 The ``B`` specifier results in the symbol name with offsets and should be
 used when printing stack backtraces. The specifier takes into
 consideration the effect of compiler optimisations which may occur
 when tail-call``s are used and marked with the noreturn GCC attribute.
 
-On ia64, ppc64 and parisc64 architectures function pointers are
-actually function descriptors which must first be resolved. The ``F`` and
-``f`` specifiers perform this resolution and then provide the same
-functionality as the ``S`` and ``s`` specifiers.
+Examples::
+
+	printk("Going to call: %pF\n", gettimeofday);
+	printk("Going to call: %pF\n", p->func);
+	printk("%s: called from %pS\n", __func__, _RET_IP_);
+	printk("%s: called from %pS\n", __func__, __builtin_return_address(0));
+	printk("Faulted at %pS\n", (void *)regs->ip);
+	printk(" %s%pB\n", reliable ? "" : "? ", (void *)*stack);
 
 Kernel Pointers
 ===============
-- 
1.8.5.6

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Steven Rostedt]

On Wed, 23 Aug 2017 16:48:24 +0200
Petr Mladek <pmladek@suse.com> wrote:

> +
> +	printk("Going to call: %pF\n", gettimeofday);
> +	printk("Going to call: %pF\n", p->func);
> +	printk("%s: called from %pS\n", __func__, _RET_IP_);
> +	printk("%s: called from %pS\n", __func__, __builtin_return_address(0));
> +	printk("Faulted at %pS\n", (void *)regs->ip);
> +	printk(" %s%pB\n", reliable ? "" : "? ", (void *)*stack);

Much better!

-- Steve

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Helge Deller]

On 23.08.2017 16:49, Steven Rostedt wrote:
> On Wed, 23 Aug 2017 16:48:24 +0200
> Petr Mladek <pmladek@suse.com> wrote:
> 
>> +
>> +	printk("Going to call: %pF\n", gettimeofday);
>> +	printk("Going to call: %pF\n", p->func);
>> +	printk("%s: called from %pS\n", __func__, _RET_IP_);
>> +	printk("%s: called from %pS\n", __func__, __builtin_return_address(0));
>> +	printk("Faulted at %pS\n", (void *)regs->ip);
>> +	printk(" %s%pB\n", reliable ? "" : "? ", (void *)*stack);
> 
> Much better!

Petr, that's really much better.

I've already pushed the part which restructured the text upstream.
I'll push this part of the patch (which adds the examples) for v4.14.

Thanks,
Helge  

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Petr Mladek]

On Wed 2017-08-23 21:36:21, Helge Deller wrote:
> On 23.08.2017 16:49, Steven Rostedt wrote:
> > On Wed, 23 Aug 2017 16:48:24 +0200
> > Petr Mladek <pmladek@suse.com> wrote:
> > 
> >> +
> >> +	printk("Going to call: %pF\n", gettimeofday);
> >> +	printk("Going to call: %pF\n", p->func);
> >> +	printk("%s: called from %pS\n", __func__, _RET_IP_);
> >> +	printk("%s: called from %pS\n", __func__, __builtin_return_address(0));
> >> +	printk("Faulted at %pS\n", (void *)regs->ip);
> >> +	printk(" %s%pB\n", reliable ? "" : "? ", (void *)*stack);
> > 
> > Much better!
> 
> Petr, that's really much better.
> 
> I've already pushed the part which restructured the text upstream.

Ah, great, I have missed this.

> I'll push this part of the patch (which adds the examples) for v4.14.

Sounds good.

Thanks,
Petr

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Sergey Senozhatsky]

On (08/23/17 16:48), Petr Mladek wrote:
[..]
> Sometimes people seems unclear when to use the %pS or %pF printk format.
> For example, see commit 51d96dc2e2dc ("random: fix warning message on ia64
> and parisc") which fixed such a wrong format string.
> 
> The documentation should be more clear about the difference.
> Also examples might help to avoid some typical mistakes.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> [pmladek@suse.com: Restructure the entire section]
> Signed-off-by: Petr Mladek <pmladek@suse.com>

looks great.

FWIW,
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Steven Rostedt]

On Thu, 24 Aug 2017 09:41:03 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> On (08/23/17 16:48), Petr Mladek wrote:
> [..]
> > Sometimes people seems unclear when to use the %pS or %pF printk format.
> > For example, see commit 51d96dc2e2dc ("random: fix warning message on ia64
> > and parisc") which fixed such a wrong format string.
> > 
> > The documentation should be more clear about the difference.
> > Also examples might help to avoid some typical mistakes.
> > 
> > Signed-off-by: Helge Deller <deller@gmx.de>
> > [pmladek@suse.com: Restructure the entire section]
> > Signed-off-by: Petr Mladek <pmladek@suse.com>  
> 
> looks great.
> 
> FWIW,
> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Also,

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Sergey Senozhatsky]

Hello,

sorry for the delay.

On (08/15/17 13:36), Petr Mladek wrote:
[..]
> > Experts surely do know what function descriptors are.
> > Nevertheless even those often get it wrong as can be seen in
> > various commits.
> 
> It seems that these specifiers are used the wrong way on many
> locations. They might be worth fixing but I cannot test it
> easily.
> 
> Hmm, using %pF might actually cause a crash when used
> on direct function address.

:(

> > The hope with this patch is to show widely-used examples
> > and avoid additional commits afterwards to fix it up.
> 
> IMHO, one problem is that the meaning of ''F'' and ''f''
> is hidden at the end of the section. Also the first line
> 
>   'For printing symbols and function pointers. The ``S`` and ``s`` '
> 
> kind of invites to use ``S`` and ``s`` even for function pointers.
> I suggest to switch the order, slightly retranslate, add the
> examples, see below.

agree. that was my problem: I saw the examples, didn't quite
understand anything and had to read that documentation section
anyway.

> > This patch was meant to be RFC.
> > If you decide not to take it, I'm fine as well.
> > 
> > > *may be* on "ia64, ppc64 and parisc64" we can somehow check
> > > that the pointer, which we pass as %pS, belongs to .text and
> > > print some build-time warnings. well, if it's actually a
> > > problem. dunno.
> 
> I think that it would need to be a runtime check because many/most
> printed addresses are not statically defined.

yep, can do.

> > I think it's not needed. Those bugs will be seen and fixed.
> 
> I am not sure how many people are familiar with this problem.
> I might help to avoid some headaches when debugging.
> 
> If we add the warning, it should be ratelimited to reduce messing
> of the original message.

sure.

[..]
> From ef983c65095cada994c1fe531e2b98e936c943bf Mon Sep 17 00:00:00 2001
> From: Helge Deller <deller@gmx.de>
> Date: Tue, 15 Aug 2017 11:34:19 +0200
> Subject: [PATCH] printk-formats.txt: Better describe the difference between
>  %pS and %pF
> 
> Sometimes people seems unclear when to use the %pS or %pF printk format.
> For example, see commit 51d96dc2e2dc ("random: fix warning message on ia64
> and parisc") which fixed such a wrong format string.
> 
> The documentation should be more clear about the difference.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> [pmladek@suse.com: Restructure the entire section]
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

> ---
>  Documentation/printk-formats.txt | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 65ea5915178b..074670b98bac 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -58,20 +58,23 @@ Symbols/Function Pointers
>  	%ps	versatile_init
>  	%pB	prev_fn_of_versatile_init+0x88/0x88
>  
> -For printing symbols and function pointers. The ``S`` and ``s`` specifiers
> -result in the symbol name with (``S``) or without (``s``) offsets. Where
> -this is used on a kernel without KALLSYMS - the symbol address is
> -printed instead.
> +The ``F`` and ``f`` specifiers are for printing function pointers,
> +for example, f->func, &gettimeofday. They have the same result as
> +``S`` and ``s`` specifiers. But they do an extra conversion on
> +ia64, ppc64 and parisc64 architectures where the function pointers
> +are actually function descriptors.
> +
> +The ``S`` and ``s`` specifiers can be used for printing symbols
> +from direct addresses, for example, __builtin_return_address(0),
> +(void *)regs->ip. They result in the symbol name with (``S``) or
> +without (``s``) offsets. If KALLSYMS are disabled then the symbol
> +address is printed instead.
>  
>  The ``B`` specifier results in the symbol name with offsets and should be
>  used when printing stack backtraces. The specifier takes into
>  consideration the effect of compiler optimisations which may occur
>  when tail-call``s are used and marked with the noreturn GCC attribute.
>  
> -On ia64, ppc64 and parisc64 architectures function pointers are
> -actually function descriptors which must first be resolved. The ``F`` and
> -``f`` specifiers perform this resolution and then provide the same
> -functionality as the ``S`` and ``s`` specifiers.
>  
>  Kernel Pointers
>  ===============

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Steven Rostedt]

On Thu, 10 Aug 2017 19:35:33 +0200
Helge Deller <deller@gmx.de> wrote:

> Sometimes people seems unclear when to use the %pS or %pF printk format.
> Adding some examples may help to avoid such mistakes.
> 
> See for example commit 51d96dc2e2dc ("random: fix warning message on ia64 and
> parisc") which fixed such a wrong format string.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 65ea591..be8c05b 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -73,6 +73,12 @@ actually function descriptors which must first be resolved. The ``F`` and
>  ``f`` specifiers perform this resolution and then provide the same
>  functionality as the ``S`` and ``s`` specifiers.
>  
> +Examples::
> +
> +	printk("Called from %pS.\n", __builtin_return_address(0));
> +	printk("Called from %pS.\n", (void *)regs->ip);
> +	printk("Called from %pF.\n", &gettimeofday);

Is the '&' really necessary? What about using the example:

	printk("Called in %pF.\n", __func__);

?

-- Steve

> +
>  Kernel Pointers
>  ===============
>  

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Helge Deller]

On 15.08.2017 14:46, Steven Rostedt wrote:
> On Thu, 10 Aug 2017 19:35:33 +0200
> Helge Deller <deller@gmx.de> wrote:
> 
>> Sometimes people seems unclear when to use the %pS or %pF printk format.
>> Adding some examples may help to avoid such mistakes.
>>
>> See for example commit 51d96dc2e2dc ("random: fix warning message on ia64 and
>> parisc") which fixed such a wrong format string.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>> index 65ea591..be8c05b 100644
>> --- a/Documentation/printk-formats.txt
>> +++ b/Documentation/printk-formats.txt
>> @@ -73,6 +73,12 @@ actually function descriptors which must first be resolved. The ``F`` and
>>  ``f`` specifiers perform this resolution and then provide the same
>>  functionality as the ``S`` and ``s`` specifiers.
>>  
>> +Examples::
>> +
>> +	printk("Called from %pS.\n", __builtin_return_address(0));
>> +	printk("Called from %pS.\n", (void *)regs->ip);
>> +	printk("Called from %pF.\n", &gettimeofday);
> 
> Is the '&' really necessary? 
The '&' is not necessary. The compiler doesn't complain either.

> What about using the example:
> 	printk("Called in %pF.\n", __func__);

Very interesting!

This code:
void smp_cpus_done() {
printk("Called from %pF.\n", smp_cpus_done);
printk("Called from %pf.\n", smp_cpus_done);
printk("Called in %pS.\n", __func__);
printk("Called in %ps.\n", __func__);
printk("Called in %pF.\n", __func__);
printk("Called in %pf.\n", __func__);

gives:
 Called from smp_cpus_done+0x0/0x1b8.
 Called from smp_cpus_done.
 Called in __func__.28197+0x0/0x20.
 Called in __func__.28197.
 Called in 0x5041524953433332.
 Called in 0x5041524953433332.

So, the correct usage is:
printk("Called in %pS.\n", __func__);

But it should have printed
 Called from smp_cpus_done+0x0/0x1b8.
which means the (parisc?) printk resolver doesn't work correctly.

In assembly code a pointer to this object is handed to printk:
        .type   __func__.28197, @object
        .size   __func__.28197, 14
__func__.28197:
        .stringz        "smp_cpus_done"

I'll look into this problem.

Helge

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Helge Deller]

On 15.08.2017 21:41, Helge Deller wrote:
> On 15.08.2017 14:46, Steven Rostedt wrote:
>> On Thu, 10 Aug 2017 19:35:33 +0200
>> Helge Deller <deller@gmx.de> wrote:
>>
>>> Sometimes people seems unclear when to use the %pS or %pF printk format.
>>> Adding some examples may help to avoid such mistakes.
>>>
>>> See for example commit 51d96dc2e2dc ("random: fix warning message on ia64 and
>>> parisc") which fixed such a wrong format string.
>>>
>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>
>>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>>> index 65ea591..be8c05b 100644
>>> --- a/Documentation/printk-formats.txt
>>> +++ b/Documentation/printk-formats.txt
>>> @@ -73,6 +73,12 @@ actually function descriptors which must first be resolved. The ``F`` and
>>>  ``f`` specifiers perform this resolution and then provide the same
>>>  functionality as the ``S`` and ``s`` specifiers.
>>>  
>>> +Examples::
>>> +
>>> +	printk("Called from %pS.\n", __builtin_return_address(0));
>>> +	printk("Called from %pS.\n", (void *)regs->ip);
>>> +	printk("Called from %pF.\n", &gettimeofday);
>>
>> Is the '&' really necessary? 
> The '&' is not necessary. The compiler doesn't complain either.
> 
>> What about using the example:
>> 	printk("Called in %pF.\n", __func__);
> 
> Very interesting!
> 
> This code:
> void smp_cpus_done() {
> printk("Called from %pF.\n", smp_cpus_done);
> printk("Called from %pf.\n", smp_cpus_done);
> printk("Called in %pS.\n", __func__);
> printk("Called in %ps.\n", __func__);
> printk("Called in %pF.\n", __func__);
> printk("Called in %pf.\n", __func__);
> 
> gives:
>  Called from smp_cpus_done+0x0/0x1b8.
>  Called from smp_cpus_done.
>  Called in __func__.28197+0x0/0x20.
>  Called in __func__.28197.
>  Called in 0x5041524953433332.
>  Called in 0x5041524953433332.
> 
> So, the correct usage is:
> printk("Called in %pS.\n", __func__);

I'm wrong.
The correct usage would be:
 printk("Called in %s.\n", __func__);

__func__ is just a pointer to a string.

Helge

> 
> But it should have printed
>  Called from smp_cpus_done+0x0/0x1b8.
> which means the (parisc?) printk resolver doesn't work correctly.
> 
> In assembly code a pointer to this object is handed to printk:
>         .type   __func__.28197, @object
>         .size   __func__.28197, 14
> __func__.28197:
>         .stringz        "smp_cpus_done"
> 
> I'll look into this problem.
> 
> Helge
> 

Re: [PATCH] printk-formats.txt: Add examples for %pS and %pF

[Steven Rostedt]

On Tue, 15 Aug 2017 21:47:27 +0200
Helge Deller <deller@gmx.de> wrote:

> > Very interesting!
> > 
> > This code:
> > void smp_cpus_done() {
> > printk("Called from %pF.\n", smp_cpus_done);
> > printk("Called from %pf.\n", smp_cpus_done);
> > printk("Called in %pS.\n", __func__);
> > printk("Called in %ps.\n", __func__);
> > printk("Called in %pF.\n", __func__);
> > printk("Called in %pf.\n", __func__);
> > 
> > gives:
> >  Called from smp_cpus_done+0x0/0x1b8.
> >  Called from smp_cpus_done.
> >  Called in __func__.28197+0x0/0x20.
> >  Called in __func__.28197.
> >  Called in 0x5041524953433332.
> >  Called in 0x5041524953433332.
> > 
> > So, the correct usage is:
> > printk("Called in %pS.\n", __func__);  
> 
> I'm wrong.
> The correct usage would be:
>  printk("Called in %s.\n", __func__);
> 
> __func__ is just a pointer to a string.

OK, I'm still on vacation :-/

Yeah, I was looking for usages of %pF, and came across this:

		pr_warn("%s: NULL omap_sr from %pF\n",
			__func__, (void *)_RET_IP_);

And not noticing the first "%s" :-p

-- Steve