[Crash-utility] [PATCH 1/1] Resend CLI list command, print deep structure members
Dave Anderson
anderson at redhat.com
Tue May 12 22:09:39 UTC 2015
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>
More information about the Crash-utility
mailing list