From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25672 invoked by alias); 11 Mar 2004 13:25:14 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 25633 invoked from network); 11 Mar 2004 13:25:10 -0000 Received: from unknown (HELO lakemtao01.cox.net) (68.1.17.244) by sources.redhat.com with SMTP; 11 Mar 2004 13:25:10 -0000 Received: from white ([68.9.64.121]) by lakemtao01.cox.net (InterMail vM.5.01.06.08 201-253-122-130-108-20031117) with ESMTP id <20040311132509.CIQW26108.lakemtao01.cox.net@white>; Thu, 11 Mar 2004 08:25:09 -0500 Received: from bob by white with local (Exim 3.35 #1 (Debian)) id 1B1QBR-0000l2-00; Thu, 11 Mar 2004 08:25:09 -0500 Date: Fri, 19 Mar 2004 00:09:00 -0000 From: Bob Rossi To: gdb-patches@sources.redhat.com Cc: Elena Zannoni Subject: Re: -file-list-exec-source-files Message-ID: <20040311132508.GA2504@white> Mail-Followup-To: gdb-patches@sources.redhat.com, Elena Zannoni References: <20040225040059.GB19094@white> <16456.65451.461753.66554@localhost.redhat.com> <20040306155700.GA9439@white> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20040306155700.GA9439@white> User-Agent: Mutt/1.3.28i X-SW-Source: 2004-03/txt/msg00265.txt.bz2 Message-ID: <20040319000900.GXfsdEpX-o3tpqZwpEc3iRPcXzCKbBm55s_ZMKHRXQ0@z> What do you think? Bob Rossi On Sat, Mar 06, 2004 at 10:57:00AM -0500, Bob Rossi wrote: > Elena, thanks for taking the time to review my patch. > > When is a good point to resubmit a patch? After every review, or after > all the issues are ironed out? > > > > 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. > > Ok, I will try to fix all of the coding standard errors. Is there a > program I can run on the source files before I create the diff that > formats the code according to the standard? > > > > > > > > > Index: gdb/dbxread.c > > > =================================================================== > > > Index: gdb/dwarf2read.c > > > =================================================================== > > > > These are ok > > Great! > > > > > > > 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. > > It would probably just be best if you told me what case's you want me to > implement here. It seems that Jason Molenda understodd most of the > cases. I really don't know anything about what GCC emits and would would > be practical to implement. > > > > Index: gdb/source.c > > > =================================================================== > > > > this part I am not clear about. > > Ok, Ok. I thought about this a lot. I think I made the best decision and > can describe why. > > A few assumptions are in order. In order to get the fullname (abs path) > to a file, GDB need's three things. The directory the file was compiled > in (dirname), the filename in question (filename) and a list of paths > to search. > > > There is already a function called source_full_path_of() would it help > > if you used it? > > The function source_full_path_of does not take into account 'dirname'. > It calls openp, which is not capable of finding the fullname of a file, > since it doesn't understand what dirname is. Basically, I don't even > think this function (source_full_path_of) is "truly" capable of > finding the fullpath to a file. However, instead of removing it, > I left it, since caller's of this function might be using for something > I know nothing about. > > > 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. > > open_source_file was left around for backwards compatibility. The unit > source.c was used to calling a function, with just passing the symtab, > and getting back the symtab with a valid fullname. I could remove all > occurences of this function and replace it with symtab_to_fullname. > > > 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. > > I basically think that the call -file-list-exec-source-files shouldn't > 'cache' it's results. GDB looks for each file, every time it is > requested to get the fullname. This is because, the user could have > changed the path, or moved/deleted the file. I don't think GDB should > just return the filename instead, of the fullname. > > So, if find_and_open_source couldn't "find and open the source file", it > returns NULL. Also, as a side effect, fullname in the [ps]ymtab also > get's set to NULL. > > The testsuite didn't seem to have a problem with this, and I think it > makes sense to not trick the caller into having results when it couldn't > find any. > > If the caller really wanted this functionality, > return s->filename; /* File not found. Just use short name */ > I believe it should be the caller's responsibility. > > if ( symtab_to_fullname ( s ) == NULL ) > /* use symtab->filename */ > else > /* use symtab->fullname */ > > It doesn't really make sense to return the filename and not state that > it is not really the fullname. Also, if the caller tries to access > s->fullname, it will not be successful, because the file simply isn't > there. > > > > 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.... > > Ok. > > > > 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; > > > } > > > > > > + 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. > > It just unifies the output convention I am using in the > mi-cmd-file unit. What would you prefer to see? > > > > > > > /* 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? > > Ok. > > > > + 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? > > I don't know. I am thinking that GDB should just return the absolute > path to all of the source files it can find. If it can not find some, > should it issue a warning? That way the front end could say, "you need > to add a directory to the source search path". > > > > + 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.... > > Ok. > > > > + 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. > > Great! > > > > 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? > > Yes. I figured it could be mi2. Also, for some reason, I thought no one > would be using this function since I wrote it for CGDB, and I haven't > used it yet. I have a larger plan in mind for MI, than just these 2 > commands (-file-list-exec-source-file and -file-list-exec-source-files). > I would like to add the fullname to a lot of commands. However, I think > 'filename' and 'fullname' should be standardized, so that front end > writers immediatly understand what they are. It is awkard to have 1 > function say "file=" and another say "filename=", when those 2 words > mean the same thing. > > However, if this changes isn't acceptable, I can change it back. > > > > "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? > > OOO, That isn't an error condition, it's just a comment. I see. > > Thanks, > Bob Rossi