Re: -file-list-exec-source-files

[Elena Zannoni <ezannoni@redhat.com>]

Bob Rossi writes:
 > Hi,
 > 
 > Here is an initial patch for -file-list-exec-source-files.
 > Some feedback would be appreciated.
 > 
 > I ran the testsuite and the results are the same before and after this
 > patch.
 > 
 > Index: gdb/ChangeLog
 > 	* dbxread.c (read_dbx_symtab): set pst->dirname when known

Each entry should start with capital letter and end with period.

I see some coding standards are not adhered to throughout the code.
Most noticeably "foo ( int a )" should be "foo (int a)". Similarly for
calls.

 > 
 > Index: gdb/dbxread.c
 > ===================================================================
 > Index: gdb/dwarf2read.c
 > ===================================================================

These are ok

 >  
 > Index: gdb/defs.h
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/defs.h,v
 > retrieving revision 1.143
 > diff -w -u -r1.143 defs.h
 > --- gdb/defs.h	23 Feb 2004 19:26:14 -0000	1.143
 > +++ gdb/defs.h	25 Feb 2004 03:51:35 -0000
 > @@ -616,8 +616,6 @@
 >  
 >  extern void init_last_source_visited (void);
 >  
 > -extern char *symtab_to_filename (struct symtab *);
 > -
 >  /* From exec.c */
 >  
 >  extern void exec_set_section_offsets (bfd_signed_vma text_off,
 > Index: gdb/dwarf2read.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/dwarf2read.c,v
 > retrieving revision 1.135
 > diff -w -u -r1.135 dwarf2read.c
 > --- gdb/dwarf2read.c	21 Feb 2004 02:13:35 -0000	1.135
 > +++ gdb/dwarf2read.c	25 Feb 2004 03:51:43 -0000
 > @@ -316,6 +316,7 @@
 >      unsigned int offset;
 >      unsigned int abbrev;
 >      char *name;
 > +    char *dirname;
 >      int has_pc_info;
 >      CORE_ADDR lowpc;
 >      CORE_ADDR highpc;
 > @@ -1254,6 +1255,8 @@
 >  				  objfile->global_psymbols.next,
 >  				  objfile->static_psymbols.next);
 >  
 > +      pst->dirname = xstrdup ( comp_unit_die.dirname );
 > +
 >        pst->read_symtab_private = (char *)
 >  	obstack_alloc (&objfile->objfile_obstack, sizeof (struct dwarf2_pinfo));
 >        DWARF_INFO_BUFFER (pst) = dwarf_info_buffer;
 > @@ -4326,6 +4329,10 @@
 >  	  /* Prefer DW_AT_MIPS_linkage_name over DW_AT_name.  */
 >  	  if (part_die->name == NULL)
 >  	    part_die->name = DW_STRING (&attr);
 > +	  break;
 > +	case DW_AT_comp_dir:
 > +	  if (part_die->dirname == NULL)
 > +	    part_die->dirname = DW_STRING (&attr);

The dwarf2 specs say that the name is in the form ":pathname" or
"hostname:pathname". Should we worry about the hostname? Does gcc emit
that?  I have looked at a few executables and didn't see the hostname
part.


 > Index: gdb/source.c
 > ===================================================================

this part I am not clear about.

There is already a function called source_full_path_of() would it help
if you used it?

What is the difference between find_and_open_source and
open_source_file?  I.e. why did you need to introduce it. I think it's
not clear just from your comments about the file possibly baing moved
around.

I am a bit worried about the substitution of symtab_to_filename with
symtab_to_fullname. The former returns null only if there is no
symtab.  The latter returns null when there is no symtab OR when it
cannot find the file. So the behavior is slightly different.


 > RCS file: /cvs/src/src/gdb/source.c,v
 > retrieving revision 1.49
 > diff -w -u -r1.49 source.c
 > --- gdb/source.c	27 Jan 2004 23:19:51 -0000	1.49
 > +++ gdb/source.c	25 Feb 2004 03:51:45 -0000
 > @@ -805,30 +805,47 @@
 >    return 1;
 >  }
 >  
 > -
 > -/* Open a source file given a symtab S.  Returns a file descriptor or
 > -   negative number for error.  */
 > -
 > +/* This function is capable of finding the absolute path to a
 > +   source file, and opening it, provided you give it an 
 > +   OBJFILE and FILENAME. Both the DIRNAME and FULLNAME are only
 > +   added suggestions on where to find the file. 
 > +
 > +   OBJFILE should be the objfile associated with a psymtab or symtab. 
 > +   FILENAME should be the filename to open.
 > +   DIRNAME is the compilation directory of a particular source file.
 > +           Only some debug formats provide this info.
 > +   FULLNAME can be the last known absolute path to the file in question.
 > +
 > +   On Success 
 > +     A valid file descriptor is returned. ( the return value is positive )
 > +     FULLNAME is set to the absolute path to the file just opened.
 > +
 > +   On Failure
 > +     A non valid file descriptor is returned. ( the return value is negitive ) 
 > +     FULLNAME is set to NULL.  */
 >  int
 > -open_source_file (struct symtab *s)
 > +find_and_open_source ( 
 > +  struct objfile *objfile,	
 > +  const char *filename,
 > +  const char *dirname,
 > +  char **fullname )
 >  {

coding standards....

 >    char *path = source_path;
 >    const char *p;
 >    int result;
 > -  char *fullname;
 >  
 >    /* Quick way out if we already know its full name */
 > -  if (s->fullname)
 > +  if (*fullname)
 >      {
 > -      result = open (s->fullname, OPEN_MODE);
 > +      result = open (*fullname, OPEN_MODE);
 >        if (result >= 0)
 >  	return result;
 >        /* Didn't work -- free old one, try again. */
 > -      xmfree (s->objfile->md, s->fullname);
 > -      s->fullname = NULL;
 > +      xmfree (objfile->md, *fullname);
 > +      *fullname = NULL;
 >      }
 >  
 > -  if (s->dirname != NULL)
 > +  if (dirname != NULL)
 >      {
 >        /* Replace a path entry of  $cdir  with the compilation directory name */
 >  #define	cdir_len	5
 > @@ -841,60 +858,102 @@
 >  	  int len;
 >  
 >  	  path = (char *)
 > -	    alloca (strlen (source_path) + 1 + strlen (s->dirname) + 1);
 > +	    alloca (strlen (source_path) + 1 + strlen (dirname) + 1);
 >  	  len = p - source_path;
 >  	  strncpy (path, source_path, len);	/* Before $cdir */
 > -	  strcpy (path + len, s->dirname);	/* new stuff */
 > +	  strcpy (path + len, dirname);	/* new stuff */
 >  	  strcat (path + len, source_path + len + cdir_len);	/* After $cdir */
 >  	}
 >      }
 >  
 > -  result = openp (path, 0, s->filename, OPEN_MODE, 0, &s->fullname);
 > +  result = openp (path, 0, filename, OPEN_MODE, 0, fullname);
 >    if (result < 0)
 >      {
 >        /* Didn't work.  Try using just the basename. */
 > -      p = lbasename (s->filename);
 > -      if (p != s->filename)
 > -	result = openp (path, 0, p, OPEN_MODE, 0, &s->fullname);
 > +      p = lbasename (filename);
 > +      if (p != filename)
 > +	result = openp (path, 0, p, OPEN_MODE, 0, fullname);
 >      }
 >  
 >    if (result >= 0)
 >      {
 > -      fullname = s->fullname;
 > -      s->fullname = mstrsave (s->objfile->md, s->fullname);
 > -      xfree (fullname);
 > +      char *tmp_fullname;
 > +      tmp_fullname = *fullname;
 > +      *fullname = mstrsave (objfile->md, *fullname);
 > +      xfree (tmp_fullname);
 >      }
 >    return result;
 >  }
 >  
 > -/* Return the path to the source file associated with symtab.  Returns NULL
 > -   if no symtab.  */
 > +/* Open a source file given a symtab S.  Returns a file descriptor or
 > +   negative number for error.  
 > +   
 > +   This function is a convience function to find_and_open_source. */
 > +
 > +int
 > +open_source_file (struct symtab *s)
 > +{
 > +    if (!s)
 > +      return -1;
 > +
 > +    return find_and_open_source ( s->objfile, s->filename, s->dirname, &s->fullname );
 > +}
 > +
 > +/* Finds the fullname that a symtab represents.
 > +
 > +   If this functions finds the fullname, it will save it in ps->fullname
 > +   and it will also return the value.
 >  
 > +   If this function fails to find the file that this symtab represents,
 > +   NULL will be returned and ps->fullname will be set to NULL.  */
 >  char *
 > -symtab_to_filename (struct symtab *s)
 > +symtab_to_fullname (struct symtab *s)
 >  {
 > -  int fd;
 > +  int r;
 >  
 >    if (!s)
 >      return NULL;
 >  
 > -  /* If we've seen the file before, just return fullname. */
 > +  /* Don't check s->fullname here, the file could have been 
 > +     deleted/moved/..., look for it again */
 > +  r = find_and_open_source ( s->objfile, s->filename, s->dirname, &s->fullname);
 >  
 > -  if (s->fullname)
 > +  if (r)
 > +  {
 > +    close (r);
 >      return s->fullname;
 > +  }
 >  
 > -  /* Try opening the file to setup fullname */
 > +  return NULL;
 > +}
 >  
 > -  fd = open_source_file (s);
 > -  if (fd < 0)
 > -    return s->filename;		/* File not found.  Just use short name */
 > +/* Finds the fullname that a partial_symtab represents.
 >  
 > -  /* Found the file.  Cleanup and return the full name */
 > +   If this functions finds the fullname, it will save it in ps->fullname
 > +   and it will also return the value.
 >  
 > -  close (fd);
 > -  return s->fullname;
 > +   If this function fails to find the file that this partial_symtab represents,
 > +   NULL will be returned and ps->fullname will be set to NULL.  */
 > +char *
 > +psymtab_to_fullname (struct partial_symtab *ps)
 > +{
 > +  int r;
 > +
 > +  if (!ps)
 > +    return NULL;
 > +
 > +  /* Don't check ps->fullname here, the file could have been
 > +     deleted/moved/..., look for it again */
 > +  r = find_and_open_source ( ps->objfile, ps->filename, ps->dirname, &ps->fullname);
 > +
 > +  if (r) 
 > +  {
 > +    close (r);
 > +    return ps->fullname;
 >  }
 >  \f
 > +  return NULL;
 > +}
 >  
 >  /* Create and initialize the table S->line_charpos that records
 >     the positions of the lines in the source file, which is assumed



 > Index: gdb/source.h
 > ===================================================================
 > Index: gdb/symtab.c
 > ===================================================================

These are obvious if the rest goes in.


 > Index: gdb/symtab.h
 > ===================================================================

OK.


 > Index: gdb/mi/mi-cmd-file.c
 > ===================================================================


 > +static const char * const FILENAME = "filename";
 > +static const char * const FULLNAME = "fullname";

I don't think these are necessary.

 >  
 >  /* Return to the client the absolute path and line number of the 
 >     current file being executed. */
 > @@ -39,7 +43,6 @@
 >    if ( !mi_valid_noargs("mi_cmd_file_list_exec_source_file", argc, argv) )
 >      error ("mi_cmd_file_list_exec_source_file: Usage: No args");
 >  
 > -  
 >    /* Set the default file and line, also get them */
 >    set_default_source_symtab_and_line();
 >    st = get_current_source_symtab_and_line();
 > @@ -51,17 +54,67 @@
 >      error ("mi_cmd_file_list_exec_source_file: No symtab");
 >  
 >    /* Extract the fullname if it is not known yet */
 > -  if (st.symtab->fullname == NULL)
 > -    symtab_to_filename (st.symtab);
 > -
 > -  /* We may not be able to open the file (not available). */
 > -  if (st.symtab->fullname == NULL)
 > -    error ("mi_cmd_file_list_exec_source_file: File not found");

Why get rid of the error message?

 > +  symtab_to_fullname (st.symtab);
 >  
 >    /* Print to the user the line, filename and fullname */
 >    ui_out_field_int (uiout, "line", st.line);
 > -  ui_out_field_string (uiout, "file", st.symtab->filename);
 > -  ui_out_field_string (uiout, "fullname", st.symtab->fullname);
 > +  ui_out_field_string (uiout, FILENAME, st.symtab->filename);
 > +  
 > +  /* We may not be able to open the file (not available). */
 > +  if (st.symtab->fullname)
 > +    ui_out_field_string (uiout, FULLNAME, st.symtab->fullname);
 > +

if this test fails shouldn't some warning/error be issued?

 > +  return MI_CMD_DONE;
 > +}
 > +
 > +enum mi_cmd_result
 > +mi_cmd_file_list_exec_source_files(char *command, char **argv, int argc)
 > +{
 > +  struct symtab *s;
 > +  struct partial_symtab *ps;
 > +  struct objfile *objfile;
 > +
 > +  if ( !mi_valid_noargs("mi_cmd_file_list_exec_source_files", argc, argv) )
 > +    error ("mi_cmd_file_list_exec_source_files: Usage: No args");
 > +
 > +  /* Print the table header */
 > +  ui_out_begin ( uiout, ui_out_type_list, "files");
 > +
 > +  /* Look at all of the symtabs */
 > +  ALL_SYMTABS (objfile, s)
 > +  {
 > +    ui_out_begin ( uiout, ui_out_type_tuple, NULL);
 > +
 > +    ui_out_field_string (uiout, FILENAME, s->filename);
 > +
 > +	/* Extract the fullname if it is not known yet */
 > +	symtab_to_fullname (s);
 > +
 > +	if (s->fullname)
 > +      ui_out_field_string (uiout, FULLNAME, s->fullname);
 > +
 > +    ui_out_end ( uiout, ui_out_type_tuple );
 > +  }
 > +
 > +  /* Look at all of the psymtabs */
 > +  ALL_PSYMTABS (objfile, ps)
 > +  {
 > +    if (!ps->readin) {

coding standards....

 > +      ui_out_begin ( uiout, ui_out_type_tuple, NULL);
 > +
 > +      ui_out_field_string (uiout, FILENAME, ps->filename);
 > +
 > +      /* Extract the fullname if it is not known yet */
 > +	  psymtab_to_fullname (ps);
 > +
 > +	  if (ps->fullname) 
 > +	    ui_out_field_string (uiout, FULLNAME, ps->fullname);
 > +
 > +      ui_out_end ( uiout, ui_out_type_tuple );
 > +    }
 > +  }
 > +
 > +  ui_out_end ( uiout, ui_out_type_list );
 >  
 >    return MI_CMD_DONE;
 >  }



 > Index: gdb/mi/mi-cmds.c
 > ===================================================================

 > Index: gdb/mi/mi-cmds.h
 > ===================================================================

these changes are ok.




 > Index: gdb/testsuite/gdb.mi/mi-file.exp
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-file.exp,v
 > retrieving revision 1.1
 > diff -w -u -r1.1 mi-file.exp
 > --- gdb/testsuite/gdb.mi/mi-file.exp	2 Apr 2003 22:10:35 -0000	1.1
 > +++ gdb/testsuite/gdb.mi/mi-file.exp	25 Feb 2004 03:52:36 -0000
 > @@ -55,7 +55,7 @@
 >  
 >      # get the path and absolute path to the current executable
 >      mi_gdb_test "111-file-list-exec-source-file" \
 > -	    "111\\\^done,line=\"23\",file=\"${srcfilepath}\",fullname=\"/.*/${srcfile}\"" \
 > +	    "111\\\^done,line=\"23\",filename=\"${srcfilepath}\",fullname=\"/.*/${srcfile}\"" \

Wouldn't this break existing MI parsers?


 >                 "request path info of current source file (${srcfile})"
 >  }
 >  
 > Index: gdb/testsuite/gdb.mi/mi2-file.exp
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi2-file.exp,v
 > retrieving revision 1.1
 > diff -w -u -r1.1 mi2-file.exp
 > --- gdb/testsuite/gdb.mi/mi2-file.exp	7 Aug 2003 17:47:42 -0000	1.1
 > +++ gdb/testsuite/gdb.mi/mi2-file.exp	25 Feb 2004 03:52:36 -0000
 > @@ -47,7 +47,7 @@
 >  mi_gdb_reinitialize_dir $srcdir/$subdir
 >  mi_gdb_load ${binfile}
 >  
 > -proc test_tbreak_creation_and_listing {} {
 > +proc test_file_list_exec_source_file {} {
 >      global srcfile
 >      global srcdir
 >      global subdir
 > @@ -55,11 +55,21 @@
 >  
 >      # get the path and absolute path to the current executable
 >      mi_gdb_test "111-file-list-exec-source-file" \
 > -	    "111\\\^done,line=\"23\",file=\"${srcfilepath}\",fullname=\"/.*/${srcfile}\"" \
 > +	    "111\\\^done,line=\"23\",filename=\"${srcfilepath}\",fullname=\"/.*/${srcfile}\"" \
 >                 "request path info of current source file (${srcfile})"
 >  }
 >  
 > -test_tbreak_creation_and_listing
 > +proc test_file_list_exec_source_files {} {
 > +    global srcfile
 > +
 > +    # get the path and absolute path to the current executable
 > +    mi_gdb_test "222-file-list-exec-source-files" \
 > +	    "222\\\^done,files=\\\[\{filename=\".*/${srcfile}\",fullname=\"/.*/${srcfile}\"\},\{filename=\".*\"\},\{filename=\".*\"\},\{filename=\".*\"\},\{filename=\".*\"\}\\\]" \

 > +              "Getting a list of source files failed."
                                                 ^^^^^^^
                                                  why failed?