Hi Alex, This patch certainly looks promising. I've have only just begun looking at the patch, and based upon a few initial tests, I like the functionality a lot. Nice job! However, for starters, there are a few crash utility conventions that should be followed. I'm sorry if they seem pedantic, unnecessary, unreasonable, pathetic, or whatever, but let's get them out of the way first. Before posting a patch, please build it with "make warn", and fix all the complaints: $ make warn TARGET: X86_64 CRASH: 7.1.1rc28 GDB: 7.6 cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 build_data.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 tools.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6 task.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security 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:7426:6: warning: no previous prototype for ‘free_structure’ [-Wmissing-prototypes] symbols.c:7434:15: warning: no previous prototype for ‘is_right_brace’ [-Wmissing-prototypes] symbols.c:7455:21: warning: no previous prototype for ‘find_node’ [-Wmissing-prototypes] symbols.c:7518:6: warning: no previous prototype for ‘dump_node’ [-Wmissing-prototypes] ... Global functions should be declared in the relevant location in defs.h. So for example, at the top of task.c and symbols.c you have: +void parse_for_member_new(struct datatype_member *, ulong); Please move both of those to the single location in defs.h where the other global functions for symbols.c are listed. When adding new functions, please put the function name on its own line. For example, you have: + void parse_for_member_new(struct datatype_member *dm, + ulong __attribute__ ((unused)) flag) + { I'm a dedicated cscope user, and it does not always find functions when they are declared that way. I don't know why, but here's an example where I've entered "parse_for_member_new": Could not find the global definition: parse_for_member_new Find this C symbol: Find this global definition: parse_for_member_new Find functions called by this function: Find functions calling this function: Find this text string: Change this text string: Find this egrep pattern: Find this file: Find files #including this file: Find assignments to this symbol: Also, when I'm editing a file with vi, I typically search for a function by using "^function-name", which obviously doesn't work when the function name is not at the beginning of the line. Calling printf() should never be used, but rather you should always use fprintf(fp, ...) so that the output is redirected as expected: > #define PUT_INDENTED_STRING(m, ...) { \ > for (i = 0; i++ < 2 + 2 * (m * is_array + level); printf(" ")); \ > printf(__VA_ARGS__); } Using calloc() to allocate temporary memory for functions that are called by crash commands should be avoided because of a high possibility that the command will be interrupted by ctrl-c, a "q" from the scroll prompt, or simply by an unexpected error(FATAL): > #define ALLOC_XXX_ELEMENT(xxx, clone_parent) \ > { \ > if (NULL == current) { \ > error(FATAL, "Internal error while parsing structure %s\n", dm->name); \ > } \ > current->xxx = calloc(1, sizeof(struct struct_elem)); \ > if (clone_parent) current->xxx->parent = current->parent; \ > else current->xxx->parent = current; \ > current = current->xxx; \ > } > #define ALLOC_INNER_ELEMENT { ALLOC_XXX_ELEMENT(inner, 0) } > #define ALLOC_NEXT_ELEMENT { ALLOC_XXX_ELEMENT(next, 1) } > > void free_structure(struct struct_elem *p) { > if (p == NULL) > return; > free_structure(p->inner); > free_structure(p->next); > free(p); > } To prevent memory leaks in those cases, please use the GETBUF() and FREEBUF() macros. They can be used like malloc/free, but if you do not call FREEBUF(), or if the command is prematurely interrupted as above, the buffers will be be freed automatically by restore_sanity() prior to the next "crash>" prompt. So in your case, you should be able to use GETBUF() and FREEBUF() in ALLOC_XXX_ELEMENT() and free_structure(). And lastly, I see that all of your new functions use the 4-space convention instead of using tabs. That is not done anywhere in the crash utility code, but rather the Linux kernel coding convention is used when at all possible: + void parse_for_member_new(struct datatype_member *dm, + ulong __attribute__ ((unused)) flag) + { + struct struct_elem *i, *current = NULL, *root = NULL; + + char buf[BUFSIZE]; + char *p, *p1; + char *s_e; // structure_element + unsigned int len; + unsigned char trailing_comma, braces, found = 0; + + rewind(pc->tmpfile); + + root = calloc(1, sizeof(struct struct_elem)); + current = root; + ALLOC_INNER_ELEMENT; + + while (fgets(buf, BUFSIZE, pc->tmpfile)) { + len = strlen(buf) - 1; + for (; buf[len] <= ' '; buf[len--] = '\0'); + if ((trailing_comma = (buf[len] == ','))) + buf[len--] = '\0'; + + if ((braces = is_right_brace(buf))) { + for (; braces && current; braces--) + current = current->parent; Again, I just prefer to keep things "conventional". Please don't take any offense -- the content and functionality looks quite good. Thanks, Dave ----- Original Message ----- > Hello Dave, > > here is the second reincarnation of the parser. I've updated > `task` and `struct` handlers along with the `list` handler: > > crash> list task_struct.tasks -s task_struct.comm,pid,thread.cr2 -H > 0xde83a1e0 > de840aa0 > comm = "ksoftirqd/0\000\000\000\000" > pid = 4 > thread.cr2 = 0, > de840550 > comm = "stopper/0\000\000\000\000\000\000" > pid = 5 > thread.cr2 = 0, > .................. > > crash> task -R > restart_block.fn,restart_block.futex,thread.tls_array[0].base1,cpu_timers[2].next > restart_block.fn = 0xc046fe10 <do_no_restart_syscall>, > restart_block.futex = { > uaddr = 0x0, > val = 0, > flags = 0, > bitset = 0, > time = 0, > uaddr2 = 0x0 > }, > thread.tls_array[0].base1 = 112, > cpu_timers[2].next = 0xdb9a7838, > > crash> task_struct.comm,thread.tls_array[0].dpl 0xdb9a7550 > comm = "bash\000\000\000\000\000\000\000\000\000\000\000" > thread.tls_array[0].dpl = 3, > > I'm looking forward to your comments/suggestions. > > Best regards, > Alexandr > > --- crash-7.1.0.orig/symbols.c 2015-02-06 21:44:11.000000000 +0300 > +++ crash-7.1.0/symbols.c 2015-05-11 16:11:56.587128595 +0300 > @@ -93,6 +93,7 @@ static int dereference_pointer(ulong, st > #define PARSE_FOR_DATA (1) > #define PARSE_FOR_DECLARATION (2) > static void parse_for_member(struct datatype_member *, ulong); > +void parse_for_member_new(struct datatype_member *, ulong); > static int show_member_offset(FILE *, struct datatype_member *, char *); > > > @@ -5576,17 +5577,18 @@ dump_struct_member(char *s, ulong addr, > FREEBUF(buf); > error(FATAL, "invalid structure name: %s\n", dm->name); > } > - if (!MEMBER_EXISTS(dm->name, dm->member)) { > - FREEBUF(buf); > - error(FATAL, "invalid structure member name: %s\n", > - dm->member); > - } > > set_temporary_radix(radix, &restore_radix); > > open_tmpfile(); > print_struct(dm->name, addr); > - parse_for_member(dm, PARSE_FOR_DATA); > + > + if (MEMBER_EXISTS(dm->name, dm->member)) { > + parse_for_member(dm, PARSE_FOR_DATA); > + } else { > + parse_for_member_new(dm, PARSE_FOR_DATA); > + } > + > close_tmpfile(); > > restore_current_radix(restore_radix); > @@ -6026,8 +6028,7 @@ cmd_datatype_common(ulong flags) > if ((count_chars(args[optind], ',')+1) > MAXARGS) > error(FATAL, "too many members in comma-separated list!\n"); > > - if ((count_chars(args[optind], '.') > 1) || > - (LASTCHAR(args[optind]) == ',') || > + if ((LASTCHAR(args[optind]) == ',') || > (LASTCHAR(args[optind]) == '.')) > error(FATAL, "invalid format: %s\n", args[optind]); > > @@ -6219,6 +6220,10 @@ do_datatype_addr(struct datatype_member > i = 0; > do { > if (argc_members) { > + /* This call works fine with fields > + * of the second, third, ... levels. > + * There is no need to fix it > + */ > if (!member_to_datatype(memberlist[i], dm, > ANON_MEMBER_QUERY)) > error(FATAL, "invalid data structure reference: %s.%s\n", > @@ -6250,7 +6255,12 @@ do_datatype_addr(struct datatype_member > if (dm->member) { > if (!((flags & DEREF_POINTERS) && > dereference_pointer(addr, dm, flags))) > - parse_for_member(dm, PARSE_FOR_DATA); > + { > + if (count_chars(dm->member, > '.')) > + parse_for_member_new(dm, > PARSE_FOR_DATA); > + else > + parse_for_member(dm, > PARSE_FOR_DATA); > + } > close_tmpfile(); > } > > @@ -6275,6 +6285,10 @@ do_datatype_declaration(struct datatype_ > if (CRASHDEBUG(1)) > dump_datatype_member(fp, dm); > > + if (dm->member && count_chars(dm->member, '.')) > + error(FATAL, "invalid data structure reference: %s.%s\n", > + dm->name, dm->member); > + > open_tmpfile(); > whatis_datatype(dm->name, flags, pc->tmpfile); > rewind(pc->tmpfile); > @@ -7383,6 +7397,270 @@ next_item: > } > } > > +struct struct_elem { > + char field_name[BUFSIZE]; > + unsigned char field_len; > + char value[BUFSIZE]; > + unsigned char is_array_root:1; > + > + struct struct_elem *parent; > + struct struct_elem *inner; > + struct struct_elem *next; > + struct struct_elem *prev; > +}; > + > +#define ALLOC_XXX_ELEMENT(xxx, clone_parent) \ > +{ \ > + if (NULL == current) { \ > + error(FATAL, "Internal error while parsing structure %s\n", > dm->name); \ > + } \ > + current->xxx = calloc(1, sizeof(struct struct_elem)); \ > + if (clone_parent) current->xxx->parent = current->parent; \ > + else current->xxx->parent = current; \ > + current = current->xxx; \ > +} > + > +#define ALLOC_INNER_ELEMENT { ALLOC_XXX_ELEMENT(inner, 0) } > +#define ALLOC_NEXT_ELEMENT { ALLOC_XXX_ELEMENT(next, 1) } > + > +void free_structure(struct struct_elem *p) { > + if (p == NULL) > + return; > + free_structure(p->inner); > + free_structure(p->next); > + free(p); > +} > + > +unsigned char is_right_brace(const char *b) { > + unsigned char r = 0; > + for (; *b == ' '; b++); > + if (*b == '}') { > + b++; > + r = 1; > + if (*b == '}') { > + r = 2; > + b++; > + } > + } > + > + if (*b == ',') > + b++; > + > + if (*b == '\0') > + return r; > + else > + return 0; > +} > + > +struct struct_elem *find_node(struct struct_elem *s, char *n) { > + char *p, *b, *e; > + struct struct_elem *t = s; > + unsigned i; > + > + if (('\0' == *n) || (NULL == s)) > + return s; > + > + /* [n .. p) - struct member with index*/ > + if (NULL == (p = strstr(n, "."))) > + p = n + strlen(n); > + > + /* [n .. b) - struct member without index*/ > + for (b = n; (b < p) && (*b != '['); b++); > + > + /* s - is the current level of items [s, s->next, ..., s->...->next] */ > + for (; s; s = s->next) { > + if (*s->field_name == '\0') > + continue; > + > + /* `field_name` doesn't match */ > + if (((b - n) != s->field_len) || memcmp(s->field_name, n, b - n)) > + continue; > + > + if (*b == '[') { /* Array */ > + i = strtol(b + 1, &e, 10); > + /* Check if the current node is array and > + * we've parsed index more or less correctly > + */ > + if (!(s->is_array_root && *e == ']'&& (e != b + 1))) > + return NULL; > + > + /* Look for the i-th element */ > + for (s = s->inner; s && i; s = s->next, i--); > + if (i || (NULL == s)) > + return NULL; > + } > + > + /* Ok. We've found node, it's - the last member > + * in our search string, let's return it. > + */ > + if ('\0' == *p) > + return s; > + else > + return find_node(s->inner, p + 1); > + } > + > + // We haven't found any field. > + // Might happen, we've encountered anonymous structure > + // of union. Lets try every record without `field_name` > + s = t; > + t = NULL; > + for(; s; s = s->next) { > + if (*s->field_name) > + continue; > + t = find_node(s->inner, n); > + if (t) > + break; > + } > + > + return t; > +} > + > +void dump_node(struct struct_elem *p, char *f, unsigned char level, unsigned > char is_array) { > + unsigned int i; > + if (p == NULL) > + return; > + do { > +#define PUT_INDENTED_STRING(m, ...) { \ > + for (i = 0; i++ < 2 + 2 * (m * is_array + level); printf(" ")); \ > + printf(__VA_ARGS__); } > + > + if (p->inner) { > + if (*p->field_name) { > + PUT_INDENTED_STRING(1, "%s = %s\n", f ? f : p->field_name, > p->inner->is_array_root ? "{{" : "{"); > + } else { > + if (f) /* For union */ > + PUT_INDENTED_STRING(1, "%s = ", f); > + PUT_INDENTED_STRING(1, "%s\n", p->inner->is_array_root ? > "{{" : "{"); > + } > + dump_node(p->inner, NULL, is_array + level + 1, > p->inner->is_array_root); > + PUT_INDENTED_STRING(1, "%s%s\n", p->inner->is_array_root ? "}}" > : "}", (p->next && !p->next->is_array_root) ? "," : ""); > + } else { > + PUT_INDENTED_STRING(1, "%s = %s%s", f ? f : p->field_name, > p->value, p->next ? ",\n" : "\n"); > + } > + if (level) { > + p = p->next; > + if (p && p->is_array_root) > + PUT_INDENTED_STRING(0, "}, {\n"); > + } > + } while (p && level); > +} > + > +void parse_for_member_new(struct datatype_member *dm, > + ulong __attribute__ ((unused)) flag) > +{ > + struct struct_elem *i, *current = NULL, *root = NULL; > + > + char buf[BUFSIZE]; > + char *p, *p1; > + char *s_e; // structure_element > + unsigned int len; > + unsigned char trailing_comma, braces, found = 0; > + > + rewind(pc->tmpfile); > + > + root = calloc(1, sizeof(struct struct_elem)); > + current = root; > + ALLOC_INNER_ELEMENT; > + > + while (fgets(buf, BUFSIZE, pc->tmpfile)) { > + len = strlen(buf) - 1; > + for (; buf[len] <= ' '; buf[len--] = '\0'); > + if ((trailing_comma = (buf[len] == ','))) > + buf[len--] = '\0'; > + > + if ((braces = is_right_brace(buf))) { > + for (; braces && current; braces--) > + current = current->parent; > + > + if ((current->parent == root) || trailing_comma) > + ALLOC_NEXT_ELEMENT; > + continue; > + } > + > + for (p1 = buf; *p1 == ' '; p1++); > + > + if (NULL != (p = strstr(buf, " = "))) { > + s_e = p + 3; > + } else { > + s_e = p1; > + } > + > + /* > + * After that we have pointers: > + * foobar = bazzz > + * -----^ ^ ^ > + * | ------| | > + * | | | > + * p1 p s_e > + * > + * OR > + * > + * { > + * ^ > + * | > + * --------- > + * | | > + * p1 s_e > + * > + * p == NULL > + * > + * > + * p1 - the first non-whitespace symbol in line > + * p - pointer to line ' = '. If not NULL, there is identifier > + * s_e - element of structure (brace / double brace / array > separator / scalar :)) > + * > + */ > + > + if (current && p) strncpy(current->field_name, p1, p - p1); > + current->field_len = p - p1; > + > + if ( p && (*s_e != '{' || (*s_e == '{' && buf[len] == '}') )) { > + /* Scalar or one-line array > + * next = 0x0 > + * or > + * files = {0x0, 0x0} > + */ > + strcpy(current->value, s_e); > + if (trailing_comma) ALLOC_NEXT_ELEMENT; > + } > + else > + if ( *s_e == '{' ) { > + ALLOC_INNER_ELEMENT; > + if (*(s_e + 1) == '{') { > + current->parent->is_array_root = 1; > + ALLOC_INNER_ELEMENT; > + } > + } > + else > + if (strstr(s_e, "}, {")) { > + /* Next array element */ > + current = current->parent; > + ALLOC_NEXT_ELEMENT; > + ALLOC_INNER_ELEMENT; > + } > + else > + if (buf == (p = strstr(buf, "struct "))) { > + p += 7; /* strlen "struct " */ > + p1 = strstr(buf, " {"); > + strncpy(current->field_name, p, p1 - p); > + ALLOC_INNER_ELEMENT; > + } > + } > + > + for (i = root->inner; i; i = i->next) { > + if ((current = find_node(i->inner, dm->member))) { > + dump_node(current, dm->member, 0, 0); > + found = 1; > + break; > + } > + } > + > + free_structure(root); > + > + if (!found) > + error(WARNING, "invalid structure reference: %s\n", dm->member); > +} > + > /* > * Dig out a member name from a formatted gdb structure declaration dump, > * and print its offset from the named structure passed in. > --- crash-7.1.0.orig/task.c 2015-02-06 21:44:11.000000000 +0300 > +++ crash-7.1.0/task.c 2015-05-11 15:52:57.387111513 +0300 > @@ -107,6 +107,8 @@ static int sort_by_last_run(const void * > static void sort_context_array_by_last_run(void); > static void show_ps_summary(ulong); > static void irqstacks_init(void); > +static void parse_task_thread(int argcnt, char *arglist[], struct > task_context *); > +void parse_for_member_new(struct datatype_member *, ulong); > > /* > * Figure out how much space will be required to hold the task context > @@ -2746,13 +2748,9 @@ task_struct_member(struct task_context * > int argcnt; > char *arglist[MAXARGS]; > char *refcopy; > - char buf[BUFSIZE]; > - char lookfor1[BUFSIZE]; > - char lookfor2[BUFSIZE]; > - char lookfor3[BUFSIZE]; > - int header_printed; > - > - header_printed = FALSE; > + unsigned char call_new_parser = 0; > + struct datatype_member dm; > + char *member = GETBUF(BUFSIZE); > > if ((count_chars(ref->str, ',')+1) > MAXARGS) { > error(INFO, > @@ -2767,14 +2765,39 @@ task_struct_member(struct task_context * > argcnt = parse_line(refcopy, arglist); > for (i = 0; i < argcnt; i++) > if (!MEMBER_EXISTS("task_struct", arglist[i]) && > - !MEMBER_EXISTS("thread_info", arglist[i])) > - error(INFO, "%s: not a task_struct or thread_info member\n", > - arglist[i]); > - > + !MEMBER_EXISTS("thread_info", arglist[i])) { > + if (count_chars(arglist[i], '.') || count_chars(arglist[i], '[')) > + call_new_parser = 1; > + else > + error(INFO, "%s: not a task_struct or " > + "thread_info member\n", arglist[i]); > + } > open_tmpfile(); > dump_struct("task_struct", tc->task, radix); > - if (tt->flags & THREAD_INFO) > - dump_struct("thread_info", tc->thread_info, radix); > + if (tt->flags & THREAD_INFO) > + dump_struct("thread_info", tc->thread_info, radix); > + if (call_new_parser) { > + for (i = 0; i < argcnt; i++) { > + dm.member = arglist[i]; > + parse_for_member_new(&dm, 0); > + } > + } else { > + parse_task_thread(argcnt, arglist, tc); > + } > + close_tmpfile(); > + > + FREEBUF(member); > + > +} > + > +static void parse_task_thread(int argcnt, char *arglist[], struct > task_context *tc) { > + char buf[BUFSIZE]; > + char lookfor1[BUFSIZE]; > + char lookfor2[BUFSIZE]; > + char lookfor3[BUFSIZE]; > + int header_printed = FALSE; > + int i; > + > rewind(pc->tmpfile); > > BZERO(lookfor1, BUFSIZE); > @@ -2825,7 +2848,6 @@ task_struct_member(struct task_context * > } > } > } > - close_tmpfile(); > } > > static char *ps_exclusive = > --- crash-7.1.0.orig/tools.c 2015-02-06 21:44:11.000000000 +0300 > +++ crash-7.1.0/tools.c 2015-05-11 15:52:57.391111513 +0300 > @@ -3526,13 +3526,9 @@ do_list(struct list_data *ld) > dump_struct(ld->structname[i], > next - ld->list_head_offset, radix); > break; > - case 1: > + default: > dump_struct_members(ld, i, next); > break; > - default: > - error(FATAL, > - "invalid structure reference: %s\n", > - ld->structname[i]); > } > } > } > > -- > Crash-utility mailing list > Crash-utility@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/crash-utility > -- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/crash-utility