Re: [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps

Re: [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps

[Petr Mladek]

On Mon 2019-09-02 11:32:36, Sakari Ailus wrote:
> %pS and %ps are now the preferred conversion specifiers to print function
> names. The functionality is equivalent; remove the old, deprecated %pF
> and %pf support.

Hmm, I see the following in master:

$> git grep %pF
tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt:or events have "%pF" or "%pS" parameter in its format string. It is common to

$> git grep %pf
tools/lib/traceevent/event-parse.c:             if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0)
tools/lib/traceevent/event-parse.c:     if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0)

I wonder how this is related to printk(). In each case, it seems
that libtraceevent somehow implements the non-standard kernel
%p mofifiers. It looks error-prone to keep another %pf user
with the old semantic around.

I am adding some tracing people into CC.

Best Regards,
Petr

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/core-api/printk-formats.rst | 10 ----------
>  lib/vsprintf.c                            |  8 ++------
>  scripts/checkpatch.pl                     |  1 -
>  3 files changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index c6224d039bcbe..922a29eb70e6c 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -86,8 +86,6 @@ Symbols/Function Pointers
>  
>  	%pS	versatile_init+0x0/0x110
>  	%ps	versatile_init
> -	%pF	versatile_init+0x0/0x110
> -	%pf	versatile_init
>  	%pSR	versatile_init+0x9/0x110
>  		(with __builtin_extract_return_addr() translation)
>  	%pB	prev_fn_of_versatile_init+0x88/0x88
> @@ -97,14 +95,6 @@ The ``S`` and ``s`` specifiers are used for printing a pointer in symbolic
>  format. They result in the symbol name with (S) or without (s)
>  offsets. If KALLSYMS are disabled then the symbol address is printed instead.
>  
> -Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``)
> -and thus deprecated. We have ``F`` and ``f`` because on ia64, ppc64 and
> -parisc64 function pointers are indirect and, in fact, are function
> -descriptors, which require additional dereferencing before we can lookup
> -the symbol. As of now, ``S`` and ``s`` perform dereferencing on those
> -platforms (when needed), so ``F`` and ``f`` exist for compatibility
> -reasons only.
> -
>  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
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b0967cf17137d..b00b57f9f911f 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -909,7 +909,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
>  #ifdef CONFIG_KALLSYMS
>  	if (*fmt == 'B')
>  		sprint_backtrace(sym, value);
> -	else if (*fmt != 'f' && *fmt != 's')
> +	else if (*fmt != 's')
>  		sprint_symbol(sym, value);
>  	else
>  		sprint_symbol_no_offset(sym, value);
> @@ -2007,9 +2007,7 @@ static char *kobject_string(char *buf, char *end, void *ptr,
>   *
>   * - 'S' For symbolic direct pointers (or function descriptors) with offset
>   * - 's' For symbolic direct pointers (or function descriptors) without offset
> - * - 'F' Same as 'S'
> - * - 'f' Same as 's'
> - * - '[FfSs]R' as above with __builtin_extract_return_addr() translation
> + * - '[Ss]R' as above with __builtin_extract_return_addr() translation
>   * - 'B' For backtraced symbolic direct pointers with offset
>   * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
>   * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
> @@ -2112,8 +2110,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	      struct printf_spec spec)
>  {
>  	switch (*fmt) {
> -	case 'F':
> -	case 'f':
>  	case 'S':
>  	case 's':
>  		ptr = dereference_symbol_descriptor(ptr);
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 93a7edfe0f059..a60c241112cd4 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6012,7 +6012,6 @@ sub process {
>  					my $ext_type = "Invalid";
>  					my $use = "";
>  					if ($bad_specifier =~ /p[Ff]/) {
> -						$ext_type = "Deprecated";
>  						$use = " - use %pS instead";
>  						$use =~ s/pS/ps/ if ($bad_specifier =~ /pf/);
>  					}
> -- 
> 2.20.1
> 

Re: [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps

[Andy Shevchenko]

On Mon, Sep 02, 2019 at 04:39:35PM +0200, Petr Mladek wrote:
> On Mon 2019-09-02 11:32:36, Sakari Ailus wrote:
> > %pS and %ps are now the preferred conversion specifiers to print function
> > names. The functionality is equivalent; remove the old, deprecated %pF
> > and %pf support.
> 
> Hmm, I see the following in master:
> 
> $> git grep %pF
> tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt:or events have "%pF" or "%pS" parameter in its format string. It is common to
> 
> $> git grep %pf
> tools/lib/traceevent/event-parse.c:             if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0)
> tools/lib/traceevent/event-parse.c:     if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0)
> 
> I wonder how this is related to printk(). In each case, it seems

It's going thru binary printf() I suppose. The fist stage just saves the format
string and argument addresses or so and prints in later on when user is looking
for human-readable output.

> that libtraceevent somehow implements the non-standard kernel
> %p mofifiers. It looks error-prone to keep another %pf user
> with the old semantic around.
> 
> I am adding some tracing people into CC.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps

[Petr Mladek]

On Mon 2019-09-02 19:01:39, Andy Shevchenko wrote:
> On Mon, Sep 02, 2019 at 04:39:35PM +0200, Petr Mladek wrote:
> > On Mon 2019-09-02 11:32:36, Sakari Ailus wrote:
> > > %pS and %ps are now the preferred conversion specifiers to print function
> > > names. The functionality is equivalent; remove the old, deprecated %pF
> > > and %pf support.
> > 
> > Hmm, I see the following in master:
> > 
> > $> git grep %pF
> > tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt:or events have "%pF" or "%pS" parameter in its format string. It is common to
> > 
> > $> git grep %pf
> > tools/lib/traceevent/event-parse.c:             if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0)
> > tools/lib/traceevent/event-parse.c:     if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0)
> > 
> > I wonder how this is related to printk(). In each case, it seems
> 
> It's going thru binary printf() I suppose. The fist stage just saves the format
> string and argument addresses or so and prints in later on when user is looking
> for human-readable output.

It seems that vbin_printf() still thinks that %pf and %pF
handle function pointers. If I get it correctly, it just
stores the binary data and the formating is done when
tracing log is read. The idea is the function pointers
will stay the same.

We need to fix/obsolete this path as well.

Best Regards,
Petr

Re: [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps

[Sakari Ailus]

On Tue, Sep 03, 2019 at 04:04:20PM +0200, Petr Mladek wrote:
> On Mon 2019-09-02 19:01:39, Andy Shevchenko wrote:
> > On Mon, Sep 02, 2019 at 04:39:35PM +0200, Petr Mladek wrote:
> > > On Mon 2019-09-02 11:32:36, Sakari Ailus wrote:
> > > > %pS and %ps are now the preferred conversion specifiers to print function
> > > > names. The functionality is equivalent; remove the old, deprecated %pF
> > > > and %pf support.
> > > 
> > > Hmm, I see the following in master:
> > > 
> > > $> git grep %pF
> > > tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt:or events have "%pF" or "%pS" parameter in its format string. It is common to
> > > 
> > > $> git grep %pf
> > > tools/lib/traceevent/event-parse.c:             if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0)
> > > tools/lib/traceevent/event-parse.c:     if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0)
> > > 
> > > I wonder how this is related to printk(). In each case, it seems
> > 
> > It's going thru binary printf() I suppose. The fist stage just saves the format
> > string and argument addresses or so and prints in later on when user is looking
> > for human-readable output.
> 
> It seems that vbin_printf() still thinks that %pf and %pF
> handle function pointers. If I get it correctly, it just
> stores the binary data and the formating is done when
> tracing log is read. The idea is the function pointers
> will stay the same.
> 
> We need to fix/obsolete this path as well.

Agreed. I'll include a patch to do that in v6.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com