gdb v7.0 - user defined command's document section - space prefixed end

gdb v7.0 - user defined command's document section - space prefixed end

[James Pandavan]

Hi,

I noticed some of my user defined commands defined in '.gdbinit' file 
stopped working in gdb v7.0. They used to work in gdb v6.8. In trying to 
troubleshoot, I went through the gdb source code.

In gdb v6.8, the init file processing logic was in 
cli/cli-script.c:read_next_line(). The following code ignores all 
leading (and trailing) spaces.

   841    /* Strip leading and trailing whitespace.  */
   842    while (*p == ' ' || *p == '\t')
   843      p++;
   844 
   845    p1 = p + strlen (p);
   846    while (p1 != p && (p1[-1] == ' ' || p1[-1] == '\t'))
   847      p1--;

In gdb v7.0, the processing logic was in 
cli/cli-script.c:process_next_line(). Apparently, the leading spaces are 
ignored only for commands and not for comments. So, if I had a comment 
section with and 'end' having prefixed spaces, gdb did not treat it as 
end of comment section.

   888    if (parse_commands)
   889      {
   890        /* Strip leading whitespace.  */
   891        while (*p == ' ' || *p == '\t')
   892          p++;
   893      }

So, removing all leading white spaces in line containing 'end' fixed my 
problem for v7.0.

Assuming v7.0's has the correct and intended behavior (as it appears to 
be so), shouldn't the document mention it?
(http://sourceware.org/gdb/current/onlinedocs/gdb_24.html#SEC249) Or am 
I referring to wrong document?

Thanks,
James Pandavan


My gdbinit 
file------------------------------------------------------------------------------------------------------------

|define enable_only
    disable
    enable $arg0
    end
document enable_only
   enables only the given breakpoint
   end

define sstep
    finish
    step
    end
|

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Joel Brobecker]

> Assuming v7.0's has the correct and intended behavior (as it appears to  
> be so), shouldn't the document mention it?
> (http://sourceware.org/gdb/current/onlinedocs/gdb_24.html#SEC249) Or am  
> I referring to wrong document?

I'm not sure whether this change of behavior was intended or not. I wonder
how many users indent their GDB scripts the way you did, with the "end"
not being on the first column...

For now, perhaps we should document the fact that we expect the "end"
keyword to be on the first column of the line. I'm also wondering
whether we should consider allowing white spaces before the end in
the comment as well.  It would make us backwards-compatible with
previous versions.

-- 
Joel

Re: gdb v7.0 - user defined command's document section - space prefixed end

[James Pandavan]

Joel Brobecker wrote:
> I'm not sure whether this change of behavior was intended or not. I wonder
> how many users indent their GDB scripts the way you did, with the "end"
> not being on the first column...
>
> For now, perhaps we should document the fact that we expect the "end"
> keyword to be on the first column of the line. I'm also wondering
> whether we should consider allowing white spaces before the end in
> the comment as well.  It would make us backwards-compatible with
> previous versions.
>
>   
I noticed my scripts didn't work under v7.0 only when I saw this bug in 
launchpad. I guess I am not the only one who has indented "ends" this way :)
https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/461594

I've also filed a bug report a day after I sent my original mail.
http://sourceware.org/bugzilla/show_bug.cgi?id=10868

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Joel Brobecker]

> I noticed my scripts didn't work under v7.0 only when I saw this bug in  
> launchpad. I guess I am not the only one who has indented "ends" this way 
> :)
> https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/461594

If you could maybe help us a little bit, and track down the author
of the patch that introduced the change of behavior, and then ask
him whether the change was intended?

Thanks,
-- 
Joel

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Daniel Jacobowitz]

On Tue, Nov 03, 2009 at 11:43:13AM -0800, Joel Brobecker wrote:
> > I noticed my scripts didn't work under v7.0 only when I saw this bug in  
> > launchpad. I guess I am not the only one who has indented "ends" this way 
> > :)
> > https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/461594
> 
> If you could maybe help us a little bit, and track down the author
> of the patch that introduced the change of behavior, and then ask
> him whether the change was intended?

I believe it was:

2009-08-03  Jim Ingham  <jingham@apple.com>
            Vladimir Prus  <vladimir@codesourcery.com>

        Refactor reading of commands

        * defs.h (read_command_lines_1): Declare.
        * cli/cli-script.c (read_next_line): Only return string,
        do not process.
        (process_next_line): New, extracted from read_next_line.
        (recurse_read_control_structure): Take a function pointer to the
        read function.
        (get_command_line) Pass the read_next_line as reader function
        into recurse_read_control_structure.
        (read_command_lines_1): New, extracted from...
        (read_command_lines): ...here.

-- 
Daniel Jacobowitz
CodeSourcery

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Jim Ingham]

This patch has my and Vladimir's names on it, presumably Vladimir was porting some of the Apple local changes over to the FSF gdb.  But in our version of gdb we unconditionally strip leading whitespace, and spaces before "end" don't matter.  So sadly I can't help explain the reasoning behind the change.  If this is indeed the right patch, you'll have to bug Vladimir for the motivation.

Jim

On Nov 3, 2009, at 11:51 AM, Daniel Jacobowitz wrote:

> On Tue, Nov 03, 2009 at 11:43:13AM -0800, Joel Brobecker wrote:
>>> I noticed my scripts didn't work under v7.0 only when I saw this bug in  
>>> launchpad. I guess I am not the only one who has indented "ends" this way 
>>> :)
>>> https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/461594
>> 
>> If you could maybe help us a little bit, and track down the author
>> of the patch that introduced the change of behavior, and then ask
>> him whether the change was intended?
> 
> I believe it was:
> 
> 2009-08-03  Jim Ingham  <jingham@apple.com>
>            Vladimir Prus  <vladimir@codesourcery.com>
> 
>        Refactor reading of commands
> 
>        * defs.h (read_command_lines_1): Declare.
>        * cli/cli-script.c (read_next_line): Only return string,
>        do not process.
>        (process_next_line): New, extracted from read_next_line.
>        (recurse_read_control_structure): Take a function pointer to the
>        read function.
>        (get_command_line) Pass the read_next_line as reader function
>        into recurse_read_control_structure.
>        (read_command_lines_1): New, extracted from...
>        (read_command_lines): ...here.
> 
> -- 
> Daniel Jacobowitz
> CodeSourcery

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Tue, Nov 03, 2009 at 11:43:13AM -0800, Joel Brobecker wrote:
>> > I noticed my scripts didn't work under v7.0 only when I saw this bug in
>> > launchpad. I guess I am not the only one who has indented "ends" this way
>> > :)
>> > https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/461594
>> 
>> If you could maybe help us a little bit, and track down the author
>> of the patch that introduced the change of behavior, and then ask
>> him whether the change was intended?
> 
> I believe it was:
> 
> 2009-08-03  Jim Ingham  <jingham@apple.com>
>             Vladimir Prus  <vladimir@codesourcery.com>
> 
>         Refactor reading of commands
> 
>         * defs.h (read_command_lines_1): Declare.
>         * cli/cli-script.c (read_next_line): Only return string,
>         do not process.
>         (process_next_line): New, extracted from read_next_line.
>         (recurse_read_control_structure): Take a function pointer to the
>         read function.
>         (get_command_line) Pass the read_next_line as reader function
>         into recurse_read_control_structure.
>         (read_command_lines_1): New, extracted from...
>         (read_command_lines): ...here.

I don't think that the patch associated to the above changelog entry
is directly responsible -- consider:

-      val = read_next_line (&next, current_cmd->control_type != python_control);
+      val = process_next_line (read_next_line_func (), &next, 
+                              current_cmd->control_type != python_control);

read_next_line already used to took process_commands parameter, and process_next_line
was nothing but "Extract Function" refactoring, so it did not change that.

The process_commands parameter was added to read_next_line for exactly the reason
Tom suggested -- to support Python. When parsing 'python' command we need to skip
all the way to the 'end' without stripping indentation. And when not parsing 'python'
command, process_commands should be set. Therefore, here's the patch that should fix
this problem.  Grep claims no other problematic places.

OK?

- Volodya

Index: gdb/cli/cli-script.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
retrieving revision 1.53
diff -u -p -r1.53 cli-script.c
--- gdb/cli/cli-script.c        3 Aug 2009 12:26:37 -0000       1.53
+++ gdb/cli/cli-script.c        6 Nov 2009 16:40:12 -0000
@@ -1470,7 +1470,7 @@ document_command (char *comname, int fro
     error (_("Command \"%s\" is built-in."), comfull);

   sprintf (tmpbuf, "Type documentation for \"%s\".", comfull);
-  doclines = read_command_lines (tmpbuf, from_tty, 0);
+  doclines = read_command_lines (tmpbuf, from_tty, 1);

   if (c->doc)
     xfree (c->doc);

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Daniel Jacobowitz]

On Fri, Nov 06, 2009 at 07:40:39PM +0300, Vladimir Prus wrote:
> read_next_line already used to took process_commands parameter, and process_next_line
> was nothing but "Extract Function" refactoring, so it did not change that.
> 
> The process_commands parameter was added to read_next_line for exactly the reason
> Tom suggested -- to support Python. When parsing 'python' command we need to skip
> all the way to the 'end' without stripping indentation. And when not parsing 'python'
> command, process_commands should be set. Therefore, here's the patch that should fix
> this problem.  Grep claims no other problematic places.
> 
> OK?

No.  I don't think we should strip all whitespace and lines starting
with "#" from documentation, or do special handling of "else", or any
of the other bits enabled by parse_commands.

-- 
Daniel Jacobowitz
CodeSourcery

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Fri, Nov 06, 2009 at 07:40:39PM +0300, Vladimir Prus wrote:
>> read_next_line already used to took process_commands parameter, and process_next_line
>> was nothing but "Extract Function" refactoring, so it did not change that.
>> 
>> The process_commands parameter was added to read_next_line for exactly the reason
>> Tom suggested -- to support Python. When parsing 'python' command we need to skip
>> all the way to the 'end' without stripping indentation. And when not parsing 'python'
>> command, process_commands should be set. Therefore, here's the patch that should fix
>> this problem.  Grep claims no other problematic places.
>> 
>> OK?
> 
> No.  I don't think we should strip all whitespace and lines starting
> with "#" from documentation, or do special handling of "else", or any
> of the other bits enabled by parse_commands.

Uh, right:

        ghost@wind:~/local/bin$ gdb
        GNU gdb 6.8-debian
        ...
        (gdb) define whatever
        Type commands for definition of "whatever".
        End with a line saying just "end".
        >end
        (gdb) document whatever
        Type documentation for "whatever".
        End with a line saying just "end".
        >            # foo
        >    bar
        >if $args
         >      dance around
         >end
        >end
        (gdb) help whatever
        bar
        $args
        (gdb)

Although apparently, *this* was broken since forever and nobody ever cared,
it's weird. What about the attached patch, that produces this effect:

(gdb) document whatever
Type documentation for "whatever".
End with a line saying just "end".
>first
>        second
>#comment
>if $args
>           dance around
>end
(gdb) help whatever
first
        second

if $args
           dance around
(gdb) 

Note that comment has disappeared, but it's not me -- process_next_line is given
an empty string, presumably because readline is trying to act smart.

- Volodya

Index: gdb/cli/cli-script.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
retrieving revision 1.53
diff -u -p -r1.53 cli-script.c
--- gdb/cli/cli-script.c        3 Aug 2009 12:26:37 -0000       1.53
+++ gdb/cli/cli-script.c        6 Nov 2009 17:56:26 -0000
@@ -879,27 +879,35 @@ static enum misc_command_type
 process_next_line (char *p, struct command_line **command, int parse_commands)
 {
   char *p1;
+  char *p2;
   int not_handled = 0;

   /* Not sure what to do here.  */
   if (p == NULL)
     return end_command;

-  if (parse_commands)
-    {
-      /* Strip leading whitespace.  */
-      while (*p == ' ' || *p == '\t')
-       p++;
-    }
-
   /* Strip trailing whitespace.  */
   p1 = p + strlen (p);
   while (p1 != p && (p1[-1] == ' ' || p1[-1] == '\t'))
     p1--;

-  /* Is this the end of a simple, while, or if control structure?  */
-  if (p1 - p == 3 && !strncmp (p, "end", 3))
+  p2 = p;
+  /* Strip leading whitespace.  */
+  while (*p2 == ' ' || *p2 == '\t')
+    p2++;
+
+  /* 'end' is always recognized, regardless of parse_commands value.
+     We also permit whitespace before end and after.  */
+  if (p1 - p2 == 3 && !strncmp (p, "end", 3))
     return end_command;
+
+  if (parse_commands)
+    {
+      /* If commands are parsed, we skip initial spaces. Otherwise,
+        which is the case for Python commands and documentation
+        (see the 'document' command), spaces are preserved.  */
+      p = p2;
+    }

   if (parse_commands)
     {

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Daniel Jacobowitz]

On Fri, Nov 06, 2009 at 08:56:40PM +0300, Vladimir Prus wrote:
> Note that comment has disappeared, but it's not me -- process_next_line is given
> an empty string, presumably because readline is trying to act smart.

It's command_line_input, from top.c.  It also does history expansion.
Oh, well... for another day.

This patch is OK.  Please include changelogs...

> +  if (parse_commands)
> +    {
> +      /* If commands are parsed, we skip initial spaces. Otherwise,
> +        which is the case for Python commands and documentation
> +        (see the 'document' command), spaces are preserved.  */
> +      p = p2;
> +    }
> 
>    if (parse_commands)
>      {
> 

Want to combine the two if statements?

-- 
Daniel Jacobowitz
CodeSourcery

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Vladimir Prus]

[-- Attachment #1: Type: Text/Plain, Size: 886 bytes --]

On Friday 13 November 2009 17:36:05 Daniel Jacobowitz wrote:

> On Fri, Nov 06, 2009 at 08:56:40PM +0300, Vladimir Prus wrote:
> > Note that comment has disappeared, but it's not me -- process_next_line is given
> > an empty string, presumably because readline is trying to act smart.
> 
> It's command_line_input, from top.c.  It also does history expansion.
> Oh, well... for another day.
> 
> This patch is OK.  Please include changelogs...
> 
> > +  if (parse_commands)
> > +    {
> > +      /* If commands are parsed, we skip initial spaces. Otherwise,
> > +        which is the case for Python commands and documentation
> > +        (see the 'document' command), spaces are preserved.  */
> > +      p = p2;
> > +    }
> > 
> >    if (parse_commands)
> >      {
> > 
> 
> Want to combine the two if statements?

Yes, thanks. Here's the final version I've checked in.

- Volodya


[-- Attachment #2: final.diff --]
[-- Type: text/x-patch, Size: 2248 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.11088
diff -u -p -r1.11088 ChangeLog
--- gdb/ChangeLog	18 Nov 2009 16:28:42 -0000	1.11088
+++ gdb/ChangeLog	18 Nov 2009 20:40:56 -0000
@@ -1,3 +1,9 @@
+2009-11-18  Vladimir Prus  <vladimir@codesourcery.com>
+
+	* cli/cli-script.c (process_next_line): Recognize 'end'
+	even when the line has leading space and we're not parsing
+	commands.
+
 2009-11-18  Tom Tromey  <tromey@redhat.com>
 
 	* symtab.c (symbol_set_names): Correctly set 'name' on symbol when
Index: gdb/cli/cli-script.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
retrieving revision 1.53
diff -u -p -r1.53 cli-script.c
--- gdb/cli/cli-script.c	3 Aug 2009 12:26:37 -0000	1.53
+++ gdb/cli/cli-script.c	18 Nov 2009 20:40:56 -0000
@@ -879,30 +879,35 @@ static enum misc_command_type
 process_next_line (char *p, struct command_line **command, int parse_commands)
 {
   char *p1;
+  char *p2;
   int not_handled = 0;
 
   /* Not sure what to do here.  */
   if (p == NULL)
     return end_command;
 
-  if (parse_commands)
-    {
-      /* Strip leading whitespace.  */
-      while (*p == ' ' || *p == '\t')
-	p++;
-    }
-
   /* Strip trailing whitespace.  */
   p1 = p + strlen (p);
   while (p1 != p && (p1[-1] == ' ' || p1[-1] == '\t'))
     p1--;
 
-  /* Is this the end of a simple, while, or if control structure?  */
-  if (p1 - p == 3 && !strncmp (p, "end", 3))
+  p2 = p;
+  /* Strip leading whitespace.  */
+  while (*p2 == ' ' || *p2 == '\t')
+    p2++;
+
+  /* 'end' is always recognized, regardless of parse_commands value. 
+     We also permit whitespace before end and after.  */
+  if (p1 - p2 == 3 && !strncmp (p2, "end", 3))
     return end_command;
-
+  
   if (parse_commands)
     {
+      /* If commands are parsed, we skip initial spaces. Otherwise,
+	 which is the case for Python commands and documentation
+	 (see the 'document' command), spaces are preserved.  */
+      p = p2;
+
       /* Blanks and comments don't really do anything, but we need to
 	 distinguish them from else, end and other commands which can be
 	 executed.  */

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> I noticed my scripts didn't work under v7.0 only when I saw this bug in  
>> launchpad. I guess I am not the only one who has indented "ends" this way 
>> :)
>> https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/461594

Joel> If you could maybe help us a little bit, and track down the author
Joel> of the patch that introduced the change of behavior, and then ask
Joel> him whether the change was intended?

I think it may be because we allow multi-line python commands, and
python is sensitive to indentation.

Tom

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Paul Koning]

Excerpt of message (sent 4 November 2009) by Tom Tromey:
> >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> >> I noticed my scripts didn't work under v7.0 only when I saw this bug in  
> >> launchpad. I guess I am not the only one who has indented "ends" this way 
> >> :)
> >> https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/461594
> 
> Joel> If you could maybe help us a little bit, and track down the author
> Joel> of the patch that introduced the change of behavior, and then ask
> Joel> him whether the change was intended?
> 
> I think it may be because we allow multi-line python commands, and
> python is sensitive to indentation.

Yes, but "end" is not a Python keyword, so that isn't a reason to stop
recognizing <whitespace>end.  

	    paul

Re: gdb v7.0 - user defined command's document section - space prefixed end

[Joel Brobecker]

> Yes, but "end" is not a Python keyword, so that isn't a reason to stop
> recognizing <whitespace>end.  

I was thinking that the change of behavior was unintended, but Volodya
should be able to confirm, or at least give us more details.

-- 
Joel