Hi Dave, while I was working on your suggestions, I faced the following problem: (gdb) call datatype_info("page", "mapping", -3) $1 = 0 It happens because entire anonymous structure is represented as single field with empty name within gdb. Please, consider the patch which fixes such behavior: (gdb) call datatype_info("page", "mapping", -3) $1 = 1 (gdb) call datatype_info("page", "index", -3) $5 = 8 (gdb) call datatype_info("page", "lru", -3) $6 = 3 (gdb) call datatype_info("page", "objects", -3) $7 = 22 diff --git a/gdb-7.6.patch b/gdb-7.6.patch index 794555f..8a61f20 100644 --- a/gdb-7.6.patch +++ b/gdb-7.6.patch @@ -351,7 +351,7 @@ return lookup_symbol_in_language (name, block, domain, current_language->la_language, is_a_field_of_this); -@@ -5100,3 +5115,662 @@ When enabled, debugging messages are pri +@@ -5100,3 +5115,666 @@ When enabled, debugging messages are pri observer_attach_executable_changed (symtab_observer_executable_changed); } @@ -736,6 +736,10 @@ + (typedef_type = check_typedef(nextfield->type))) + req->member_length = TYPE_LENGTH(typedef_type); + return; ++ } else if (*nextfield->name == 0) { /* Anonymous struct/union */ ++ get_member_data(req, nextfield->type); ++ if (req->member_offset != -1) ++ return; + } + nextfield++; + } ______________________________________ From: crash-utility-bounces(a)redhat.com <crash-utility-bounces(a)redhat.com&gt; on behalf of Dave Anderson <anderson(a)redhat.com&gt; Sent: Thursday, August 18, 2016 23:39 To: Discussion list for crash utility usage, maintenance and development Subject: Re: [Crash-utility] Speed up list/tree '-s' output. Another suggestion: Use MEMBER_TYPE(), or pass a datatype_member structure to datatype_info(), in order to determine whether a structure member is TYPE_CODE_PTR, and if so, always print the member in hexadecimal regardless of the current radix. (i.e. like gdb does) Dave

----- Original Message ----- Alexandr, Can you make your symtab.c patches a discrete patchset that gets appended to gdb-7.6.patch? It needs to be that way in order to get recognized and applied correctly to a pre-existing source tree. Thanks, Dave

Hi Dave, here is updated patch for gdb/symtab.c: --- gdb-7.6.orig/gdb/symtab.c 2016-08-18 09:06:33.000000000 +0300 +++ gdb-7.6/gdb/symtab.c 2016-08-23 14:28:45.456510348 +0300 @@ -5486,13 +5486,15 @@ struct field *nextfield; short nfields; struct type *typedef_type; + static int deep = 0; + static long offset = 0; req->member_offset = -1; nfields = TYPE_MAIN_TYPE(type)->nfields; nextfield = TYPE_MAIN_TYPE(type)->flds_bnds.fields; - if (nfields == 0) { + if (nfields == 0 && deep == 0 /* The first call */) { struct type *newtype; newtype = lookup_transparent_type(req->name); if (newtype) { @@ -5505,13 +5507,21 @@ for (i = 0; i < nfields; i++) { if (STREQ(req->member, nextfield->name)) { - req->member_offset = nextfield->loc.bitpos; + req->member_offset = offset + nextfield->loc.bitpos; req->member_length = TYPE_LENGTH(nextfield->type); req->member_typecode = TYPE_CODE(nextfield->type); if ((req->member_typecode == TYPE_CODE_TYPEDEF) && (typedef_type = check_typedef(nextfield->type))) req->member_length = TYPE_LENGTH(typedef_type); return; + } else if (*nextfield->name == 0) { /* Anonymous struct/union */ + deep++; + offset += nextfield->loc.bitpos; + get_member_data(req, nextfield->type); + offset -= nextfield->loc.bitpos; + deep--; + if (req->member_offset != -1) + return; } nextfield++; } @@ -5705,7 +5715,7 @@ req->target_length = target_type->length; } - if (req->member) + if (req->member) get_member_data(req, type); do_cleanups (old_chain); The patch for crash tool can be found on github: https://raw.githubusercontent.com/hziSot/crash-patches/41abc6f9c91c2a6029... Thanks, Alexandr

Hi Dave, here is patch with arguments instead of static variables: --- gdb-7.6.orig/gdb/symtab.c 2016-08-18 09:06:33.000000000 +0300 +++ gdb-7.6/gdb/symtab.c 2016-08-24 08:40:45.700772046 +0300 @@ -5122,7 +5122,7 @@ #define GDB_COMMON #include "../../defs.h" -static void get_member_data(struct gnu_request *, struct type *); +static void get_member_data(struct gnu_request *, struct type *, long, char); static void dump_enum(struct type *, struct gnu_request *); static void eval_enum(struct type *, struct gnu_request *); static void gdb_get_line_number(struct gnu_request *); @@ -5327,7 +5327,7 @@ req->typecode = TYPE_CODE(sym->type); req->length = TYPE_LENGTH(sym->type); if (req->member) - get_member_data(req, sym->type); + get_member_data(req, sym->type, 0, 1); if (TYPE_CODE(sym->type) == TYPE_CODE_ENUM) { if (req->flags & GNU_PRINT_ENUMERATORS) @@ -5397,7 +5397,7 @@ } if (req->member) - get_member_data(req, type); + get_member_data(req, type, 0, 1); break; @@ -5480,7 +5480,7 @@ * member field, and when found, return its relevant data. */ static void -get_member_data(struct gnu_request *req, struct type *type) +get_member_data(struct gnu_request *req, struct type *type, long offset, char is_first) { register short i; struct field *nextfield; @@ -5492,7 +5492,7 @@ nfields = TYPE_MAIN_TYPE(type)->nfields; nextfield = TYPE_MAIN_TYPE(type)->flds_bnds.fields; - if (nfields == 0) { + if (nfields == 0 && is_first /* The first call */) { struct type *newtype; newtype = lookup_transparent_type(req->name); if (newtype) { @@ -5505,13 +5505,18 @@ for (i = 0; i < nfields; i++) { if (STREQ(req->member, nextfield->name)) { - req->member_offset = nextfield->loc.bitpos; + req->member_offset = offset + nextfield->loc.bitpos; req->member_length = TYPE_LENGTH(nextfield->type); req->member_typecode = TYPE_CODE(nextfield->type); if ((req->member_typecode == TYPE_CODE_TYPEDEF) && (typedef_type = check_typedef(nextfield->type))) req->member_length = TYPE_LENGTH(typedef_type); return; + } else if (*nextfield->name == 0) { /* Anonymous struct/union */ + get_member_data(req, nextfield->type, + offset + nextfield->loc.bitpos, 0); + if (req->member_offset != -1) + return; } nextfield++; } @@ -5706,7 +5711,7 @@ } if (req->member) - get_member_data(req, type); + get_member_data(req, type, 0, 1); do_cleanups (old_chain); } Thanks, Alexandr ________________________________________ From: crash-utility-bounces(a)redhat.com <crash-utility-bounces(a)redhat.com&gt; on behalf of Dave Anderson <anderson(a)redhat.com&gt; Sent: Tuesday, August 23, 2016 23:17 To: Discussion list for crash utility usage, maintenance and development Subject: Re: [Crash-utility] Speed up list/tree '-s' output. Hi Alexandr, I'm still testing and sanity-checking the symtab.c patch given that it's modifying such a commonly-called function. Interestingly enough, it's actually uncovered a bug with the current determination of the page.list members, which I found by saving the output of "help -o" into a file, both without and then with your patch, into /tmp/old and /tmp/new, and then diff'ed the two files: # diff /tmp/old /tmp/new 221,223c221,223 < page_list: -1 < page_list_next: -1 < page_list_prev: -1 --- # Fortunately those fields aren't used by the crash code for more recent kernels, which have gone crazy with the anonymous structures in the page structure. But anyway, that was a good catch by your patch. On the other hand, I don't like the use of the deep and offset static variables, because there's always the remote possibility that a CTRL-C or other function failure may happen while they are set, and then leave them in a bogus state. So I would rather they get passed as arguments. Thanks, Dave ----- Original Message ----- -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility

Alexandr, The symtab.c patch has been queued for crash-7.1.6: https://github.com/crash-utility/crash/commit/5e6fbde738b827e2575aa9ec9ba... Nice fix! Thanks, Dave ----- Original Message -----

----- Original Message ----- Hi Alexandr, Now, aside from the functionality issues in my previous email, let's move on to the implemention... I'd like to clarify and simplify the patch a bit -- my comments are interleaved below. diff --git a/defs.h b/defs.h index d6691a3..c2c76c2 100644 --- a/defs.h +++ b/defs.h @@ -2380,6 +2380,12 @@ struct datatype_member { /* minimal definition of a structure/union */ }; #define union_name struct_name +struct req_entry { + char *arg, *name; + char **member, *width, *is_ptr; + short *offset; + unsigned char count; +}; This req_entry data structure is confusing as hell, primarily because things like the is_ptr, width and offset fields seem to be mis-typed. I understand that they are pointers to arrays, but why not make them pointers to their real types instead of "char *" and "short *"? By that I mean is_ptr is a long returned by MEMBER_TYPE(), width is a long returned by ANON_MEMBER_OFFSET(), and offset is a long returned by MEMBER_OFFSET(). I presume you made them "char *" because they use GETBUF(), but typically the users of GETBUF() will cast the return value from GETBUF() as a pointer to what it really points to. Now, I also understand that you may feel that it would be a waste of allocated space, but keep in mind that GETBUF() parses out the pre-allocated buffers first, so while you may think you're getting just a few bytes, you're really getting 1K or 2K buffers. So it really makes more sense to type-cast them as what they really are. Now, with respect to how the req_entry structure is appended to the list_data and tree_data structures: struct list_data { /* generic structure used by do_list() to walk */ ulong flags; /* through linked lists in the kernel */ @@ -2395,6 +2401,8 @@ struct list_data { /* generic structure used by do_list() to walk */ int (*callback_func)(void *, void *); void *callback_data; long struct_list_offset; + int entries; + struct req_entry **e; }; #define LIST_OFFSET_ENTERED (VERBOSE << 1) #define LIST_START_ENTERED (VERBOSE << 2) @@ -2416,6 +2424,8 @@ struct tree_data { char **structname; int structname_args; int count; + int entries; + struct req_entry **e; }; The new "entries" and "e" pointers are really just duplications of the existing "structname", "structname_args" and "count" members. In the do_list() and do_tree() functions, there's no reason you couldn't continue to use those existing fields. Furthermore, I see no reason that you have to export the req_entry structure in defs.h. It's really an implementation-only structure that is used internally in tools.c. Please declare it there only. diff --git a/tools.c b/tools.c index 2c3aab9..1b5420e 100644 --- a/tools.c +++ b/tools.c @@ -27,6 +27,9 @@ static void dump_struct_members(struct list_data *, int, ulong); static void rbtree_iteration(ulong, struct tree_data *, char *); static void rdtree_iteration(ulong, struct tree_data *, char *, ulong, uint); static void dump_struct_members_for_tree(struct tree_data *, int, ulong); +static void print_value(char *, ulong, short, unsigned int); +static struct req_entry *fill_member_offsets(char *); +static void dump_struct_members_fast(struct req_entry *, int, ulong); /* * General purpose error reporting routine. Type INFO prints the message @@ -3222,6 +3225,7 @@ cmd_list(void) struct list_data list_data, *ld; struct datatype_member struct_member, *sm; struct syment *sp; + struct req_entry *re; ulong value, struct_list_offset; sm = &struct_member; @@ -3229,7 +3233,7 @@ cmd_list(void) BZERO(ld, sizeof(struct list_data)); struct_list_offset = 0; - while ((c = getopt(argcnt, args, "Hhrs:e:o:xdl:")) != EOF) { + while ((c = getopt(argcnt, args, "Hhrs:S:e:o:xdl:")) != EOF) { switch(c) { case 'H': @@ -3246,11 +3250,18 @@ cmd_list(void) break; case 's': - if (ld->structname_args++ == 0) + if (ld->structname_args++ == 0) hq_open(); hq_enter((ulong)optarg); break; + case 'S': + re = fill_member_offsets(optarg); + if (ld->entries++ == 0) + hq_open(); + hq_enter((ulong)re); + break; + The 's' and 'S' case handlers above could be almost identical. Instead of stashing the "re" pointers, save the optarg variables. For mutual exclusion, do what the other crash commands do, and create local "sflag" and "Sflag" variables. To differentiate the -s and -S option later when they are acted upon, create another LIST_XXXX flag and set it above in ld->flags. The req_entry allocation(s) and fill_member_offsets() call(s) can be done later. case 'l': if (IS_A_NUMBER(optarg)) struct_list_offset = stol(optarg, @@ -3313,13 +3324,24 @@ cmd_list(void) cmd_usage(pc->curcmd, SYNOPSIS); } + if (ld->structname_args && ld->entries) { + error(INFO, "-S and -s options are mutually exclusive"); + cmd_usage(pc->curcmd, SYNOPSIS); + } + if (ld->structname_args) { ld->structname = (char **)GETBUF(sizeof(char *) * ld->structname_args); retrieve_list((ulong *)ld->structname, ld->structname_args); hq_close(); ld->struct_list_offset = struct_list_offset; + } else if (ld->entries) { + ld->e = (struct req_entry **)GETBUF(sizeof (struct req_entry *) * + ld->entries); + retrieve_list((ulong *)ld->e, ld->entries); + hq_close(); + ld->struct_list_offset = struct_list_offset; Again, the "if (ld->entries)" section above could be removed entirely. } else if (struct_list_offset) { - error(INFO, "-l option can only be used with -s option\n"); + error(INFO, "-l option can only be used with -s or -S option\n"); cmd_usage(pc->curcmd, SYNOPSIS); } @@ -3483,6 +3505,102 @@ next_arg: FREEBUF(ld->structname); } +void +dump_struct_members_fast(struct req_entry *e, int radix, ulong p) +{ + unsigned int i; + char b[BUFSIZE]; + if (!IS_KVADDR(p)) + return; + for (i = 0; i < e->count; i++) { + if (e->width[i] == 0 || e->width[i] > 8) { + snprintf(b, BUFSIZE, "%s.%s", e->name, e->member[i]); + dump_struct_member(b, p, radix); + } else { + print_value(e->member[i], p + e->offset[i], e->width[i], + e->is_ptr[i] ? 16 : radix); + } + } +} + +static struct req_entry * +fill_member_offsets(char *arg) +{ + int j; + char *p, m; + struct req_entry *e; + char b[BUFSIZE]; + + if (!(arg && *arg)) + return NULL; + + j = count_chars(arg, ',') + 1; + e = (struct req_entry *)GETBUF(sizeof(*e)); + + e->arg = GETBUF(strlen(arg + 1)); + strcpy(e->arg, arg); + + m = ((p = strchr(e->arg, '.')) != NULL); + if (!p++) + p = e->arg + strlen(e->arg) + 1; + + e->name = GETBUF(p - e->arg); + strncpy(e->name, e->arg, p - e->arg - 1); + + if (!m) + return e; + + e->count = count_chars(p, ',') + 1; + e->width = GETBUF(e->count); + e->is_ptr = GETBUF(e->count); + e->member = (char **)GETBUF(e->count * sizeof(char *)); + e->offset = (short *)GETBUF(e->count * sizeof(short)); + + replace_string(p, ",", ' '); + parse_line(p, e->member); + + for (j = 0; j < e->count; j++) { + e->offset[j] = MEMBER_OFFSET(e->name, e->member[j]); + if (e->offset[j] == -1) + e->offset[j] = ANON_MEMBER_OFFSET(e->name, e->member[j]); + if (e->offset[j] == -1) + break; + + e->is_ptr[j] = MEMBER_TYPE(e->name, e->member[j]) == TYPE_CODE_PTR; + // Dirty hack for obtaining size of particular field + snprintf(b, BUFSIZE, "%s + 1", e->member[j]); + e->width[j] = ANON_MEMBER_OFFSET(e->name, b) - e->offset[j]; + } + + return e; +} + +static void +print_value(char *name, ulong addr, short width, unsigned int radix) +{ + union { uint64_t v64; uint32_t v32; + uint16_t v16; uint8_t v8; + } v; + char fmt[BUFSIZE]; + + if (!readmem(addr, KVADDR, &v, width, + "structure value", RETURN_ON_ERROR | QUIET)) { + error(INFO, "cannot access field: %s at %lx\n", name, addr); + return; + } + snprintf(fmt, BUFSIZE, " %%s = %s%%%s%s\n", + (radix == 16 ? "0x" : ""), + (width == 8 ? "l" : ""), + (radix == 16 ? "x" : "u" ) + ); + + switch (width) { + case 1: fprintf(fp, fmt, name, v.v8); break; + case 2: fprintf(fp, fmt, name, v.v16); break; + case 4: fprintf(fp, fmt, name, v.v32); break; + case 8: fprintf(fp, fmt, name, v.v64); break; + } +} /* * Does the work for cmd_list() and any other function that requires the @@ -3491,10 +3609,11 @@ next_arg: int do_list(struct list_data *ld) { - ulong next, last, first; + ulong next, last, first, offset; ulong searchfor, readflag; - int i, count, others, close_hq_on_return; + int i, j, count, others, close_hq_on_return; unsigned int radix; + char b[BUFSIZE]; if (CRASHDEBUG(1)) { others = 0; @@ -3536,6 +3655,16 @@ do_list(struct list_data *ld) console(" structname: (unused)\n"); for (i = 0; i < ld->structname_args; i++) console(" structname[%d]: %s\n", i, ld->structname[i]); + console(" entries: %lx\n", ld->entries); + for (i = 0; i < ld->entries; i++) { + console(" entry[%d]: ", i); + for (j = 0; j < ld->e[i]->count; j++) { + snprintf(b, BUFSIZE, "{ member: %s, width: %d, offset: %d }", + ld->e[i]->member[j], ld->e[i]->width[j], ld->e[i]->offset[j]); + console("%s", b); + console(j == ld->e[i]->count - 1 ? "\n" : ", "); + } + } console(" header: %s\n", ld->header); console(" list_ptr: %lx\n", (ulong)ld->list_ptr); console(" callback_func: %lx\n", (ulong)ld->callback_func); @@ -3580,6 +3709,7 @@ do_list(struct list_data *ld) if (ld->header) fprintf(fp, "%s", ld->header); + offset = ld->list_head_offset + ld->struct_list_offset; while (1) { if (ld->flags & VERBOSE) { fprintf(fp, "%lx\n", next - ld->list_head_offset); @@ -3589,9 +3719,8 @@ do_list(struct list_data *ld) switch (count_chars(ld->structname[i], '.')) { case 0: - dump_struct(ld->structname[i], - next - ld->list_head_offset - ld->struct_list_offset, - radix); + dump_struct(ld->structname[i], + next - offset, radix); break; default: dump_struct_members(ld, i, next); @@ -3599,6 +3728,8 @@ do_list(struct list_data *ld) } } } + for (i = 0; i < ld->entries; i++) + dump_struct_members_fast(ld->e[i], radix, next - offset); } Move the section above into the "if (ld->structname)" section, and check the new LIST_XXX flag. The req_entry allocation(s) and the calls to fill_member_offsets() should be able to be done somewhere around here. if (next && !hq_enter(next - ld->list_head_offset)) { @@ -3737,6 +3868,7 @@ cmd_tree() struct tree_data tree_data, *td; struct datatype_member struct_member, *sm; struct syment *sp; + struct req_entry *re; ulong value; type_flag = 0; @@ -3745,7 +3877,7 @@ cmd_tree() td = &tree_data; BZERO(td, sizeof(struct tree_data)); - while ((c = getopt(argcnt, args, "xdt:r:o:s:pN")) != EOF) { + while ((c = getopt(argcnt, args, "xdt:r:o:s:S:pN")) != EOF) { switch (c) { case 't': @@ -3804,6 +3936,13 @@ cmd_tree() hq_enter((ulong)optarg); break; + case 'S': + re = fill_member_offsets(optarg); + if (td->entries++ == 0) + hq_open(); + hq_enter((ulong)re); + break; + Same comments as cmd_list()... case 'p': td->flags |= TREE_POSITION_DISPLAY; break; @@ -3878,11 +4017,21 @@ next_arg: cmd_usage(pc->curcmd, SYNOPSIS); } + if (td->structname_args && td->entries) { + error(INFO, "-S and -s options are mutually exclusive"); + cmd_usage(pc->curcmd, SYNOPSIS); + } + if (td->structname_args) { td->structname = (char **)GETBUF(sizeof(char *) * td->structname_args); retrieve_list((ulong *)td->structname, td->structname_args); hq_close(); + } else if (td->entries) { + td->e = (struct req_entry **)GETBUF(sizeof (struct req_entry *) * + td->entries); + retrieve_list((ulong *)td->e, td->entries); + hq_close(); } if (!(td->flags & TREE_NODE_POINTER)) @@ -3917,6 +4066,7 @@ next_arg: fprintf(fp, " start: %lx\n", td->start); fprintf(fp, "node_member_offset: %ld\n", td->node_member_offset); fprintf(fp, " structname_args: %d\n", td->structname_args); + fprintf(fp, " entries: %d\n", td->entries); fprintf(fp, " count: %d\n", td->count); } @@ -4031,6 +4181,13 @@ rdtree_iteration(ulong node_p, struct tree_data *td, char *ppos, ulong indexnum, else sprintf(pos, "%s", ppos); + if (td->flags & TREE_STRUCT_RADIX_10) + print_radix = 10; + else if (td->flags & TREE_STRUCT_RADIX_16) + print_radix = 16; + else + print_radix = 0; + for (index = 0; index < RADIX_TREE_MAP_SIZE; index++) { readmem((ulong)node_p + OFFSET(radix_tree_node_slots) + sizeof(void *) * index, KVADDR, &slot, sizeof(void *), @@ -4052,13 +4209,6 @@ rdtree_iteration(ulong node_p, struct tree_data *td, char *ppos, ulong indexnum, fprintf(fp, " position: %s/%d\n", pos, index); if (td->structname) { - if (td->flags & TREE_STRUCT_RADIX_10) - print_radix = 10; - else if (td->flags & TREE_STRUCT_RADIX_16) - print_radix = 16; - else - print_radix = 0; - for (i = 0; i < td->structname_args; i++) { switch(count_chars(td->structname[i], '.')) { @@ -4073,6 +4223,8 @@ rdtree_iteration(ulong node_p, struct tree_data *td, char *ppos, ulong indexnum, } } } + for (i = 0; i < td->entries; i++) + dump_struct_members_fast(td->e[i], print_radix, slot); Same as do_list() -- move it up into the "if (td->structname)" section, check for the new LIST_XXX pointer, and do the pre-allocations around here. } else rdtree_iteration(slot, td, pos, index, height-1); } Here's something I didn't notice until now. You set things up in cmd_tree() for subsequent execution in rdtree_iteration(). But what about rbtree_iteration()? Did you skip it on purpose? Thanks, Dave