[PATCH] Fix 'edit' command to go to right line

[PATCH] Fix 'edit' command to go to right line

[Lluís Batlle i Rossell]

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

I attach this patch I used more than 20 years ago.

I thought I had sent it in these years but apparently not.

[-- Attachment #2: 0001-Fix-edit-command-to-go-to-right-line.patch --]
[-- Type: text/plain, Size: 719 bytes --]

From fc0f3f1200f18c86a6d90185f02b1f5ce0c0235b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Llu=C3=ADs=20Batlle=20i=20Rossell?= <viric@viric.name>
Date: Tue, 5 Nov 2024 23:18:30 +0100
Subject: [PATCH] Fix 'edit' command to go to right line

The cursor was placed at a line off by a bunch of lines.
---
 gdb/cli/cli-cmds.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 65ac7d6e7fb..54bef435e86 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -973,7 +973,6 @@ edit_command (const char *arg, int from_tty)
     {
       if (sal.symtab == 0)
 	error (_("No default source file yet."));
-      sal.line += get_lines_to_list () / 2;
     }
   else
     {
-- 
2.44.1

Re: [PATCH] Fix 'edit' command to go to right line

[Andrew Burgess]

Lluís Batlle i Rossell <viric@viric.name> writes:

> I attach this patch I used more than 20 years ago.
>
> I thought I had sent it in these years but apparently not.
> From fc0f3f1200f18c86a6d90185f02b1f5ce0c0235b Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Llu=C3=ADs=20Batlle=20i=20Rossell?= <viric@viric.name>
> Date: Tue, 5 Nov 2024 23:18:30 +0100
> Subject: [PATCH] Fix 'edit' command to go to right line
>
> The cursor was placed at a line off by a bunch of lines.

Thanks for reporting this issue.  I took a look at this patch and
started writing some tests for the 'edit' command.  It would seem that
this was mostly untested until now!

Anyway, though your patch does fix some cases, I found a bunch more
problems, so I'd like to propose the patch below as an alternative.

I hope this should fix the case that I think you care about, though I'm
guessing really as you didn't describe exactly which case you believe
this fixes.  If you are willing to give this alternative patch a try,
and report if this fixes your use case or not that would be really
helpful.

Thanks,
Andrew

---

commit 3c340112d9c71ece32f98a59ec67ceb9fa28d393
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Nov 6 22:18:55 2024 +0000

    gdb: fixes and tests for the 'edit' command
    
    This commit was inspired by this mailing list post:
    
      https://inbox.sourceware.org/gdb-patches/osmtfvf5xe3yx4n7oirukidym4cik7lehhy4re5mxpset2qgwt@6qlboxhqiwgm
    
    When reviewing that patch, the first thing I wanted to do was add some
    tests for the 'edit' command because, as far as I can tell, there are
    no real tests right now.
    
    The approach I've taken for testing is to override the EDITOR
    environment variable, setting this to just 'echo'.  Now when the
    'edit' command is run, instead of entering an interactive editor, the
    shell instead echos back the arguments that GDB is trying to pass to
    the editor.  The output might look like this:
    
      (gdb) edit
      +22 /tmp/gdb/testsuite/gdb.base/edit-cmd.c
      (gdb)
    
    We can then test this like any other normal command.  I then wrote
    some basic tests covering a few situations like, using 'edit' before
    the inferior is started.  Using 'edit' without any arguments, and
    using 'edit' with a line number argument.
    
    There are plenty of cases that are still not tested, for example, the
    test program only has a single source file for example.  But we can
    always add more tests later.
    
    I then used these tests to validate the fix proposed in the above
    patch.
    
    The patch above does indeed fix some cases, specifically, when GDB
    stops at a location (e.g. a breakpoint location) and then the 'edit'
    command without any arguments is fixed.  But using the 'list' command
    to show some other location, and then 'edit' to edit the just listed
    location broken before and after the above patch.
    
    I am instead proposing this alternative patch which I think fixes more
    cases.  When GDB stops at a location then 'edit' with no arguments
    should correctly edit the current line.  And using 'list XX' to list a
    specific location, followed by 'edit' should also now edit the just
    listed location.
    
    Co-Authored-By: Lluís Batlle i Rossell <viric@viric.name>

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 65ac7d6e7fb..225615f0210 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -973,7 +973,8 @@ edit_command (const char *arg, int from_tty)
     {
       if (sal.symtab == 0)
 	error (_("No default source file yet."));
-      sal.line += get_lines_to_list () / 2;
+      if (get_first_line_listed () != 0)
+	sal.line = get_first_line_listed () + get_lines_to_list () / 2;
     }
   else
     {
diff --git a/gdb/testsuite/gdb.base/basic-edit-cmd.c b/gdb/testsuite/gdb.base/basic-edit-cmd.c
new file mode 100644
index 00000000000..fb0f70db51c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/basic-edit-cmd.c
@@ -0,0 +1,55 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2024 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Used so we have some work to do.  */
+volatile int global_var = 0;
+
+int
+main (void)
+{			/* prologue location */
+  ++global_var;		/* first location */
+
+  /*
+   *
+   *
+   * This comment is here as filler.
+   *
+   *
+   */
+
+  ++global_var;		/* second location */
+
+  /*
+   *
+   *
+   * This comment is also here as filler.
+   *
+   *
+   */
+
+  ++global_var;		/* third location */
+
+  /*
+   *
+   *
+   * This is yet another filler comment.
+   *
+   *
+   */
+
+  return 0;		/* fourth location */
+}
diff --git a/gdb/testsuite/gdb.base/basic-edit-cmd.exp b/gdb/testsuite/gdb.base/basic-edit-cmd.exp
new file mode 100644
index 00000000000..dd8fe7022ab
--- /dev/null
+++ b/gdb/testsuite/gdb.base/basic-edit-cmd.exp
@@ -0,0 +1,136 @@
+# Copyright 2024 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test the 'edit' command.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return
+}
+
+# Check that 'echo' is available in the shell.
+gdb_test_multiple "shell echo test 1234 xyz" "check echo is available" {
+    -re -wrap "^test 1234 xyz" {
+    }
+
+    -re -wrap "" {
+	unsupported "shell cannot use echo command"
+	return
+    }
+}
+
+# Find line numbers for use in tests.
+set line_0 [gdb_get_line_number "prologue location"]
+set line_1 [gdb_get_line_number "first location"]
+set line_2 [gdb_get_line_number "second location"]
+set line_3 [gdb_get_line_number "third location"]
+set line_4 [gdb_get_line_number "fourth location"]
+
+# Regexp to match SRCFILE.
+set srcfile_re "\[^\r\n\]+/[string_to_regexp $srcfile]"
+
+# Setup the EDITOR environment variable to run our helper script, and
+# then run the tests.
+
+save_vars { env(EDITOR) } {
+    set env(EDITOR) "echo"
+
+    # Start with no test binary loaded.
+    clean_restart
+    gdb_test "edit" \
+	"^No symbol table is loaded.  Use the \"file\" command\\." \
+	"try edit when no symbol file is loaded"
+
+    # Now start with a test binary.
+    clean_restart $binfile
+
+    with_test_prefix "before starting inferior" {
+	gdb_test "edit" \
+	    "\r\n\\+$line_0 $srcfile_re" \
+	    "check edit of default location"
+
+	gdb_test "list $line_4" \
+	    "\r\n$line_4\\s+\[^\r\n\]+/\\* fourth location \\*/\r\n.*" \
+	    "list lines around the fourth location"
+
+	gdb_test "edit" \
+	    "\r\n\\+$line_4 $srcfile_re" \
+	    "check edit of fourth location after listing"
+
+	gdb_test "edit $line_2" \
+	    "\r\n\\+$line_2 $srcfile_re" \
+	    "check edit of second location"
+
+	gdb_test "edit xxx" \
+	    "^Function \"xxx\" not defined\\." \
+	    "try to edit an unknown function"
+    }
+
+    if {![runto_main]} {
+	return
+    }
+
+    set first_loc_pc [get_hexadecimal_valueof "\$pc" "*UNKNOWN*" \
+			  "get \$pc at first location"]
+
+    with_test_prefix "stopped at first location" {
+	gdb_test "edit" \
+	    "\r\n\\+$line_1 $srcfile_re" \
+	    "check edit of current location"
+    }
+
+    gdb_breakpoint $line_2
+    gdb_continue_to_breakpoint "stop at second location"
+
+    with_test_prefix "at second location" {
+	gdb_test "edit" \
+	    "\r\n\\+$line_2 $srcfile_re" \
+	    "check edit current location results"
+
+	gdb_test "edit $line_3" \
+	    "\r\n\\+$line_3 $srcfile_re" \
+	    "check edit third location results"
+    }
+
+    with_test_prefix "list first location" {
+	gdb_test "list $line_1" \
+	    "\r\n$line_1\\s+\[^\r\n\]+/\\* first location \\*/\r\n.*" \
+	    "list lines around the first location"
+
+	gdb_test "edit" \
+	    "\r\n\\+$line_1 $srcfile_re" \
+	    "check edit current location results"
+    }
+
+    gdb_breakpoint $line_4
+    gdb_continue_to_breakpoint "stop at fourth location"
+
+    with_test_prefix "at fourth location" {
+	gdb_test "edit" \
+	    "\r\n\\+$line_4 $srcfile_re" \
+	    "check edit current location results"
+
+	gdb_test "edit $line_1" \
+	    "\r\n\\+$line_1 $srcfile_re" \
+	    "check edit first location results"
+
+	gdb_test "edit *$first_loc_pc" \
+	    [multi_line \
+		 "[string_to_regexp $first_loc_pc] is in main \\($srcfile_re:$line_1\\)\\." \
+		 "\\+$line_1 $srcfile_re"] \
+	    "check edit first location by address results"
+    }
+}

Re: Re: [PATCH] Fix 'edit' command to go to right line

[Lluís Batlle i Rossell]

On Thu, Nov 07, 2024 at 11:13:12AM +0000, Andrew Burgess wrote:
> Lluís Batlle i Rossell <viric@viric.name> writes:
> 
> > I attach this patch I used more than 20 years ago.
> >
> > I thought I had sent it in these years but apparently not.
> > From fc0f3f1200f18c86a6d90185f02b1f5ce0c0235b Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Llu=C3=ADs=20Batlle=20i=20Rossell?= <viric@viric.name>
> > Date: Tue, 5 Nov 2024 23:18:30 +0100
> > Subject: [PATCH] Fix 'edit' command to go to right line
> >
> > The cursor was placed at a line off by a bunch of lines.
> 
> Thanks for reporting this issue.  I took a look at this patch and
> started writing some tests for the 'edit' command.  It would seem that
> this was mostly untested until now!
> 
> Anyway, though your patch does fix some cases, I found a bunch more
> problems, so I'd like to propose the patch below as an alternative.
> 
> I hope this should fix the case that I think you care about, though I'm
> guessing really as you didn't describe exactly which case you believe
> this fixes.  If you are willing to give this alternative patch a try,
> and report if this fixes your use case or not that would be really
> helpful.
> 
> Thanks,
> Andrew

(Now replying Cc to the list)

Sure! Use your patch.

I have EDITOR=vim. And when I'm debugging in some frame, I run 'edit' to
get me to that position in vim.

I don't really understand what is the get_first_line_listed ()
situation... I think it means that if 'list' has been run, then the editor
on 'edit' will point to the line in the middle of the last 'list'.
I like this as well, if I interpret correctly.

Thanks,
Lluís.

[PATCHv2] gdb: fixes and tests for the 'edit' command

[Andrew Burgess]

In v2:

  - Changes to the test so that 'make check-all-boards' runs clean.

---

This commit was inspired by this mailing list post:

  https://inbox.sourceware.org/gdb-patches/osmtfvf5xe3yx4n7oirukidym4cik7lehhy4re5mxpset2qgwt@6qlboxhqiwgm

When reviewing that patch, the first thing I wanted to do was add some
tests for the 'edit' command because, as far as I can tell, there are
no real tests right now.

The approach I've taken for testing is to override the EDITOR
environment variable, setting this to just 'echo'.  Now when the
'edit' command is run, instead of entering an interactive editor, the
shell instead echos back the arguments that GDB is trying to pass to
the editor.  The output might look like this:

  (gdb) edit
  +22 /tmp/gdb/testsuite/gdb.base/edit-cmd.c
  (gdb)

We can then test this like any other normal command.  I then wrote
some basic tests covering a few situations like, using 'edit' before
the inferior is started.  Using 'edit' without any arguments, and
using 'edit' with a line number argument.

There are plenty of cases that are still not tested, for example, the
test program only has a single source file for example.  But we can
always add more tests later.

I then used these tests to validate the fix proposed in the above
patch.

The patch above does indeed fix some cases, specifically, when GDB
stops at a location (e.g. a breakpoint location) and then the 'edit'
command without any arguments is fixed.  But using the 'list' command
to show some other location, and then 'edit' to edit the just listed
location broken before and after the above patch.

I am instead proposing this alternative patch which I think fixes more
cases.  When GDB stops at a location then 'edit' with no arguments
should correctly edit the current line.  And using 'list XX' to list a
specific location, followed by 'edit' should also now edit the just
listed location.

Co-Authored-By: Lluís Batlle i Rossell <viric@viric.name>
---
 gdb/cli/cli-cmds.c                        |   3 +-
 gdb/testsuite/gdb.base/basic-edit-cmd.c   |  55 ++++++++
 gdb/testsuite/gdb.base/basic-edit-cmd.exp | 153 ++++++++++++++++++++++
 3 files changed, 210 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/basic-edit-cmd.c
 create mode 100644 gdb/testsuite/gdb.base/basic-edit-cmd.exp

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 65ac7d6e7fb..225615f0210 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -973,7 +973,8 @@ edit_command (const char *arg, int from_tty)
     {
       if (sal.symtab == 0)
 	error (_("No default source file yet."));
-      sal.line += get_lines_to_list () / 2;
+      if (get_first_line_listed () != 0)
+	sal.line = get_first_line_listed () + get_lines_to_list () / 2;
     }
   else
     {
diff --git a/gdb/testsuite/gdb.base/basic-edit-cmd.c b/gdb/testsuite/gdb.base/basic-edit-cmd.c
new file mode 100644
index 00000000000..fb0f70db51c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/basic-edit-cmd.c
@@ -0,0 +1,55 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2024 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Used so we have some work to do.  */
+volatile int global_var = 0;
+
+int
+main (void)
+{			/* prologue location */
+  ++global_var;		/* first location */
+
+  /*
+   *
+   *
+   * This comment is here as filler.
+   *
+   *
+   */
+
+  ++global_var;		/* second location */
+
+  /*
+   *
+   *
+   * This comment is also here as filler.
+   *
+   *
+   */
+
+  ++global_var;		/* third location */
+
+  /*
+   *
+   *
+   * This is yet another filler comment.
+   *
+   *
+   */
+
+  return 0;		/* fourth location */
+}
diff --git a/gdb/testsuite/gdb.base/basic-edit-cmd.exp b/gdb/testsuite/gdb.base/basic-edit-cmd.exp
new file mode 100644
index 00000000000..608ab1818a7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/basic-edit-cmd.exp
@@ -0,0 +1,153 @@
+# Copyright 2024 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test the 'edit' command.
+
+# This relies on setting environment variables, so best to run on
+# non-remote hosts.
+require {!is_remote host}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return
+}
+
+# Check that 'echo' is available in the shell.
+gdb_test_multiple "shell echo test 1234 xyz" "check echo is available" {
+    -re -wrap "^test 1234 xyz" {
+    }
+
+    -re -wrap "" {
+	unsupported "shell cannot use echo command"
+	return
+    }
+}
+
+if {![runto_main]} {
+    return
+}
+
+# Are we using DWARF debug format?
+get_debug_format
+set non_dwarf [expr ! [test_debug_format "DWARF \[0-9\]"]]
+
+# Find line numbers for use in tests.
+set line_0 [gdb_get_line_number "prologue location"]
+set line_1 [gdb_get_line_number "first location"]
+set line_2 [gdb_get_line_number "second location"]
+set line_3 [gdb_get_line_number "third location"]
+set line_4 [gdb_get_line_number "fourth location"]
+
+# Regexp to match SRCFILE.
+set srcfile_re "\[^\r\n\]+/[string_to_regexp $srcfile]"
+
+# Setup the EDITOR environment variable to run our helper script, and
+# then run the tests.
+
+save_vars { env(EDITOR) } {
+    set env(EDITOR) "echo"
+
+    # Start with no test binary loaded.
+    clean_restart
+    gdb_test "edit" \
+	"^No symbol table is loaded.  Use the \"file\" command\\." \
+	"try edit when no symbol file is loaded"
+
+    # Now start with a test binary.
+    clean_restart $binfile
+
+    with_test_prefix "before starting inferior" {
+
+	# We should be able to find the default location (of main)
+	# even for non-dwarf debug formats, but this currently fails
+	# with the stabs board.
+	if { $non_dwarf } { setup_xfail *-*-* }
+	gdb_test "edit" \
+	    "\r\n\\+$line_0 $srcfile_re" \
+	    "check edit of default location"
+
+	gdb_test "list $line_4" \
+	    "\r\n$line_4\\s+\[^\r\n\]+/\\* fourth location \\*/\r\n.*" \
+	    "list lines around the fourth location"
+
+	gdb_test "edit" \
+	    "\r\n\\+$line_4 $srcfile_re" \
+	    "check edit of fourth location after listing"
+
+	gdb_test "edit $line_2" \
+	    "\r\n\\+$line_2 $srcfile_re" \
+	    "check edit of second location"
+
+	gdb_test "edit xxx" \
+	    "^Function \"xxx\" not defined\\." \
+	    "try to edit an unknown function"
+    }
+
+    if {![runto_main]} {
+	return
+    }
+
+    set first_loc_pc [get_hexadecimal_valueof "\$pc" "*UNKNOWN*" \
+			  "get \$pc at first location"]
+
+    with_test_prefix "stopped at first location" {
+	gdb_test "edit" \
+	    "\r\n\\+$line_1 $srcfile_re" \
+	    "check edit of current location"
+    }
+
+    gdb_breakpoint $line_2
+    gdb_continue_to_breakpoint "stop at second location"
+
+    with_test_prefix "at second location" {
+	gdb_test "edit" \
+	    "\r\n\\+$line_2 $srcfile_re" \
+	    "check edit current location results"
+
+	gdb_test "edit $line_3" \
+	    "\r\n\\+$line_3 $srcfile_re" \
+	    "check edit third location results"
+    }
+
+    with_test_prefix "list first location" {
+	gdb_test "list $line_1" \
+	    "\r\n$line_1\\s+\[^\r\n\]+/\\* first location \\*/\r\n.*" \
+	    "list lines around the first location"
+
+	gdb_test "edit" \
+	    "\r\n\\+$line_1 $srcfile_re" \
+	    "check edit current location results"
+    }
+
+    gdb_breakpoint $line_4
+    gdb_continue_to_breakpoint "stop at fourth location"
+
+    with_test_prefix "at fourth location" {
+	gdb_test "edit" \
+	    "\r\n\\+$line_4 $srcfile_re" \
+	    "check edit current location results"
+
+	gdb_test "edit $line_1" \
+	    "\r\n\\+$line_1 $srcfile_re" \
+	    "check edit first location results"
+
+	gdb_test "edit *$first_loc_pc" \
+	    [multi_line \
+		 "[string_to_regexp $first_loc_pc] is in main \\($srcfile_re:$line_1\\)\\." \
+		 "\\+$line_1 $srcfile_re"] \
+	    "check edit first location by address results"
+    }
+}

base-commit: ebc73070f4b85e4adee7d2f49cb32e2bd7b16324
-- 
2.25.4

Re: Re: [PATCH] Fix 'edit' command to go to right line

[Andrew Burgess]

Lluís Batlle i Rossell <viric@viric.name> writes:

> On Thu, Nov 07, 2024 at 11:13:12AM +0000, Andrew Burgess wrote:
>> Lluís Batlle i Rossell <viric@viric.name> writes:
>> 
>> > I attach this patch I used more than 20 years ago.
>> >
>> > I thought I had sent it in these years but apparently not.
>> > From fc0f3f1200f18c86a6d90185f02b1f5ce0c0235b Mon Sep 17 00:00:00 2001
>> > From: =?UTF-8?q?Llu=C3=ADs=20Batlle=20i=20Rossell?= <viric@viric.name>
>> > Date: Tue, 5 Nov 2024 23:18:30 +0100
>> > Subject: [PATCH] Fix 'edit' command to go to right line
>> >
>> > The cursor was placed at a line off by a bunch of lines.
>> 
>> Thanks for reporting this issue.  I took a look at this patch and
>> started writing some tests for the 'edit' command.  It would seem that
>> this was mostly untested until now!
>> 
>> Anyway, though your patch does fix some cases, I found a bunch more
>> problems, so I'd like to propose the patch below as an alternative.
>> 
>> I hope this should fix the case that I think you care about, though I'm
>> guessing really as you didn't describe exactly which case you believe
>> this fixes.  If you are willing to give this alternative patch a try,
>> and report if this fixes your use case or not that would be really
>> helpful.
>> 
>> Thanks,
>> Andrew
>
> (Now replying Cc to the list)
>
> Sure! Use your patch.
>
> I have EDITOR=vim. And when I'm debugging in some frame, I run 'edit' to
> get me to that position in vim.
>
> I don't really understand what is the get_first_line_listed ()
> situation... I think it means that if 'list' has been run, then the editor
> on 'edit' will point to the line in the middle of the last 'list'.
> I like this as well, if I interpret correctly.

That's correct.  It means I can do:

  (gdb) list SOME_LINE_NUMBER
  (gdb) edit

and GDB will start editing SOME_LINE_NUMBER.  I think we were already
trying to do this, but we were off by 'get_lines_to_list()' in this
case.

I've posted a v2 which fixes some problems with the new test.  I'll
leave this for a while to see if anyone else wants to comment before
merging.

Thanks,
Andrew

Re: [PATCHv2] gdb: fixes and tests for the 'edit' command

[Tom Tromey]

Andrew> I am instead proposing this alternative patch which I think fixes more
Andrew> cases.  When GDB stops at a location then 'edit' with no arguments
Andrew> should correctly edit the current line.  And using 'list XX' to list a
Andrew> specific location, followed by 'edit' should also now edit the just
Andrew> listed location.

Looks good to me.  Thank you.

I think this fixes
https://sourceware.org/bugzilla/show_bug.cgi?id=17669

I also wonder if it fixes
https://sourceware.org/bugzilla/show_bug.cgi?id=8837

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCHv2] gdb: fixes and tests for the 'edit' command

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> Andrew> I am instead proposing this alternative patch which I think fixes more
> Andrew> cases.  When GDB stops at a location then 'edit' with no arguments
> Andrew> should correctly edit the current line.  And using 'list XX' to list a
> Andrew> specific location, followed by 'edit' should also now edit the just
> Andrew> listed location.
>
> Looks good to me.  Thank you.
>
> I think this fixes
> https://sourceware.org/bugzilla/show_bug.cgi?id=17669
>
> I also wonder if it fixes
> https://sourceware.org/bugzilla/show_bug.cgi?id=8837
>
> Approved-By: Tom Tromey <tom@tromey.com>

Thanks Tom,

I updated the commit message to mention PR gdb/17669 as this patch does
fix that issue.

The other bug looks like it was fixed long ago, I added a comment to the
bug and closed it.

I did however update the tests in _this_ commit slightly to better cover
the case from the second bug (PR gdb/8837), though the GDB code didn't
change at all.  I've attached the updated patch below.

Thanks,
Andrew

---

commit 31ada87f91b4c5306d81c8a896df9764c32941f3
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Nov 6 22:18:55 2024 +0000

    gdb: fixes and tests for the 'edit' command
    
    This commit was inspired by this mailing list post:
    
      https://inbox.sourceware.org/gdb-patches/osmtfvf5xe3yx4n7oirukidym4cik7lehhy4re5mxpset2qgwt@6qlboxhqiwgm
    
    When reviewing that patch, the first thing I wanted to do was add some
    tests for the 'edit' command because, as far as I can tell, there are
    no real tests right now.
    
    The approach I've taken for testing is to override the EDITOR
    environment variable, setting this to just 'echo'.  Now when the
    'edit' command is run, instead of entering an interactive editor, the
    shell instead echos back the arguments that GDB is trying to pass to
    the editor.  The output might look like this:
    
      (gdb) edit
      +22 /tmp/gdb/testsuite/gdb.base/edit-cmd.c
      (gdb)
    
    We can then test this like any other normal command.  I then wrote
    some basic tests covering a few situations like, using 'edit' before
    the inferior is started.  Using 'edit' without any arguments, and
    using 'edit' with a line number argument.
    
    There are plenty of cases that are still not tested, for example, the
    test program only has a single source file for example.  But we can
    always add more tests later.
    
    I then used these tests to validate the fix proposed in the above
    patch.
    
    The patch above does indeed fix some cases, specifically, when GDB
    stops at a location (e.g. a breakpoint location) and then the 'edit'
    command without any arguments is fixed.  But using the 'list' command
    to show some other location, and then 'edit' to edit the just listed
    location broken before and after the above patch.
    
    I am instead proposing this alternative patch which I think fixes more
    cases.  When GDB stops at a location then 'edit' with no arguments
    should correctly edit the current line.  And using 'list XX' to list a
    specific location, followed by 'edit' should also now edit the just
    listed location.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17669
    
    Co-Authored-By: Lluís Batlle i Rossell <viric@viric.name>
    Approved-By: Tom Tromey <tom@tromey.com>

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 65ac7d6e7fb..225615f0210 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -973,7 +973,8 @@ edit_command (const char *arg, int from_tty)
     {
       if (sal.symtab == 0)
 	error (_("No default source file yet."));
-      sal.line += get_lines_to_list () / 2;
+      if (get_first_line_listed () != 0)
+	sal.line = get_first_line_listed () + get_lines_to_list () / 2;
     }
   else
     {
diff --git a/gdb/testsuite/gdb.base/basic-edit-cmd.c b/gdb/testsuite/gdb.base/basic-edit-cmd.c
new file mode 100644
index 00000000000..fb0f70db51c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/basic-edit-cmd.c
@@ -0,0 +1,55 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2024 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Used so we have some work to do.  */
+volatile int global_var = 0;
+
+int
+main (void)
+{			/* prologue location */
+  ++global_var;		/* first location */
+
+  /*
+   *
+   *
+   * This comment is here as filler.
+   *
+   *
+   */
+
+  ++global_var;		/* second location */
+
+  /*
+   *
+   *
+   * This comment is also here as filler.
+   *
+   *
+   */
+
+  ++global_var;		/* third location */
+
+  /*
+   *
+   *
+   * This is yet another filler comment.
+   *
+   *
+   */
+
+  return 0;		/* fourth location */
+}
diff --git a/gdb/testsuite/gdb.base/basic-edit-cmd.exp b/gdb/testsuite/gdb.base/basic-edit-cmd.exp
new file mode 100644
index 00000000000..116daf9e3b7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/basic-edit-cmd.exp
@@ -0,0 +1,154 @@
+# Copyright 2024 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test the 'edit' command.
+
+# This relies on setting environment variables, so best to run on
+# non-remote hosts.
+require {!is_remote host}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return
+}
+
+# Check that 'echo' is available in the shell.
+gdb_test_multiple "shell echo test 1234 xyz" "check echo is available" {
+    -re -wrap "^test 1234 xyz" {
+    }
+
+    -re -wrap "" {
+	unsupported "shell cannot use echo command"
+	return
+    }
+}
+
+if {![runto_main]} {
+    return
+}
+
+# Are we using DWARF debug format?
+get_debug_format
+set non_dwarf [expr ! [test_debug_format "DWARF \[0-9\]"]]
+
+# Find line numbers for use in tests.
+set line_0 [gdb_get_line_number "prologue location"]
+set line_1 [gdb_get_line_number "first location"]
+set line_2 [gdb_get_line_number "second location"]
+set line_3 [gdb_get_line_number "third location"]
+set line_4 [gdb_get_line_number "fourth location"]
+
+# Regexp to match SRCFILE.
+set srcfile_re [string_to_regexp [file normalize $srcdir/$subdir]/$srcfile]
+set srcfile_re_simple "\[^\r\n\]+/[string_to_regexp $srcfile]"
+
+# Setup the EDITOR environment variable to run our helper script, and
+# then run the tests.
+
+save_vars { env(EDITOR) } {
+    set env(EDITOR) "echo"
+
+    # Start with no test binary loaded.
+    clean_restart
+    gdb_test "edit" \
+	"^No symbol table is loaded.  Use the \"file\" command\\." \
+	"try edit when no symbol file is loaded"
+
+    # Now start with a test binary.
+    clean_restart $binfile
+
+    with_test_prefix "before starting inferior" {
+
+	# We should be able to find the default location (of main)
+	# even for non-dwarf debug formats, but this currently fails
+	# with the stabs board.
+	if { $non_dwarf } { setup_xfail *-*-* }
+	gdb_test "edit" \
+	    "\r\n\\+$line_0 $srcfile_re" \
+	    "check edit of default location"
+
+	gdb_test "list $line_4" \
+	    "\r\n$line_4\\s+\[^\r\n\]+/\\* fourth location \\*/\r\n.*" \
+	    "list lines around the fourth location"
+
+	gdb_test "edit" \
+	    "\r\n\\+$line_4 $srcfile_re" \
+	    "check edit of fourth location after listing"
+
+	gdb_test "edit $line_2" \
+	    "\r\n\\+$line_2 $srcfile_re" \
+	    "check edit of second location"
+
+	gdb_test "edit xxx" \
+	    "^Function \"xxx\" not defined\\." \
+	    "try to edit an unknown function"
+    }
+
+    if {![runto_main]} {
+	return
+    }
+
+    set first_loc_pc [get_hexadecimal_valueof "\$pc" "*UNKNOWN*" \
+			  "get \$pc at first location"]
+
+    with_test_prefix "stopped at first location" {
+	gdb_test "edit" \
+	    "\r\n\\+$line_1 $srcfile_re" \
+	    "check edit of current location"
+    }
+
+    gdb_breakpoint $line_2
+    gdb_continue_to_breakpoint "stop at second location"
+
+    with_test_prefix "at second location" {
+	gdb_test "edit" \
+	    "\r\n\\+$line_2 $srcfile_re" \
+	    "check edit current location results"
+
+	gdb_test "edit $line_3" \
+	    "\r\n\\+$line_3 $srcfile_re" \
+	    "check edit third location results"
+    }
+
+    with_test_prefix "list first location" {
+	gdb_test "list $line_1" \
+	    "\r\n$line_1\\s+\[^\r\n\]+/\\* first location \\*/\r\n.*" \
+	    "list lines around the first location"
+
+	gdb_test "edit" \
+	    "\r\n\\+$line_1 $srcfile_re" \
+	    "check edit current location results"
+    }
+
+    gdb_breakpoint $line_4
+    gdb_continue_to_breakpoint "stop at fourth location"
+
+    with_test_prefix "at fourth location" {
+	gdb_test "edit" \
+	    "\r\n\\+$line_4 $srcfile_re" \
+	    "check edit current location results"
+
+	gdb_test "edit $line_1" \
+	    "\r\n\\+$line_1 $srcfile_re" \
+	    "check edit first location results"
+
+	gdb_test "edit *$first_loc_pc" \
+	    [multi_line \
+		 "[string_to_regexp $first_loc_pc] is in main \\($srcfile_re_simple:$line_1\\)\\." \
+		 "\\+$line_1 $srcfile_re"] \
+	    "check edit first location by address results"
+    }
+}

Re: Re: [PATCHv2] gdb: fixes and tests for the 'edit' command

[Lluís Batlle i Rossell]

On Fri, Nov 08, 2024 at 10:51:49AM +0000, Andrew Burgess wrote:
> > I think this fixes
> > https://sourceware.org/bugzilla/show_bug.cgi?id=17669
> >
> > I also wonder if it fixes
> > https://sourceware.org/bugzilla/show_bug.cgi?id=8837
> >
> > Approved-By: Tom Tromey <tom@tromey.com>
> 
> I updated the commit message to mention PR gdb/17669 as this patch does
> fix that issue.

AH! Now I see I posted my short solution in that bugzilla in 2014.
That explains why I remembered as if I already had "sent the patch".

Finally the bug is fixed. I'm very happy because I use the edit command
constantly.