[Crash-utility] [PATCH v2] struct: Fix handing of percpu symbols (when symbolic argument only)

[Dave Anderson]

Aaron,

First things first:

$ make warn
... [ cut ] ...
cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6  symbols.c -I./gdb-7.6/bfd -I./gdb-7.6/include -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security 
symbols.c: In function ‘cmd_datatype_common’:
symbols.c:6119:15: warning: unused variable ‘tmpfp’ [-Wunused-variable]
         FILE *tmpfp;
               ^
In file included from symbols.c:18:0:
defs.h:4819:28: warning: ‘typename’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 #define FREEBUF(X)  freebuf((char *)(X))
                            ^
symbols.c:6116:15: note: ‘typename’ was declared here
         char *typename;
               ^
...

Dave




----- Original Message -----
> Hi Dave,
> 
> Sorry about the delay. I completely forgot about this one!
> Please note I have only addressed the case when the argument to the
> 'struct' command is not an address:
> 
> When a percpu symbol is of type pointer, the 'struct' command does
> not generate the expected output. For example:
> 
> Note: The correct value of 'exec_start' should be
>       0x15b070b28b0c27 not 0x0
> 
>   crash> struct task_struct.se.exec_start softlockup_watchdog:0
>   [0]: ffff880214e55a00
>     se.exec_start = 0x0,
> 
>   crash> px softlockup_watchdog:0-1
>   per_cpu(softlockup_watchdog, 0) = $1 = (struct task_struct *)
>   0xffff880fe97e2e00
> 
>   crash> px ((struct task_struct *)0xffff880fe97e2e00)->se.exec_start
>   $2 = 0x15b070b28b0c27
> 
> Currently, the 'struct' and 'p' command simply calculates 'cpuaddr' as
> follows -- where 'addr' is a percpu's value:
> 
>         cpuaddr = addr + __per_cpu_offset
> 
> This is correct if the percpu symbol or offset
> (i.e. [percpu symbol:cpu-specifier] or [percpu value:cpu-specifier])
> is not of type pointer. If a given percpu symbol is of type pointer such as
> in 'static DEFINE_PER_CPU(struct task_struct *, softlockup_watchdog)', we
> need to dereference the pointer to obtain the above correct kernel virtual
> address.
> 
> For instance, using the above example, we need to pass the following to gdb
> to resolve appropriately:
> 
> kernel virtual addresses for the cpus specified.
> 
>   crash> set gdb on
>   gdb: on
> 
>   gdb> p *(struct task_struct **) 0xffff880fffc0ddc0
>   $1 = (struct task_struct *) 0xffff880fe97e2e00
>   gdb>
> 
> With this patch, we now obtain the expected output:
> 
>   crash> struct task_struct.se.exec_start softlockup_watchdog:0
>   [0]: ffff880214e55a00
>     se.exec_start = 0x15b070b28b0c27,
> 
> Signed-off-by: Aaron Tomlin <atomlin at redhat.com>
> ---
>  symbols.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/symbols.c b/symbols.c
> index 8b0d8b9..b68d0d3 100644
> --- a/symbols.c
> +++ b/symbols.c
> @@ -81,6 +81,7 @@ static void cmd_datatype_common(ulong);
>  static void do_datatype_addr(struct datatype_member *, ulong, int,
>  			     ulong, char **, int);
>  static void process_gdb_output(char *, unsigned, const char *, int);
> +static char *expr_type_name(const char *);
>  static int display_per_cpu_info(struct syment *, int, char *);
>  static struct load_module *get_module_percpu_sym_owner(struct syment *);
>  static int is_percpu_symbol(struct syment *);
> @@ -6112,6 +6113,11 @@ cmd_datatype_common(ulong flags)
>          char *separator;
>          char *structname, *members;
>          char *memberlist[MAXARGS];
> +        char *typename;
> +        char buf[BUFSIZE];
> +        char *argv[MAXARGS];
> +        FILE *tmpfp;
> +        ushort ptype;
>  
>          dm = &datatype_member;
>  	count = 0xdeadbeef;
> @@ -6122,6 +6128,7 @@ cmd_datatype_common(ulong flags)
>  	separator = members = NULL;
>  	cpuspec = NULL;
>  	cpus = NULL;
> +	ptype = 0;
>  
>          while ((c = getopt(argcnt, args, "pxdhfuc:rvol:")) != EOF) {
>                  switch (c)
> @@ -6262,6 +6269,12 @@ cmd_datatype_common(ulong flags)
>  			SET_BIT(cpus, CURRENT_CONTEXT()->processor);
>  		else
>  			make_cpumask(cpuspec, cpus, FAULT_ON_ERROR, NULL);
> +
> +		if (sp) {
> +			typename = expr_type_name(sp->name);
> +			ptype = typename &&
> +				LASTCHAR(typename) == '*' ? 1 : 0;
> +		}
>  	}
>  
>  	optind = optind_save;
> @@ -6346,6 +6359,19 @@ cmd_datatype_common(ulong flags)
>  				continue;
>  			}
>  
> +			if (ptype) {
> +				open_tmpfile();
> +				snprintf(buf, sizeof buf, "p *(%s*) 0x%lx",
> +						typename, cpuaddr);
> +				gdb_pass_through(buf, pc->tmpfile, GNU_RETURN_ON_ERROR);
> +
> +				rewind(pc->tmpfile);
> +				fgets(buf, BUFSIZE, pc->tmpfile);
> +				parse_line(buf, argv);
> +				cpuaddr = htol(argv[3], FAULT_ON_ERROR, NULL);
> +				close_tmpfile();
> +			}
> +
>  			fprintf(fp, "%lx\n", cpuaddr);
>  			do_datatype_addr(dm, cpuaddr , count,
>  					 flags, memberlist, argc_members);
> @@ -6364,6 +6390,9 @@ freebuf:
>  
>  	if (cpus)
>  		FREEBUF(cpus);
> +
> +	if (typename)
> +		FREEBUF(typename);
>  }
>  
>  static void
> --
> 2.5.5
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>