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

[Dave Anderson]

----- Original Message -----
> On Thu 2016-09-29 12:37 -0400, Dave Anderson wrote:
> [ ... ]
> > Given that you are essentially specifying a completely new argument
> > type to the struct command, I'm going to have to NAK this patch.
> 
> That's OK.
> 
> > To do what you want, honestly I don't really feel that it's asking too much
> > to make it a two-step process, i.e, get the address first, and then apply
> > it
> > to the struct command.
> 
> Agreed.
> 
> > However, if you really feel it's worth it, I suppose you could create a
> > new "indirect-pointer-argument-type" for the struct command that explicitly
> > specifies that it is a pointer to the target address.
> 
> No, it's not worth it actually. Perhaps a warning instead?
> For e.g. something like this:
> 
> Subject: [PATCH] struct: warn if percpu symbol is of type pointer
> 
> ---
>  symbols.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/symbols.c b/symbols.c
> index 8b0d8b9..6447fad 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,7 @@ cmd_datatype_common(ulong flags)
>          char *separator;
>          char *structname, *members;
>          char *memberlist[MAXARGS];
> +        char *typename;
>  
>          dm = &datatype_member;
>  	count = 0xdeadbeef;
> @@ -6122,6 +6124,7 @@ cmd_datatype_common(ulong flags)
>  	separator = members = NULL;
>  	cpuspec = NULL;
>  	cpus = NULL;
> +	typename = NULL;
>  
>          while ((c = getopt(argcnt, args, "pxdhfuc:rvol:")) != EOF) {
>                  switch (c)
> @@ -6245,6 +6248,12 @@ cmd_datatype_common(ulong flags)
>  				      sp->name);
>  				cpuspec = NULL;
>  			}
> +			typename = expr_type_name(sp->name);
> +			if (cpuspec && typename &&
> +				LASTCHAR(typename) == '*' ? 1 : 0)
> +				error(WARNING,
> +				      "percpu symbol %s is of type pointer.\n",
> +				      sp->name);
>  	                addr = sp->value;
>  			aflag++;
>  	        } else {
> @@ -6364,6 +6373,9 @@ freebuf:
>  
>  	if (cpus)
>  		FREEBUF(cpus);
> +
> +	if (typename)
> +		FREEBUF(typename);
>  }
>  
>  static void


Fair enough -- but let's avoid the heavy-handed call to expr_type_name()
unless the argument is actually a cpuspec:

--- crash-7.1.5/symbols.c.orig	2016-09-30 09:21:15.577896529 -0400
+++ crash-7.1.5/symbols.c	2016-09-30 09:16:53.676652458 -0400
@@ -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,7 @@ cmd_datatype_common(ulong flags)
         char *separator;
         char *structname, *members;
         char *memberlist[MAXARGS];
+        char *typename;
 
         dm = &datatype_member;
 	count = 0xdeadbeef;
@@ -6245,6 +6247,15 @@ cmd_datatype_common(ulong flags)
 				      sp->name);
 				cpuspec = NULL;
 			}
+			if (cpuspec) {
+				if ((typename = expr_type_name(sp->name))) {
+				    	if (LASTCHAR(typename) == '*')
+						error(WARNING,
+						    "percpu symbol %s is of type pointer\n",
+							sp->name);
+					FREEBUF(typename);
+				}
+			}
 	                addr = sp->value;
 			aflag++;
 	        } else {
 

Dave