[RFA] maintenance_print_msymbols: Try harder to match files

[RFA] maintenance_print_msymbols: Try harder to match files

[Corinna Vinschen]

Hi,

while testing on Cygwin, I found that the maintenance_print_msymbols
doesn't evaluate the optional symbol filename from the command line
very user friendly.  Actually it evaluates nothing at all and tests
the objfile names from the loaded object files just against the
command line parameter using strcmp.  This has two disadvantages.

- If the user types a relative pathname, the maintenance_print_msymbols
  function prints nothing, because object file names are stored with
  absolute pathnames in the objfile structure.

- On Windows based systems, the user might like to enter the filename
  without the .exe suffix.  That would fail as well.

The below patch changes the maintenance_print_msymbols so that the
optional filename is evaluated to an absolute pathname, just to be sure.
If the file doesn't exist, maintenance_print_msymbols will generate an
appropriate error message.

Then it tests the inode number of this file against the inode number
of the object files stored in GDB.  This should be pretty reliable.


Corinna

	* symmisc.c: Include gdb_stat.h.
	(maintenance_print_msymbols): Use inode numbers to compare files.

Index: symmisc.c
===================================================================
RCS file: /cvs/src/src/gdb/symmisc.c,v
retrieving revision 1.34
diff -p -u -r1.34 symmisc.c
--- symmisc.c   12 Jan 2005 18:31:33 -0000      1.34
+++ symmisc.c   24 Jan 2005 20:56:09 -0000
@@ -35,6 +35,7 @@
 #include "bcache.h"
 #include "block.h"
 #include "gdb_regex.h"
+#include "gdb_stat.h"
 #include "dictionary.h"
 
 #include "gdb_string.h"
@@ -930,6 +931,8 @@ maintenance_print_msymbols (char *args, 
   char *symname = NULL;
   struct objfile *objfile;
 
+  struct stat sym_st, obj_st;
+
   dont_repeat ();
 
   if (args == NULL)
@@ -948,7 +951,10 @@ maintenance_print_msymbols (char *args, 
       /* If a second arg is supplied, it is a source file name to match on */
       if (argv[1] != NULL)
 	{
-	  symname = argv[1];
+	  symname = xfullpath (argv[1]);
+	  make_cleanup (xfree, symname);
+	  if (symname && stat (symname, &sym_st))
+	    perror_with_name (symname);
 	}
     }
 
@@ -962,8 +968,9 @@ maintenance_print_msymbols (char *args, 
 
   immediate_quit++;
   ALL_OBJFILES (objfile)
-    if (symname == NULL || strcmp (symname, objfile->name) == 0)
-    dump_msymbols (objfile, outfile);
+    if (symname == NULL
+	|| (!stat (objfile->name, &obj_st) && sym_st.st_ino == obj_st.st_ino))
+      dump_msymbols (objfile, outfile);
   immediate_quit--;
   fprintf_filtered (outfile, "\n\n");
   do_cleanups (cleanups);


-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat, Inc.

Re: [RFA] maintenance_print_msymbols: Try harder to match files

[Daniel Jacobowitz]

On Mon, Jan 24, 2005 at 10:09:31PM +0100, Corinna Vinschen wrote:
> Hi,
> 
> while testing on Cygwin, I found that the maintenance_print_msymbols
> doesn't evaluate the optional symbol filename from the command line
> very user friendly.  Actually it evaluates nothing at all and tests
> the objfile names from the loaded object files just against the
> command line parameter using strcmp.  This has two disadvantages.
> 
> - If the user types a relative pathname, the maintenance_print_msymbols
>   function prints nothing, because object file names are stored with
>   absolute pathnames in the objfile structure.
> 
> - On Windows based systems, the user might like to enter the filename
>   without the .exe suffix.  That would fail as well.
> 
> The below patch changes the maintenance_print_msymbols so that the
> optional filename is evaluated to an absolute pathname, just to be sure.
> If the file doesn't exist, maintenance_print_msymbols will generate an
> appropriate error message.
> 
> Then it tests the inode number of this file against the inode number
> of the object files stored in GDB.  This should be pretty reliable.

The xfullpath call doesn't do what you think it does:

  /* Extract the basename of filename, and return immediately 
     a copy of filename if it does not contain any directory prefix. */
  if (base_name == filename)
    return xstrdup (filename);

I've got no idea at all why that is there.  Joel added this function;
maybe he can explain.

You're comparing solely based on inode number; I think that's an unwise
choice for two reasons:
  - The file may have been replaced on disk; we should use the copy
    that was there when GDB opened it.
  - There are some file systems in which all st_ino values are zero.

If you've got the full path, why not use that?


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] maintenance_print_msymbols: Try harder to match files

[Eli Zaretskii]

> Date: Sun, 1 May 2005 19:37:00 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Joel Brobecker <brobecker@adacore.com>
> 
> You're comparing solely based on inode number; I think that's an unwise
> choice for two reasons:
>   - The file may have been replaced on disk; we should use the copy
>     that was there when GDB opened it.
>   - There are some file systems in which all st_ino values are zero.
> 
> If you've got the full path, why not use that?

Indeed.  symtab.c:lookup_symbol has an example of what I think you
should use (and what AFAIU Daniel had in mind).  Note that file-name
comparison should use FILENAME_CMP instead of strcmp, to avoid
failures on non-Posix systems.

Re: [RFA] maintenance_print_msymbols: Try harder to match files

[Eli Zaretskii]

> Date: Mon, 02 May 2005 22:17:58 +0300
> From: "Eli Zaretskii" <eliz@gnu.org>
> 
> Indeed.  symtab.c:lookup_symbol has an example of what I think you

Oops!  I meant lookup_symtab, of course.  Sorry.

Re: [RFA] maintenance_print_msymbols: Try harder to match files

[Joel Brobecker]

> The xfullpath call doesn't do what you think it does:
> 
>   /* Extract the basename of filename, and return immediately 
>      a copy of filename if it does not contain any directory prefix. */
>   if (base_name == filename)
>     return xstrdup (filename);
> 
> I've got no idea at all why that is there.  Joel added this function;
> maybe he can explain.

I don't remember all the details, I could dig up the relevant messages
if necessary. But the comment at the begining of the function seems to
confirm that this code is correct to be here:

    /* Return a copy of FILENAME, with its directory prefix canonicalized
       by gdb_realpath.  */

Perhaps the function is incorrectly named, though (misleading)...
Should we consider finding a more meaningful name?

-- 
Joel

Re: [RFA] maintenance_print_msymbols: Try harder to match files

[Daniel Jacobowitz]

On Mon, May 02, 2005 at 01:05:15PM -0700, Joel Brobecker wrote:
> > The xfullpath call doesn't do what you think it does:
> > 
> >   /* Extract the basename of filename, and return immediately 
> >      a copy of filename if it does not contain any directory prefix. */
> >   if (base_name == filename)
> >     return xstrdup (filename);
> > 
> > I've got no idea at all why that is there.  Joel added this function;
> > maybe he can explain.
> 
> I don't remember all the details, I could dig up the relevant messages
> if necessary. But the comment at the begining of the function seems to
> confirm that this code is correct to be here:
> 
>     /* Return a copy of FILENAME, with its directory prefix canonicalized
>        by gdb_realpath.  */
> 
> Perhaps the function is incorrectly named, though (misleading)...
> Should we consider finding a more meaningful name?

I would rather see an example of why the current behavior is correct,
first.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] maintenance_print_msymbols: Try harder to match files

[Joel Brobecker]

> > I don't remember all the details, I could dig up the relevant messages
> > if necessary. But the comment at the begining of the function seems to
> > confirm that this code is correct to be here:
> > 
> >     /* Return a copy of FILENAME, with its directory prefix canonicalized
> >        by gdb_realpath.  */
> > 
> > Perhaps the function is incorrectly named, though (misleading)...
> > Should we consider finding a more meaningful name?
> 
> I would rather see an example of why the current behavior is correct,
> first.

I think I remember the whole story now. The description of the problem
xfullpath fixes is at:

        http://sources.redhat.com/ml/gdb-patches/2002-03/msg00345.html

All the followup messages were refreshing... Basically, we're trying
in xfullpath to avoid canonicalizing the basename part of the path.
So, if the name doesn't have any directory part in it, then we're
done.

-- 
Joel