[PATCH v2 0/1] gdb: adjust the default place of 'list' to main's prologue

[PATCH v2 0/1] gdb: adjust the default place of 'list' to main's prologue

[Stephan Rohr]

Hello,

this is version 2 of my proposed patch.  The patch adjusts main's
default location to the prologue and corrects the offsets for the list
command.

You can find the previous discussion at
https://sourceware.org/pipermail/gdb-patches/2024-June/209613.html.

I appreciate your feedback.

Thanks
stephan

Stephan Rohr (1):
  gdb: adjust the default place of 'list' to main's prologue

 gdb/source.c                                 |  4 +--
 gdb/testsuite/gdb.base/cursal.exp            |  2 +-
 gdb/testsuite/gdb.base/list-before-start.exp | 36 ++++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-file.exp             |  9 ++---
 4 files changed, 41 insertions(+), 10 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/list-before-start.exp

-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v2 1/1] gdb: adjust the default place of 'list' to main's prologue

[Stephan Rohr]

The 'list' command prints around the 'main' function if the current
source location is not set.  The prologue of 'main' is skipped and the
first real line of 'main' is offset by 'lines_to_print - 1'.  This is
incorrect, the location should be defaulted to main's prologue without
applying offsets (similar to 'list main').  Printing around the selected
line is then done in 'list_around_line'.

The patch also fixes an issue if the list command is used before the
program is started.  For example, with the following code:

26 static void attribute ((used)) ambiguous_fun (void) {}
27
28 static int attribute ((used)) ambiguous_var;
29
30
31
32
33
34
35
36
37
38 int
39 main (void)
40 {
41   return 0;
42 }

GDB offsets the relevant line by 'lines_to_print - 1' and then by another
'lines_to_print / 2' and prints:

(gdb) list
27
28 static int attribute ((used)) ambiguous_var;
29
30
31
32
33
34
35
36

With this patch, GDB correctly prints:

37
38      int
39      main (void)
40      {
41        return 0;
42      }
---
 gdb/source.c                                 |  4 +--
 gdb/testsuite/gdb.base/cursal.exp            |  2 +-
 gdb/testsuite/gdb.base/list-before-start.exp | 36 ++++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-file.exp             |  9 ++---
 4 files changed, 41 insertions(+), 10 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/list-before-start.exp

diff --git a/gdb/source.c b/gdb/source.c
index 24a8769da91..e85fda5bac5 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -319,13 +319,13 @@ select_source_symtab ()
 				     SEARCH_FUNCTION_DOMAIN, nullptr);
   if (bsym.symbol != nullptr)
     {
-      symtab_and_line sal = find_function_start_sal (bsym.symbol, true);
+      symtab_and_line sal = find_function_start_sal (bsym.symbol, false);
       if (sal.symtab == NULL)
 	/* We couldn't find the location of `main', possibly due to missing
 	   line number info, fall back to line 1 in the corresponding file.  */
 	loc->set (bsym.symbol->symtab (), 1);
       else
-	loc->set (sal.symtab, std::max (sal.line - (lines_to_list - 1), 1));
+	loc->set (sal.symtab, sal.line);
       return;
     }
 
diff --git a/gdb/testsuite/gdb.base/cursal.exp b/gdb/testsuite/gdb.base/cursal.exp
index 3acced86e3c..55e1793d430 100644
--- a/gdb/testsuite/gdb.base/cursal.exp
+++ b/gdb/testsuite/gdb.base/cursal.exp
@@ -27,7 +27,7 @@ gdb_test_no_output "set listsize 1"
 
 # initial sal should be first statement in main
 gdb_test "list" \
-    "v0 = 0;" \
+    "{" \
     "list before run"
 
 gdb_load ${binfile}
diff --git a/gdb/testsuite/gdb.base/list-before-start.exp b/gdb/testsuite/gdb.base/list-before-start.exp
new file mode 100644
index 00000000000..176fc4ab8a7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/list-before-start.exp
@@ -0,0 +1,36 @@
+# 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 "list" command to print the location around main before the
+# program is started.
+
+standard_testfile list-ambiguous0.c
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+set fill "${decimal}\[ \t\]+\[^\n\r\]+"
+
+gdb_test_no_output "set listsize 10"
+
+gdb_test "list" \
+    [multi_line \
+        "${decimal}\[ \t\]+" \
+        "${decimal}\[ \t\]+int" \
+        "${decimal}\[ \t\]+main\[^\n\r\]+" \
+        "${fill}" \
+        "${fill}" \
+        "${fill}" ]
diff --git a/gdb/testsuite/gdb.mi/mi-file.exp b/gdb/testsuite/gdb.mi/mi-file.exp
index 8e404f9c9bd..efa1db8d2d2 100644
--- a/gdb/testsuite/gdb.mi/mi-file.exp
+++ b/gdb/testsuite/gdb.mi/mi-file.exp
@@ -42,16 +42,11 @@ proc test_file_list_exec_source_file {} {
     }
 
     # get the path and absolute path to the current executable
-    #
-    # In gdb 6.2 (at least), the default line number is set by
-    # select_source_symtab to the first line of "main" minus
-    # the value of "lines_to_list" (which defaults to 10) plus one.
-    # --chastain 2004-08-13
 
     set line_main_head [gdb_get_line_number "main ("]
-    set line_main_body [expr $line_main_head + 2]
+    set line_main_prologue [expr $line_main_head + 1]
     set gdb_lines_to_list 10
-    set line_default [expr $line_main_body - $gdb_lines_to_list + 1]
+    set line_default $line_main_prologue
 
     mi_gdb_test "111-file-list-exec-source-file" \
 	    "111\\\^done,line=\"$line_default\",file=\"${srcfilepath}\",fullname=\"$fullname_syntax${srcfile}\",macro-info=\"0\"" \
-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 1/1] gdb: adjust the default place of 'list' to main's prologue

[Guinevere Larsen]

On 6/19/24 2:47 AM, Stephan Rohr wrote:
> The 'list' command prints around the 'main' function if the current
> source location is not set.  The prologue of 'main' is skipped and the
> first real line of 'main' is offset by 'lines_to_print - 1'.  This is
> incorrect, the location should be defaulted to main's prologue without
> applying offsets (similar to 'list main').  Printing around the selected
> line is then done in 'list_around_line'.
>
> The patch also fixes an issue if the list command is used before the
> program is started.  For example, with the following code:
>
> 26 static void attribute ((used)) ambiguous_fun (void) {}
> 27
> 28 static int attribute ((used)) ambiguous_var;
> 29
> 30
> 31
> 32
> 33
> 34
> 35
> 36
> 37
> 38 int
> 39 main (void)
> 40 {
> 41   return 0;
> 42 }
>
> GDB offsets the relevant line by 'lines_to_print - 1' and then by another
> 'lines_to_print / 2' and prints:
>
> (gdb) list
> 27
> 28 static int attribute ((used)) ambiguous_var;
> 29
> 30
> 31
> 32
> 33
> 34
> 35
> 36
>
> With this patch, GDB correctly prints:
>
> 37
> 38      int
> 39      main (void)
> 40      {
> 41        return 0;
> 42      }
> ---

Hi!

Thanks for working on this! I only have 2 minor nits inlined.

>   gdb/source.c                                 |  4 +--
>   gdb/testsuite/gdb.base/cursal.exp            |  2 +-
>   gdb/testsuite/gdb.base/list-before-start.exp | 36 ++++++++++++++++++++
>   gdb/testsuite/gdb.mi/mi-file.exp             |  9 ++---
>   4 files changed, 41 insertions(+), 10 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.base/list-before-start.exp
>
> diff --git a/gdb/source.c b/gdb/source.c
> index 24a8769da91..e85fda5bac5 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -319,13 +319,13 @@ select_source_symtab ()
>   				     SEARCH_FUNCTION_DOMAIN, nullptr);
>     if (bsym.symbol != nullptr)
>       {
> -      symtab_and_line sal = find_function_start_sal (bsym.symbol, true);
> +      symtab_and_line sal = find_function_start_sal (bsym.symbol, false);
>         if (sal.symtab == NULL)
>   	/* We couldn't find the location of `main', possibly due to missing
>   	   line number info, fall back to line 1 in the corresponding file.  */
>   	loc->set (bsym.symbol->symtab (), 1);
>         else
> -	loc->set (sal.symtab, std::max (sal.line - (lines_to_list - 1), 1));
> +	loc->set (sal.symtab, sal.line);
>         return;
>       }
>   
> diff --git a/gdb/testsuite/gdb.base/cursal.exp b/gdb/testsuite/gdb.base/cursal.exp
> index 3acced86e3c..55e1793d430 100644
> --- a/gdb/testsuite/gdb.base/cursal.exp
> +++ b/gdb/testsuite/gdb.base/cursal.exp
> @@ -27,7 +27,7 @@ gdb_test_no_output "set listsize 1"
>   
>   # initial sal should be first statement in main
Now that we've changed it, this comment is incorrect, it should say it's 
in the prologue in main. Also, when you update that, add a period at the 
end of the sentence, since the last contributor seems to have forgotten.
>   gdb_test "list" \
> -    "v0 = 0;" \
> +    "{" \
>       "list before run"
>   
>   gdb_load ${binfile}
> diff --git a/gdb/testsuite/gdb.base/list-before-start.exp b/gdb/testsuite/gdb.base/list-before-start.exp
> new file mode 100644
> index 00000000000..176fc4ab8a7
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/list-before-start.exp
> @@ -0,0 +1,36 @@
> +# 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 "list" command to print the location around main before the
> +# program is started.
> +
> +standard_testfile list-ambiguous0.c
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
> +    return -1
> +}
> +
> +set fill "${decimal}\[ \t\]+\[^\n\r\]+"
> +
> +gdb_test_no_output "set listsize 10"
> +
> +gdb_test "list" \
> +    [multi_line \
> +        "${decimal}\[ \t\]+" \
> +        "${decimal}\[ \t\]+int" \
> +        "${decimal}\[ \t\]+main\[^\n\r\]+" \
> +        "${fill}" \
> +        "${fill}" \
> +        "${fill}" ]

These should be indented by a tab instead of 8 spaces.

With these nits fixed, LGTM, Reviewed-By: Guinevere Larsen 
<blarsen@redhat.com>

I hope a maintainer approves this soon, this has bugged me for a long time.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

> diff --git a/gdb/testsuite/gdb.mi/mi-file.exp b/gdb/testsuite/gdb.mi/mi-file.exp
> index 8e404f9c9bd..efa1db8d2d2 100644
> --- a/gdb/testsuite/gdb.mi/mi-file.exp
> +++ b/gdb/testsuite/gdb.mi/mi-file.exp
> @@ -42,16 +42,11 @@ proc test_file_list_exec_source_file {} {
>       }
>   
>       # get the path and absolute path to the current executable
> -    #
> -    # In gdb 6.2 (at least), the default line number is set by
> -    # select_source_symtab to the first line of "main" minus
> -    # the value of "lines_to_list" (which defaults to 10) plus one.
> -    # --chastain 2004-08-13
>   
>       set line_main_head [gdb_get_line_number "main ("]
> -    set line_main_body [expr $line_main_head + 2]
> +    set line_main_prologue [expr $line_main_head + 1]
>       set gdb_lines_to_list 10
> -    set line_default [expr $line_main_body - $gdb_lines_to_list + 1]
> +    set line_default $line_main_prologue
>   
>       mi_gdb_test "111-file-list-exec-source-file" \
>   	    "111\\\^done,line=\"$line_default\",file=\"${srcfilepath}\",fullname=\"$fullname_syntax${srcfile}\",macro-info=\"0\"" \