Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
       [not found] ` <20190902083240.20367-8-sakari.ailus@linux.intel.com>
@ 2019-09-02 14:39   ` Petr Mladek
  2019-09-02 16:01     ` Andy Shevchenko
  0 siblings, 1 reply; 4+ messages in thread
From: Petr Mladek @ 2019-09-02 14:39 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: rafael, linux-kernel, Rob Herring, Andy Shevchenko,
	Heikki Krogerus, devicetree, linux-acpi, Steven Rostedt,
	Arnaldo Carvalho de Melo, Tzvetomir Stoyanov, linux-trace-devel,
	Jiri Olsa, Namhyung Kim

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
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
  2019-09-02 14:39   ` [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps Petr Mladek
@ 2019-09-02 16:01     ` Andy Shevchenko
  2019-09-03 14:04       ` Petr Mladek
  0 siblings, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2019-09-02 16:01 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Sakari Ailus, rafael, linux-kernel, Rob Herring, Heikki Krogerus,
	devicetree, linux-acpi, Steven Rostedt, Arnaldo Carvalho de Melo,
	Tzvetomir Stoyanov, linux-trace-devel, Jiri Olsa, Namhyung Kim

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
  2019-09-02 16:01     ` Andy Shevchenko
@ 2019-09-03 14:04       ` Petr Mladek
  2019-09-06  6:59         ` Sakari Ailus
  0 siblings, 1 reply; 4+ messages in thread
From: Petr Mladek @ 2019-09-03 14:04 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Steven Rostedt, Namhyung Kim, rafael, Rob Herring,
	Heikki Krogerus, Sakari Ailus, Arnaldo Carvalho de Melo,
	Jiri Olsa, devicetree, linux-acpi, linux-kernel,
	linux-trace-devel, Tzvetomir Stoyanov

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
  2019-09-03 14:04       ` Petr Mladek
@ 2019-09-06  6:59         ` Sakari Ailus
  0 siblings, 0 replies; 4+ messages in thread
From: Sakari Ailus @ 2019-09-06  6:59 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Andy Shevchenko, Steven Rostedt, Namhyung Kim, rafael,
	Rob Herring, Heikki Krogerus, Arnaldo Carvalho de Melo,
	Jiri Olsa, devicetree, linux-acpi, linux-kernel,
	linux-trace-devel, Tzvetomir Stoyanov

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190902083240.20367-1-sakari.ailus@linux.intel.com>
     [not found] ` <20190902083240.20367-8-sakari.ailus@linux.intel.com>
2019-09-02 14:39   ` [PATCH v4 07/11] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps Petr Mladek
2019-09-02 16:01     ` Andy Shevchenko
2019-09-03 14:04       ` Petr Mladek
2019-09-06  6:59         ` Sakari Ailus

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git