Re: [Crash-utility] [Patch 1/2] Request data structures of particular size. GDB part

----- Original Message ----- Hello Alexandr, I've been playing with your patch for the last couple of days, and in the interest of getting it checked in without us having to go back and forth again and again, I have an updated version queued for crash-7.1.5: https://github.com/crash-utility/crash/commit/7b5be97daafb840d59ff2ae303d... First I want to thank you for getting the base functionality in place, especially the gdb portion. But the patch did need some work -- here are a few things that I did to it: (1) I changed "-f field" to "-m member" simply because "member" is used in several other commands. (2) I made the -r and -m options independent of one another, because it's just as helpful to be able to search for all data structures that contain a given member type without caring about their size. (3) I really don't like the usage of anonymous data structures defined in the prologue of a function, especially if it's not obvious what they are supposed to be because of single-letter member names, and also if they end up being declared redundantly in different functions. So you've got this in your patch: +static void +append_struct_symbol (void *pout, struct gnu_request *r) +{ + struct { + int sz, idx; + struct { char *n; int s; } *st; + } *output = pout; and this: +static void +request_types(int lowest, int highest, char *type_name) +{ + struct gnu_request request = {0}; + struct { + int sz, idx; + struct { char *n; int s; } *st; + } output = {0, 0, NULL}; and then this, which actually seems to be a bug given that the "s" member below should actually be an int instead of a ulong: +static int +compare_size_name(const void *va, const void *vb) { + const struct { char *n; ulong s; } *a = va, *b = vb; + So to replace those 3 instances with something more maintainable, I declared a function that all three of the functions can use: struct type_request { int cnt; /* current number of entries in types array */ int idx; /* index to next entry in types array */ struct type_info { /* dynamically-sized array of collected types */ char *name; ulong size; } *types; }; The same thing goes for this iterator construct, which you define in symbols.c and then in gdb-7.6/gdb/symtab.c -- and where they don't even look alike upon first glance: +static void +request_types(int lowest, int highest, char *type_name) +{ ... [ cut ] ... + struct { char fi, i; void * p[3]; } iter = { 0 }; which is the same as this: +static void +iterate_datatypes (struct gnu_request *req) +{ ... [ cut ] ... + struct { + char fi, i; + struct symtab *st; + struct symbol *sym; + struct objfile *o; + } *gi = (void *)req->addr2; /*Global iterator */ So as I suggested that you could have done, i.e., I added a new member to the generic gnu_request data structure that gets passed from the top level crash source down into gdb. I called it a global_iterator structure, and clarified it by naming its members accordingly: struct gnu_request { int command; char *buf; FILE *fp; ulong addr; ... [ cut ] ... ulong task; ulong debug; struct stack_hook *hookp; struct global_iterator { int finished; int block_index; struct symtab *symtab; struct symbol *sym; struct objfile *obj; } global_iterator; }; I can envision that your new GNU_GET_NEXT_DATATYPE functionality could be used for other purposes in the future. (4) I changed it so GNU_LOOKUP_STRUCT_CONTENTS only gets called if -m is used; it doesn't make much sense to call it if only -r is in play. (5) I re-wrote the whatis help page to clarify and separate the -r and -m usage from the traditional usage. Again, I have to say that this is a nice feature, and that I appreciate your efforts in getting it off the ground. Thanks, Dave