[TUI] correctly display windows source files

[TUI] correctly display windows source files

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]

Hello,

The TUI displays windows source files with and extra blank line between 
each source line.
Line number are wrong and changes when source is moved up and down.

This patch fixes this problem and should take the case of MAC OS 
end-of-line into account.

Denis

[-- Attachment #2: tui-source.c.patch --]
[-- Type: text/plain, Size: 768 bytes --]

2006-10-09  Denis Pilat  <denis.pilat@st.com>

	* tui-source.c (tui_set_source_content): handle source files that 
	contain non unix end-of-line.


Index: tui/tui-source.c
===================================================================
--- tui/tui-source.c	(revision 528)
+++ tui/tui-source.c	(working copy)
@@ -194,6 +194,13 @@ tui_set_source_content (struct symtab *s
                                            chars until we do  */
 				  while (c != EOF && c != '\n' && c != '\r')
 				    c = fgetc (stream);
+				  /* Handle non-'\n' end-of-line.  */
+				  if (c == '\r' && (c = fgetc (stream)) != '\n')
+				    {
+				       ungetc (c, stream);
+				       c = '\r';
+				    }
+				  
 				}
 			    }
 			  while (c != EOF && c != '\n' && c != '\r' &&

Re: [TUI] correctly display windows source files

[Mark Kettenis]

> Date: Mon, 09 Oct 2006 17:21:41 +0200
> From: Denis PILAT <denis.pilat@st.com>
> 
> Hello,
> 
> The TUI displays windows source files with and extra blank line between 
> each source line.
> Line number are wrong and changes when source is moved up and down.
> 
> This patch fixes this problem and should take the case of MAC OS 
> end-of-line into account.

Hmm, I always get nervous when I see ungetc() calls.  However, we
already use it in source.c:print_source_line_base() so I guess it is
ok.  However, is there any reason why this bit of code is different
from the code in that function?

> 
> 2006-10-09  Denis Pilat  <denis.pilat@st.com>
> 
> 	* tui-source.c (tui_set_source_content): handle source files that 
> 	contain non unix end-of-line.
> 

Re: [TUI] correctly display windows source files

[Eli Zaretskii]

> Date: Mon, 09 Oct 2006 17:21:41 +0200
> From: Denis PILAT <denis.pilat@st.com>
> 
> The TUI displays windows source files with and extra blank line between 
> each source line.
> Line number are wrong and changes when source is moved up and down.
> 
> This patch fixes this problem and should take the case of MAC OS 
> end-of-line into account.

This is okay with me, but please be sure to test the code near the end
of the file, because, according to my references, an attempt to ungetc
EOF is ignored.  What happens with a file that ends in a sole `\r'?

Re: [TUI] correctly display windows source files

[Denis PILAT]

Eli Zaretskii wrote:

>>Date: Mon, 09 Oct 2006 17:21:41 +0200
>>From: Denis PILAT <denis.pilat@st.com>
>>
>>The TUI displays windows source files with and extra blank line between 
>>each source line.
>>Line number are wrong and changes when source is moved up and down.
>>
>>This patch fixes this problem and should take the case of MAC OS 
>>end-of-line into account.
>>    
>>
>
>This is okay with me, but please be sure to test the code near the end
>of the file, because, according to my references, an attempt to ungetc
>EOF is ignored.  What happens with a file that ends in a sole `\r'that?
>
>  
>
I've tried files with  '\r' end-of-lines but there are problems to open 
them, in source.c:find_source_lines, only \n are taken as end-of-line.
Therefore the struct symtab given as an input parameter of 
tui_set_source_content() function is wrong, that prevents the file to be 
read correctly.

I don't want to mix this patch to a patch that may fix the problem in 
source.c. I'll have a look at that anyway but I guess there could be a 
lot of problem elsewhere to open '\r' only end-of-line files.


About your remark,
I've tried to ungetc(EOF) and I did not get any problem, after 
ungetc(EOF) the next call to fgetc is still EOF.

Denis

'\r' only end-of-line

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

As discussed in thread 
http://sources.redhat.com/ml/gdb-patches/2006-10/msg00090.html
about the TUI that did not display windows source files,

I propose a patch that treats source files where end-of-line are '\r' only.

The struct symtab fulfilled by find_source_lines was wrong, the field 
nlines was set to 1.
Therefore these files can not be open in the TUI for instance.

Denis



[-- Attachment #2: source.c.patch --]
[-- Type: text/plain, Size: 987 bytes --]

2006-10-10  Denis PILAT  <denis.pilat@st.com>

	* source.c (find_source_lines): Treat the case of source 
	file whith '\r' only end-of-line.

Index: source.c
===================================================================
--- source.c	(revision 528)
+++ source.c	(working copy)
@@ -1074,7 +1074,11 @@ find_source_lines (struct symtab *s, int
     nlines = 1;
     while (p != end)
       {
-	if (*p++ == '\n'
+ 	/* Skip the \r in case of \r\n end-of-line.  */
+ 	if (*p == '\r' && *(p + 1) == '\n')
+ 	    p++;
+ 	/* Lines could end with '\n' or '\r' only.  */
+	if ((*p == '\n' || *p == '\r')
 	/* A newline at the end does not start a new line.  */
 	    && p != end)
 	  {
@@ -1085,8 +1089,9 @@ find_source_lines (struct symtab *s, int
 		  (int *) xrealloc ((char *) line_charpos,
 				    sizeof (int) * lines_allocated);
 	      }
-	    line_charpos[nlines++] = p - data;
+	    line_charpos[nlines++] = p + 1 - data;
 	  }
+	p++;
       }
     do_cleanups (old_cleanups);
   }

Re: '\r' only end-of-line

[Eli Zaretskii]

> Date: Tue, 10 Oct 2006 17:08:23 +0200
> From: Denis PILAT <denis.pilat@st.com>
> Cc: Eli Zaretskii <eliz@gnu.org>
> 
> As discussed in thread 
> http://sources.redhat.com/ml/gdb-patches/2006-10/msg00090.html
> about the TUI that did not display windows source files,
> 
> I propose a patch that treats source files where end-of-line are '\r' only.

I think part of your patch is not needed: the Windows port of GDB
should use the LSEEK_NOT_LINEAR branch of the code in
find_source_lines, so the code you are patching doesn't need to
consider the \r\n case.

As for the \r case, don't such files fail in many more places?  It
looks like you need to test for \r in every place where we currently
test for \n, or am I missing something?

Re: '\r' only end-of-line

[Denis PILAT]

Eli Zaretskii wrote:

>>Date: Tue, 10 Oct 2006 17:08:23 +0200
>>From: Denis PILAT <denis.pilat@st.com>
>>Cc: Eli Zaretskii <eliz@gnu.org>
>>
>>As discussed in thread 
>>http://sources.redhat.com/ml/gdb-patches/2006-10/msg00090.html
>>about the TUI that did not display windows source files,
>>
>>I propose a patch that treats source files where end-of-line are '\r' only.
>>
>
>I think part of your patch is not needed: the Windows port of GDB
>should use the LSEEK_NOT_LINEAR branch of the code in
>find_source_lines, so the code you are patching doesn't need to
>consider the \r\n case.
>
Yes but when you use a linux hosted compiler for reading windows source 
file
(sorry for that weirdness but we sometime have common source file for 
both OS),
then this case may happen.

>
>
>As for the \r case, don't such files fail in many more places?  It
>looks like you need to test for \r in every place where we currently
>test for \n, or am I missing something?
>
>
Well, I thought reading your previous email on the other patch you just 
accepted
( "What happens with a file that ends  in a sole `\r'?"),
that MacOs files were supported by GDB. So I did some tests
with such a file and found that there were a problem in the TUI that
did not allow user to scroll these kind of source files. There are also
problem in gdb command line. Both tell that the source file contains 
only 1 line.

My patch just fixes this problem and allow scrolling of sources file in 
the TUI, not more.

It does NOT aims at globally supporting MACOS files , as you said, there 
should
be plenty of failures elsewhere, and there are.
I don't want to go into such a yard
so let's forget about my patch if you think it's useless.

Denis

Re: '\r' only end-of-line

[Daniel Jacobowitz]

On Wed, Oct 11, 2006 at 12:08:07PM +0200, Denis PILAT wrote:
> >I think part of your patch is not needed: the Windows port of GDB
> >should use the LSEEK_NOT_LINEAR branch of the code in
> >find_source_lines, so the code you are patching doesn't need to
> >consider the \r\n case.
> >
> Yes but when you use a linux hosted compiler for reading windows source 
> file
> (sorry for that weirdness but we sometime have common source file for 
> both OS),
> then this case may happen.

I was just thinking about this the other day...

As a general position, are we willing to support things written /
compiled on other hosts?  This includes both line endings and
pathnames.  I hope the answer is yes, but I don't know.

At CodeSourcery, as we've said before, we ship a native Windows GDB
and GCC.  But for a lot of things we have no way to test the native
Windows GDB - can't use Cygwin's expect to drive it.  So I found myself
testing a Cygwin GDB using a customer's Windows-only SDK.  And the
debugger got very, very unhappy because there's a filename comparison
in the dwarf reader; one of the arguments came from the compiler as
"\foo\bar.h" and the other came in two pieces "\foo" and "bar.h".  GDB
assembled that as "\foo/bar.h" and FILENAME_CMP returned false.

For my local testing I wrote a filename comparison function that
allowed windows slashes.  I didn't need to worry about case sensitivity
here, because Windows tools are generally case-preserving unless you
feed them input with strange casing.  This was sort of unpleasant, but
useful.  And in thinking about it, I think I'd rather have this return
true than false:
  FILENAME_CMP ("a\\b", "a/b")

Since almost no one puts backslashes in Unix filenames (they're a
mental health hazard), and having a\\b and a/b is a truly improbable
case.

What do y'all think?  Would a gdb_filename_cmp with this behavior be
OK?

-- 
Daniel Jacobowitz
CodeSourcery

Re: '\r' only end-of-line

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> At CodeSourcery, as we've said before, we ship a native Windows GDB
> and GCC.  But for a lot of things we have no way to test the native
> Windows GDB - can't use Cygwin's expect to drive it.

Huh? I can and do.

Admittedly I haven't got the Dejagnu testsuite running, but that's just 
because Cygwin's Dejagnu is too old and I haven't had time to do 
anything about it.

Expect is also quite old, at least in the not-so-recent Cygwin I have 
here, but it runs my own tests quite happily as long as I keep the 
regular expressions simple.

Andrew

Re: '\r' only end-of-line

[Daniel Jacobowitz]

On Wed, Oct 11, 2006 at 02:57:01PM +0100, Andrew STUBBS wrote:
> Daniel Jacobowitz wrote:
> >At CodeSourcery, as we've said before, we ship a native Windows GDB
> >and GCC.  But for a lot of things we have no way to test the native
> >Windows GDB - can't use Cygwin's expect to drive it.
> 
> Huh? I can and do.
> 
> Admittedly I haven't got the Dejagnu testsuite running, but that's just 
> because Cygwin's Dejagnu is too old and I haven't had time to do 
> anything about it.

I am pretty sure you'll get nasty lossage from, e.g., GDB thinking it's
not connected to a terminal.  Error messages will show up after
prompts, and questions will be suppressed.

Actually, if those are the only two, we could probably make the mingw32
GDB cope.  Joel posted a patch once to set the buffering.  And I could
probably put together a GDB-specific isatty that could tell if we were
connected to a Cygwin terminal - I know how to do that.

Interesting idea... doesn't really change my question in the previous
message, but a very interesting idea.

-- 
Daniel Jacobowitz
CodeSourcery

Re: '\r' only end-of-line

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> I am pretty sure you'll get nasty lossage from, e.g., GDB thinking it's
> not connected to a terminal.  Error messages will show up after
> prompts, and questions will be suppressed.

Yes, in the absence of a better solution, we have gdb_has_a_terminal 
always return 1 for MinGW. It means that it doesn't work right for 
pipes, but without this it does not work right in normal interactive 
mode, which is worse.

As far as I am aware this is a normal MinGW problem, not limited to use 
with Expect.

Andrew Stubbs

Re: '\r' only end-of-line

[Daniel Jacobowitz]

On Wed, Oct 11, 2006 at 03:42:06PM +0100, Andrew STUBBS wrote:
> Daniel Jacobowitz wrote:
> >I am pretty sure you'll get nasty lossage from, e.g., GDB thinking it's
> >not connected to a terminal.  Error messages will show up after
> >prompts, and questions will be suppressed.
> 
> Yes, in the absence of a better solution, we have gdb_has_a_terminal 
> always return 1 for MinGW. It means that it doesn't work right for 
> pipes, but without this it does not work right in normal interactive 
> mode, which is worse.
> 
> As far as I am aware this is a normal MinGW problem, not limited to use 
> with Expect.

It works fine in a Windows command shell without this, which is how we
currently use it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: '\r' only end-of-line

[Eli Zaretskii]

> Date: Wed, 11 Oct 2006 09:32:54 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org
> 
> As a general position, are we willing to support things written /
> compiled on other hosts?  This includes both line endings and
> pathnames.  I hope the answer is yes, but I don't know.

Well, _my_ answer is a firm YES, and whoever volunteers to make that
happen will get all the support I can provide in my admittedly limited
time.

> debugger got very, very unhappy because there's a filename comparison
> in the dwarf reader; one of the arguments came from the compiler as
> "\foo\bar.h" and the other came in two pieces "\foo" and "bar.h".  GDB
> assembled that as "\foo/bar.h" and FILENAME_CMP returned false.

IMHO, it's fundamentally wrong to compare file names as strings;
FILENAME_CMP is just a band-aid that I introduced as a stop-gap.  We
could (and probably should) do better, especially if situations like
above happen.

Alternatively, we could force GDB to canonicalize all slashes in its
symbol tables, and then FILENAME_CMP will work just fine.  But this is
less reliable, especially in the face of file names that come from
outside GDB, like from the command line etc.

> Would a gdb_filename_cmp with this behavior be OK?

I'd support that, but we need to talk a bit about what it should do,
as there are a few complications.

Re: '\r' only end-of-line

[Eli Zaretskii]

> Date: Wed, 11 Oct 2006 12:08:07 +0200
> From: Denis PILAT <denis.pilat@st.com>
> Cc: gdb-patches@sourceware.org
> >
> >As for the \r case, don't such files fail in many more places?  It
> >looks like you need to test for \r in every place where we currently
> >test for \n, or am I missing something?
> >
> >
> Well, I thought reading your previous email on the other patch you just 
> accepted
> ( "What happens with a file that ends  in a sole `\r'?"),
> that MacOs files were supported by GDB. So I did some tests
> with such a file and found that there were a problem in the TUI that
> did not allow user to scroll these kind of source files. There are also
> problem in gdb command line. Both tell that the source file contains 
> only 1 line.
> 
> My patch just fixes this problem and allow scrolling of sources file in 
> the TUI, not more.
> 
> It does NOT aims at globally supporting MACOS files , as you said, there 
> should
> be plenty of failures elsewhere, and there are.
> I don't want to go into such a yard
> so let's forget about my patch if you think it's useless.

It's not useless to make GDB seamlessly work with all 3 types of
end-of-line formats, but that's a lot of work, so if you don't have
the resources to do that, it doesn't make sense to fix only this one
place, because, as you saw, it will fail in many others.

Re: '\r' only end-of-line

[Denis PILAT]

>
>
>It's not useless to make GDB seamlessly work with all 3 types of
>end-of-line formats, but that's a lot of work, so if you don't have
>the resources to do that, it doesn't make sense to fix only this one
>place, because, as you saw, it will fail in many others.
>
>  
>
I don't have time for implementing this support so let's  cancel this.

Denis

Re: [TUI] correctly display windows source files

[Eli Zaretskii]

> Date: Tue, 10 Oct 2006 15:23:05 +0200
> From: Denis PILAT <denis.pilat@st.com>
> Cc: gdb-patches@sourceware.org
> 
> About your remark,
> I've tried to ungetc(EOF) and I did not get any problem, after 
> ungetc(EOF) the next call to fgetc is still EOF.

That could be so, but I'd prefer that our code didn't do anything that
could fail, even theoretically.  We shouldn't call functions with
arguments that invoke undefined or uncertain behavior.

It should be a simple matter of adding a test for EOF.

Re: [TUI] correctly display windows source files

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

>
>
>That could be so, but I'd prefer that our code didn't do anything that
>could fail, even theoretically.  We shouldn't call functions with
>arguments that invoke undefined or uncertain behavior.
>
>It should be a simple matter of adding a test for EOF.
>
You're right, I can not do testing on all hosts and libc.
Attached a new proposal.

In the case of "'\r'EOF" we keep EOF in "c" variable and we exit the 
while just below anyway.

Denis

[-- Attachment #2: tui-source.c.patch --]
[-- Type: text/plain, Size: 789 bytes --]

2006-10-09  Denis Pilat  <denis.pilat@st.com>

	* tui-source.c (tui_set_source_content): handle source files that 
	contain non unix end-of-line.

Index: tui/tui-source.c
===================================================================
--- tui/tui-source.c	(revision 529)
+++ tui/tui-source.c	(working copy)
@@ -194,6 +194,14 @@ tui_set_source_content (struct symtab *s
                                            chars until we do  */
 				  while (c != EOF && c != '\n' && c != '\r')
 				    c = fgetc (stream);
+				  /* Handle non-'\n' end-of-line.  */
+				  if (c == '\r' &&
+				     (c = fgetc (stream)) != '\n' && c != EOF)
+				    {
+				       ungetc (c, stream);
+				       c = '\r';
+				    }
+				  
 				}
 			    }
 			  while (c != EOF && c != '\n' && c != '\r' &&

Re: [TUI] correctly display windows source files

[Eli Zaretskii]

> Date: Wed, 11 Oct 2006 10:01:48 +0200
> From: Denis PILAT <denis.pilat@st.com>
> Cc: gdb-patches@sourceware.org
> >
> >It should be a simple matter of adding a test for EOF.
> >
> You're right, I can not do testing on all hosts and libc.
> Attached a new proposal.
> 
> In the case of "'\r'EOF" we keep EOF in "c" variable and we exit the 
> while just below anyway.

This new patch is okay with me.  Thanks.

Re: [TUI] correctly display windows source files

[Frederic RISS]

On Wed, 2006-10-11 at 04:39 -0400, Eli Zaretskii wrote:
> > Date: Wed, 11 Oct 2006 10:01:48 +0200
> > From: Denis PILAT <denis.pilat@st.com>
> > Attached a new proposal.
> > 
> > In the case of "'\r'EOF" we keep EOF in "c" variable and we exit the 
> > while just below anyway.
> 
> This new patch is okay with me.  Thanks.

I checked this in on Denis' behalf.

Fred.