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

[Dave Anderson]

----- Original Message -----
> Hello Dave,
> 
> I've rewritten the mentioned logic, now crash communicates with gdb by means
> of gdb_interface.

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/7b5be97daafb840d59ff2ae303d2f18162348ec4

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