[patch 4/9] TUI: Use internally fullname

[patch 4/9] TUI: Use internally fullname

[Jan Kratochvil]

Hi,

TUI was using comp_dir-relative "filaname" during matching.


Thanks,
Jan


2013-01-15  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* tui/tui-data.c (init_win_info, tui_del_window, tui_free_window):
	Rename field reference filename to fullname.
	* tui/tui-data.h (struct tui_source_info): Rename field filename to
	fullname.  New comment for it.
	* tui/tui-source.c (tui_set_source_content): Rename field reference
	filename to fullname.  Initialize field by symtab_to_fullname now.
	* tui/tui-winsource.c (tui_update_breakpoint_info): Rename field
	reference filename to fullname.  Use symtab_to_fullname during
	comparison.

--- a/gdb/tui/tui-data.c
+++ b/gdb/tui/tui-data.c
@@ -541,7 +541,7 @@ init_win_info (struct tui_win_info *win_info)
       win_info->detail.source_info.gdbarch = NULL;
       win_info->detail.source_info.start_line_or_addr.loa = LOA_ADDRESS;
       win_info->detail.source_info.start_line_or_addr.u.addr = 0;
-      win_info->detail.source_info.filename = 0;
+      win_info->detail.source_info.fullname = NULL;
       break;
     case DATA_WIN:
       win_info->detail.data_display_info.data_content = (tui_win_content) NULL;
@@ -681,10 +681,10 @@ tui_del_window (struct tui_win_info *win_info)
 	  generic_win->handle = (WINDOW *) NULL;
 	  generic_win->is_visible = FALSE;
 	}
-      if (win_info->detail.source_info.filename)
+      if (win_info->detail.source_info.fullname)
         {
-          xfree (win_info->detail.source_info.filename);
-          win_info->detail.source_info.filename = 0;
+          xfree (win_info->detail.source_info.fullname);
+          win_info->detail.source_info.fullname = NULL;
         }
       generic_win = win_info->detail.source_info.execution_info;
       if (generic_win != (struct tui_gen_win_info *) NULL)
@@ -731,10 +731,10 @@ tui_free_window (struct tui_win_info *win_info)
 	  generic_win->handle = (WINDOW *) NULL;
 	}
       tui_free_win_content (generic_win);
-      if (win_info->detail.source_info.filename)
+      if (win_info->detail.source_info.fullname)
         {
-          xfree (win_info->detail.source_info.filename);
-          win_info->detail.source_info.filename = 0;
+          xfree (win_info->detail.source_info.fullname);
+          win_info->detail.source_info.fullname = NULL;
         }
       generic_win = win_info->detail.source_info.execution_info;
       if (generic_win != (struct tui_gen_win_info *) NULL)
--- a/gdb/tui/tui-data.h
+++ b/gdb/tui/tui-data.h
@@ -270,7 +270,10 @@ struct tui_source_info
   struct tui_gen_win_info *execution_info;
   int horizontal_offset;	/* Used for horizontal scroll.  */
   struct tui_line_or_address start_line_or_addr;
-  char *filename;
+
+  /* It is the resolved form as returned by symtab_to_fullname.  */
+  char *fullname;
+
   /* Architecture associated with code at this location.  */
   struct gdbarch *gdbarch;
 };
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -99,9 +99,8 @@ tui_set_source_content (struct symtab *s,
                     xfree (TUI_SRC_WIN->generic.title);
                   TUI_SRC_WIN->generic.title = xstrdup (s->filename);
 
-                  if (src->filename)
-                    xfree (src->filename);
-                  src->filename = xstrdup (s->filename);
+		  xfree (src->fullname);
+		  src->fullname = xstrdup (symtab_to_fullname (s));
 
 		  /* Determine the threshold for the length of the
                      line and the offset to start the display.  */
--- a/gdb/tui/tui-winsource.c
+++ b/gdb/tui/tui-winsource.c
@@ -463,7 +463,8 @@ tui_update_breakpoint_info (struct tui_win_info *win,
 	    {
 	      if ((win == TUI_SRC_WIN
 		   && loc->symtab != NULL
-		   && (filename_cmp (src->filename, loc->symtab->filename) == 0)
+		   && filename_cmp (src->fullname,
+				    symtab_to_fullname (loc->symtab)) == 0
 		   && line->line_or_addr.loa == LOA_LINE
 		   && loc->line_number == line->line_or_addr.u.line_no)
 		  || (win == TUI_DISASM_WIN

Re: [patch 4/9] TUI: Use internally fullname

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> TUI was using comp_dir-relative "filaname" during matching.

Looks good to me.

Jan> -		   && (filename_cmp (src->filename, loc->symtab->filename) == 0)
Jan> +		   && filename_cmp (src->fullname,
Jan> +				    symtab_to_fullname (loc->symtab)) == 0

After your patches is it ever ok to refer directly to symtab->filename?

Tom

Re: [patch 4/9] TUI: Use internally fullname

[Jan Kratochvil]

On Mon, 21 Jan 2013 19:57:31 +0100, Tom Tromey wrote:
> Jan> -		   && (filename_cmp (src->filename, loc->symtab->filename) == 0)
> Jan> +		   && filename_cmp (src->fullname,
> Jan> +				    symtab_to_fullname (loc->symtab)) == 0
> 
> After your patches is it ever ok to refer directly to symtab->filename?

Originally I thought it almost never will so I had renamed "symtab->filename" as
a private one "symtab->filename_".

Only in rare cases one needed to access "filename_" - for example to fetch the
file extension.  symtab_to_fullname would do the whole expensive realpath
business which has no effect on the file extension (+/- BASENAMES_MAY_DIFFER).

But then I found these cases:
@@ -203,7 +203,8 @@ iterate_over_some_symtabs (const char *name,
-      if (compare_filenames_for_search (s->filename, name))
+      if (compare_filenames_for_search (s->filename, name)
+         || compare_filenames_for_search (symtab_to_fullname (s), name))

One cannot use just:
+      if (compare_filenames_for_search (symtab_to_fullname (s), name))

as in the case of S->FILENAME == NAME == "./gdb.base/return.c" it would be
a regression because SYMTAB_TO_FULLNAME == "/gdb/testsuite/gdb.base/return.c".
Formerly S->FILENAME matched NAME but SYMTAB_TO_FULLNAME does not match NAME.

One could also do some "normalization" of NAME, one could strip "./" but for
more complicated cases one cannot do much.

The next step would be to turn various pathname comparisons rather to
st_dev&&st_ino comparisons when possible which should be both faster and more
universal.  But that is left as another patchset; which I am not yet decided
to write now.  It would be a fix to this PR, which seems like a nice speedup:
	http://sourceware.org/bugzilla/show_bug.cgi?id=12332


Thanks,
Jan

Re: [patch 4/9] TUI: Use internally fullname

[Eli Zaretskii]

> Date: Mon, 21 Jan 2013 22:11:21 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: gdb-patches@sourceware.org
> 
> -      if (compare_filenames_for_search (s->filename, name))
> +      if (compare_filenames_for_search (s->filename, name)
> +         || compare_filenames_for_search (symtab_to_fullname (s), name))
> 
> One cannot use just:
> +      if (compare_filenames_for_search (symtab_to_fullname (s), name))
> 
> as in the case of S->FILENAME == NAME == "./gdb.base/return.c" it would be
> a regression because SYMTAB_TO_FULLNAME == "/gdb/testsuite/gdb.base/return.c".
> Formerly S->FILENAME matched NAME but SYMTAB_TO_FULLNAME does not match NAME.
> 
> One could also do some "normalization" of NAME, one could strip "./" but for
> more complicated cases one cannot do much.

Why strip it?  We could resolve "." to an absolute file name using the
compilation directory, and then they will match, right?

> The next step would be to turn various pathname comparisons rather to
> st_dev&&st_ino comparisons when possible which should be both faster and more
> universal.

That will break on Windows, unless we replace the library
implementation of 'stat' and 'fstat', or provide a method for
retrieving just the inode and device number (which could be
implemented on Windows using available APIs, such as
GetFileInformationByHandle).

Re: [patch 4/9] TUI: Use internally fullname

[Jan Kratochvil]

On Tue, 22 Jan 2013 07:55:10 +0100, Eli Zaretskii wrote:
> > Date: Mon, 21 Jan 2013 22:11:21 +0100
> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > Cc: gdb-patches@sourceware.org
> > 
> > -      if (compare_filenames_for_search (s->filename, name))
> > +      if (compare_filenames_for_search (s->filename, name)
> > +         || compare_filenames_for_search (symtab_to_fullname (s), name))
> > 
> > One cannot use just:
> > +      if (compare_filenames_for_search (symtab_to_fullname (s), name))
> > 
> > as in the case of S->FILENAME == NAME == "./gdb.base/return.c" it would be
> > a regression because SYMTAB_TO_FULLNAME == "/gdb/testsuite/gdb.base/return.c".
> > Formerly S->FILENAME matched NAME but SYMTAB_TO_FULLNAME does not match NAME.
> > 
> > One could also do some "normalization" of NAME, one could strip "./" but for
> > more complicated cases one cannot do much.
> 
> Why strip it?  We could resolve "." to an absolute file name using the
> compilation directory, and then they will match, right?

That can be done for S->FILENAME and that is already done by
symtab_to_fullname.

But for NAME - the string which user entered in "break ./gdb.base/return.c:main"
- has no compilation nor "current" directory.

I think the only perfect solution - which works also for symbolic links and ..
references such as in the case:
  cd gdb
  ln -s testsuite symlinked
then:
  break symlinked/gdb.base/return.c
would be to try resolving the name from each directory of each file being
evaluated for a match:
  /./gdb.base/return.c
  /gdb/./gdb.base/return.c
  /gdb/testsuite/./gdb.base/return.c
But that would be too slow and I do not think anyone would ever use it.

So far I hope this double-match of both s->filename and symtab_to_fullname (s)
is good enough for any real world case.


> > The next step would be to turn various pathname comparisons rather to
> > st_dev&&st_ino comparisons when possible which should be both faster and more
> > universal.
> 
> That will break on Windows, unless we replace the library
> implementation of 'stat' and 'fstat', or provide a method for
> retrieving just the inode and device number (which could be
> implemented on Windows using available APIs, such as
> GetFileInformationByHandle).

I am aware of:
separate_debug_file_exists
     Some operating systems, e.g. Windows, do not provide a meaningful
     st_ino; they always set it to zero.  (Windows does provide a
     meaningful st_dev.)  Do not indicate a duplicate library in that
     case.  While there is no guarantee that a system that provides

I expected to fall back to the current realpath strings matching in the
MS-Windows ST_INO == 0 case.  But when you point at GetFileInformationByHandle
I see at
	http://msdn.microsoft.com/en-us/library/windows/desktop/aa363788%28v=vs.85%29.aspx
	dwVolumeSerialNumber
	nFileIndexHigh
	nFileIndexLow
	The identifier (low and high parts) and the volume serial number
	uniquely identify a file on a single computer. To determine whether
	two open handles represent the same file, combine the identifier and
	the volume serial number for each file and compare them.

So I do not understand why MS-Windows stat call does not provide st_ino from
those fields.  I expect it is just a MS-Windows stat implementation bug
probably workarounded in Cygwin but apparently not in MinGW?


Thanks,
Jan

Re: [patch 4/9] TUI: Use internally fullname

[Eli Zaretskii]

> Date: Tue, 22 Jan 2013 08:12:05 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: tromey@redhat.com, gdb-patches@sourceware.org
> 
> > Why strip it?  We could resolve "." to an absolute file name using the
> > compilation directory, and then they will match, right?
> 
> That can be done for S->FILENAME and that is already done by
> symtab_to_fullname.
> 
> But for NAME - the string which user entered in "break ./gdb.base/return.c:main"
> - has no compilation nor "current" directory.

What is the semantics of such a 'break' command?  Does "." here mean
the current directory?  Or does it mean something else?

If the former, then we have the current directory in GDB, don't we?

> 	http://msdn.microsoft.com/en-us/library/windows/desktop/aa363788%28v=vs.85%29.aspx
> 	dwVolumeSerialNumber
> 	nFileIndexHigh
> 	nFileIndexLow
> 	The identifier (low and high parts) and the volume serial number
> 	uniquely identify a file on a single computer. To determine whether
> 	two open handles represent the same file, combine the identifier and
> 	the volume serial number for each file and compare them.
> 
> So I do not understand why MS-Windows stat call does not provide st_ino from
> those fields.

Because MS didn't bother to code that.

> I expect it is just a MS-Windows stat implementation bug probably
> workarounded in Cygwin but apparently not in MinGW?

Cygwin doesn't use the Windows runtime, it uses its own library.

MinGW does use the Windows runtime, and doesn't have a replacement for
'stat' in its "mingwex" library.  One complication with such a
replacement is that the FileIndex thing is a 64-bit quantity, while
'struct stat' in MS implementation uses a 'short' data type for it.
This creates binary incompatibility.  That's why I suggested a
separate method for retrieving the inode.

Re: [patch 4/9] TUI: Use internally fullname

[Jan Kratochvil]

On Tue, 22 Jan 2013 09:24:59 +0100, Eli Zaretskii wrote:
> > But for NAME - the string which user entered in "break ./gdb.base/return.c:main"
> > - has no compilation nor "current" directory.
> 
> What is the semantics of such a 'break' command?  Does "." here mean
> the current directory?  Or does it mean something else?

It is compilation directory (DW_AT_comp_dir) relative name (therefore CU's
DW_AT_name).

Moreover recent GDBs allow to use also any trailing part, therefore not just
"break ./gdb.base/return.c:main" and "break return.c:main"
but also "break gdb.base/return.c".


> If the former, then we have the current directory in GDB, don't we?

Current directory of GDB can be somewhere completely else, it is not related.


I just try not to regress any existing case while fully fixing the absolute
filenames.  I do not remember symlinks used in source trees so I consider
those only as a backward compatibility relict.  And in this regard it can be
all further extended.  Attachment to a C++ applications shows 26000 source
files from all its tens/hundreds libraries so using relative source filenames
is rather a pain.  I already got hit by false breakpoint hits due to an
accidental source filename match.


> > So I do not understand why MS-Windows stat call does not provide st_ino from
> > those fields.
> 
> Because MS didn't bother to code that.

BTW Free wine32 also has st_ino == 0; I understand it needs to be compatible.
Free wine64 (with 64-bit PE32+ executable) also behaves the same.


> That's why I suggested a separate method for retrieving the inode.

Thanks for the suggestion, it may match better (+it would be faster also on
MS-Windows) than the filename strings fallback.


Jan

Re: [patch 4/9] TUI: Use internally fullname

[Eli Zaretskii]

> Date: Tue, 22 Jan 2013 09:46:48 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: tromey@redhat.com, gdb-patches@sourceware.org
> 
> On Tue, 22 Jan 2013 09:24:59 +0100, Eli Zaretskii wrote:
> > > But for NAME - the string which user entered in "break ./gdb.base/return.c:main"
> > > - has no compilation nor "current" directory.
> > 
> > What is the semantics of such a 'break' command?  Does "." here mean
> > the current directory?  Or does it mean something else?
> 
> It is compilation directory (DW_AT_comp_dir) relative name (therefore CU's
> DW_AT_name).

But then it means that directory is somewhere we could get at it,
right?  And yet you said "has no compilation directory".  What am I
missing?

> Moreover recent GDBs allow to use also any trailing part, therefore not just
> "break ./gdb.base/return.c:main" and "break return.c:main"
> but also "break gdb.base/return.c".

It sounds strange to me to use "." to mean anything but GDB's current
directory, but if that's a long living tradition, so be it.

> > > So I do not understand why MS-Windows stat call does not provide st_ino from
> > > those fields.
> > 
> > Because MS didn't bother to code that.
> 
> BTW Free wine32 also has st_ino == 0; I understand it needs to be compatible.
> Free wine64 (with 64-bit PE32+ executable) also behaves the same.

Compatibility is a virulent disease.

Re: [patch 4/9] TUI: Use internally fullname

[Jan Kratochvil]

On Tue, 22 Jan 2013 12:44:30 +0100, Eli Zaretskii wrote:
> > It is compilation directory (DW_AT_comp_dir) relative name (therefore CU's
> > DW_AT_name).
> 
> But then it means that directory is somewhere we could get at it,
> right?  And yet you said "has no compilation directory".  What am I
> missing?

It is doable but it is too expensive to resolve all the possible combinations.

The compilation directory is (possibly) different for every CU (~= symtab).
And due to the possibility of choosing arbitrary number of trailing path
components you need to resolve it for each one.

With 25000 source files, each one having approx. 10 path components, even
after the not yet posted dropped xfullpath optimization it means 2500000 stat
syscalls. This is 0.744s on my box just for the stat syscalls overhead, in GDB
it would sure be higher.

Sure this is all possible but I believe it is an add-on/different patchset
than this one, it is appropriate to cache all the info some way.
This patchset tries to fix some of the found bugs and rename some confusing
variables/fields in GDB.


> > Moreover recent GDBs allow to use also any trailing part, therefore not just
> > "break ./gdb.base/return.c:main" and "break return.c:main"
> > but also "break gdb.base/return.c".
> 
> It sounds strange to me to use "." to mean anything but GDB's current
> directory, but if that's a long living tradition, so be it.

I have noticed it because whole testsuite uses the ./gdb.base/return.c names.


Thanks,
Jan

Re: [patch 4/9] TUI: Use internally fullname

[Eli Zaretskii]

> Date: Tue, 22 Jan 2013 13:05:11 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: tromey@redhat.com, gdb-patches@sourceware.org
> 
> Sure this is all possible but I believe it is an add-on/different patchset
> than this one, it is appropriate to cache all the info some way.

Fair enough.