[PATCH] gdb/disassembly: Update to handle non-statement addresses

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] gdb/disassembly: Update to handle non-statement addresses

Andrew Burgess
After the introduction of support for non-statement addresses in the
line table, the output for 'disassemble /m' can be broken in some
cases.

With the /m format disassembly GDB associates a set of addresses with
each line, these addresses are then sorted and printed for each line.

When the non-statement support was added GDB was incorrectly told to
ignore non-statement instructions, and not add these to the result
table.  This means that these instructions are completely missing from
the output.

This commit removes the code that caused non-statement lines to be
ignored.

A result of this change is that GDB will now potentially include new
line numbers in the 'disassembly /m' output, lines that previously
were only in the line table as non-statement lines will now appear in
the disassembly output.  This feels like an improvement though.

gdb/ChangeLog:

        * disasm.c (do_mixed_source_and_assembly_deprecated): Don't
        exclude non-statement entries.

gdb/testsuite/ChangeLog:

        * gdb.dwarf2/dw2-disasm-over-non-stmt.exp: New file.
---
 gdb/ChangeLog                                 |   5 +
 gdb/disasm.c                                  |   6 -
 gdb/testsuite/ChangeLog                       |   4 +
 .../gdb.dwarf2/dw2-disasm-over-non-stmt.exp   | 192 ++++++++++++++++++
 4 files changed, 201 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-disasm-over-non-stmt.exp

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 143ba2f59b9..e45c8400689 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -376,12 +376,6 @@ do_mixed_source_and_assembly_deprecated
       if (le[i].line == le[i + 1].line && le[i].pc == le[i + 1].pc)
  continue; /* Ignore duplicates.  */
 
-      /* Ignore non-statement line table entries.  This means we print the
- source line at the place where GDB would insert a breakpoint for
- that line, which seems more intuitive.  */
-      if (le[i].is_stmt == 0)
- continue;
-
       /* Skip any end-of-function markers.  */
       if (le[i].line == 0)
  continue;
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-disasm-over-non-stmt.exp b/gdb/testsuite/gdb.dwarf2/dw2-disasm-over-non-stmt.exp
new file mode 100644
index 00000000000..6bcc5e77404
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-disasm-over-non-stmt.exp
@@ -0,0 +1,192 @@
+# Copyright 2020 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/>.
+
+# Create an example function that contains address marked as both
+# statements and non-statements then disassemble the function.
+#
+# Of particular interest is how 'disassemble /m' handles the
+# non-statement addresses, we want to ensure that these addresses are
+# included in the disassembly output.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+# The .c files use __attribute__.
+if [get_compiler_info] {
+    return -1
+}
+if !$gcc_compiled {
+    return 0
+}
+
+# Reuse many of the test source files from dw2-inline-header-1.exp.
+standard_testfile dw2-inline-header-lbls.c dw2-inline-header.S \
+    dw2-inline-header.c
+
+set build_options {nodebug optimize=-O1}
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile srcfile3 testfile
+    upvar build_options build_options
+    upvar start_label start_label
+    declare_labels lines_label
+
+    get_func_info main $build_options
+
+    cu {} {
+ compile_unit {
+    {producer "gcc" }
+    {language @DW_LANG_C}
+    {name ${srcfile3}}
+    {low_pc 0 addr}
+    {stmt_list ${lines_label} DW_FORM_sec_offset}
+ } {
+    subprogram {
+ {external 1 flag}
+ {name main}
+ {low_pc $main_start addr}
+ {high_pc "$main_start + $main_len" addr}
+    }
+ }
+    }
+
+    lines {version 2 default_is_stmt 1} lines_label {
+ include_dir "${srcdir}/${subdir}"
+ file_name "$srcfile3" 1
+
+ program {
+    {DW_LNE_set_address line_label_1}
+    {DW_LNS_advance_line 15}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_2}
+    {DW_LNS_negate_stmt}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_3}
+    {DW_LNS_advance_line 1}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_4}
+    {DW_LNS_negate_stmt}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_5}
+    {DW_LNS_negate_stmt}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_6}
+    {DW_LNS_advance_line 1}
+    {DW_LNS_negate_stmt}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_7}
+    {DW_LNS_copy}
+    {DW_LNE_end_sequence}
+ }
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+  [list $srcfile $asm_file] $build_options] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+# Now for the test.  Disassemble 'main' and split up the disassembly
+# output.  We build an associative array, for each line number store
+# the list of addresses that were part of its disassembly output.
+set linenum -1
+array set lines {}
+set addrs {}
+gdb_test_multiple "disassemble /m main" "" {
+    -re "Dump of assembler code for function main:\r\n" {
+ exp_continue
+    }
+
+    -re "^(\\d+)\\s+\[^\r\n\]+\r\n" {
+ if { $linenum != -1 } {
+    set lines($linenum) $addrs
+    set addrs {}
+ }
+ set linenum $expect_out(1,string)
+ exp_continue
+    }
+
+    -re "^(?:=>)?\\s*($hex)\\s*\[^\r\n\]+\r\n" {
+ set address $expect_out(1,string)
+ lappend addrs $address
+ exp_continue
+    }
+
+    -re "^\\s*\r\n" {
+ exp_continue
+    }
+
+    -re "^End of assembler dump\\.\r\n" {
+ if { $linenum != -1 } {
+    set lines($linenum) $addrs
+    set linenum -1
+    set addrs {}
+ }
+ exp_continue
+    }
+
+    -re "^$gdb_prompt $" {
+ # All done.
+    }
+}
+
+# Look in the global LINES array and check that the disassembly for
+# line LINENUM included the address of LABEL.
+proc check_disassembly_results { linenum label } {
+    global lines
+
+    set address [get_hexadecimal_valueof "&${label}" "__unknown__"]
+    set testname "check_disassembly_results $linenum $label"
+    if {![info exists lines($linenum)]} {
+ fail "$testname (no disassembly for $linenum)"
+ return
+    }
+
+    # Use a loop to compare the addresses as the addresses extracted
+    # from the disassembly output can be padded with zeros, while the
+    # address of the label will not be padded.
+    set addrs $lines($linenum)
+    foreach a $addrs {
+ if { $a == $address } {
+    pass $testname
+    return
+ }
+    }
+    fail $testname
+}
+
+# Now check that each label we expect to be associated with each line
+# shows up in the disassembly output.
+check_disassembly_results 16 "line_label_1"
+check_disassembly_results 16 "line_label_2"
+check_disassembly_results 17 "line_label_3"
+check_disassembly_results 17 "line_label_4"
+check_disassembly_results 17 "line_label_5"
+check_disassembly_results 18 "line_label_6"
--
2.25.4

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Sourceware - gdb-patches mailing list
On Wed, 22 Jul 2020 18:56:29 +0100
Andrew Burgess <[hidden email]> wrote:

> After the introduction of support for non-statement addresses in the
> line table, the output for 'disassemble /m' can be broken in some
> cases.
>
> With the /m format disassembly GDB associates a set of addresses with
> each line, these addresses are then sorted and printed for each line.
>
> When the non-statement support was added GDB was incorrectly told to
> ignore non-statement instructions, and not add these to the result
> table.  This means that these instructions are completely missing from
> the output.
>
> This commit removes the code that caused non-statement lines to be
> ignored.
>
> A result of this change is that GDB will now potentially include new
> line numbers in the 'disassembly /m' output, lines that previously
> were only in the line table as non-statement lines will now appear in
> the disassembly output.  This feels like an improvement though.
>
> gdb/ChangeLog:
>
> * disasm.c (do_mixed_source_and_assembly_deprecated): Don't
> exclude non-statement entries.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.dwarf2/dw2-disasm-over-non-stmt.exp: New file.

LGTM.

Kevin

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Pedro Alves-2
In reply to this post by Andrew Burgess
On 7/22/20 6:56 PM, Andrew Burgess wrote:
> +# Create an example function that contains address marked as both
> +# statements and non-statements then disassemble the function.

"address" -> "addresses" ?

BTW, this reads to me as the same address being marked as
statement and non-statement, which sounds impossible.

I would suggest:

 # Create an example function that contains both addresses marked
 # as statements and addresses marked as non-statements, and then
 # disassemble the function.

> + compile_unit {
> +    {producer "gcc" }
> +    {language @DW_LANG_C}
> +    {name ${srcfile3}}
> +    {low_pc 0 addr}
> +    {stmt_list ${lines_label} DW_FORM_sec_offset}
> + } {
> +    subprogram {
> + {external 1 flag}
> + {name main}
> + {low_pc $main_start addr}
> + {high_pc "$main_start + $main_len" addr}

I wonder whether this could use MACRO_AT_func ?

> +set build_options {nodebug optimize=-O1}

OOC, why does the testcase need -O1 ?

Otherwise this LGTM.

Thanks for writing the testcase -- it obviously took a lot more
work to write it than the GDB fix itself.

Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Simon Marchi-4
In reply to this post by Andrew Burgess
On 2020-07-22 1:56 p.m., Andrew Burgess wrote:

> After the introduction of support for non-statement addresses in the
> line table, the output for 'disassemble /m' can be broken in some
> cases.
>
> With the /m format disassembly GDB associates a set of addresses with
> each line, these addresses are then sorted and printed for each line.
>
> When the non-statement support was added GDB was incorrectly told to
> ignore non-statement instructions, and not add these to the result
> table.  This means that these instructions are completely missing from
> the output.
>
> This commit removes the code that caused non-statement lines to be
> ignored.
>
> A result of this change is that GDB will now potentially include new
> line numbers in the 'disassembly /m' output, lines that previously

disassembly -> disassemble

> were only in the line table as non-statement lines will now appear in
> the disassembly output.  This feels like an improvement though.

disassemble's /m switch is considered deprecated, because it's useless
(it shows the assembly in source line order).  The /s switch is the
"newer" one.  So I'd recommend testing with the /s switch, if not with
both switches.

I haven't looked at the patch itself and won't have time before next week.
But I see that Pedro looked at it already, so I can assume it's fine :).

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Andrew Burgess
* Simon Marchi <[hidden email]> [2020-07-22 16:30:59 -0400]:

> On 2020-07-22 1:56 p.m., Andrew Burgess wrote:
> > After the introduction of support for non-statement addresses in the
> > line table, the output for 'disassemble /m' can be broken in some
> > cases.
> >
> > With the /m format disassembly GDB associates a set of addresses with
> > each line, these addresses are then sorted and printed for each line.
> >
> > When the non-statement support was added GDB was incorrectly told to
> > ignore non-statement instructions, and not add these to the result
> > table.  This means that these instructions are completely missing from
> > the output.
> >
> > This commit removes the code that caused non-statement lines to be
> > ignored.
> >
> > A result of this change is that GDB will now potentially include new
> > line numbers in the 'disassembly /m' output, lines that previously
>
> disassembly -> disassemble
>
> > were only in the line table as non-statement lines will now appear in
> > the disassembly output.  This feels like an improvement though.
>
> disassemble's /m switch is considered deprecated, because it's useless
> (it shows the assembly in source line order).  The /s switch is the
> "newer" one.  So I'd recommend testing with the /s switch, if not with
> both switches.
>
> I haven't looked at the patch itself and won't have time before next week.
> But I see that Pedro looked at it already, so I can assume it's fine :).

I'll extend the test case, but the bug and fix is only in the /m path
as it's a flaw in the logic for how instructions are associated with
lines.

FYI, I don't really consider /m "deprecated".  We don't have any plan
for how we could ever remove /m from GDB, so it's probably going to
live on forever.  That doesn't really feel like something that's
deprecated to me, just something that got known issues.

FYI(2), I only use /s myself because /m is junk :)

Thanks,
Andrew
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Andrew Burgess
In reply to this post by Andrew Burgess
Thanks for all the reviews.

Below is the final version of the patch that I pushed.

  - Fixed the typos pointed out,
  - No longer compiling the test at -O1,
  - Make use of MACRO_AT_func to generate DWARF.
  - Test /s and /m disassembly.

Thanks,
Andrew

---

commit 78344df7b5d7d7fcf6aa7945b8c4b56bcc9388ce
Author: Andrew Burgess <[hidden email]>
Date:   Tue Jul 21 11:21:50 2020 +0100

    gdb/disassembly: Update to handle non-statement addresses
   
    After the introduction of support for non-statement addresses in the
    line table, the output for 'disassemble /m' can be broken in some
    cases.
   
    With the /m format disassembly GDB associates a set of addresses with
    each line, these addresses are then sorted and printed for each line.
   
    When the non-statement support was added GDB was incorrectly told to
    ignore non-statement instructions, and not add these to the result
    table.  This means that these instructions are completely missing from
    the output.
   
    This commit removes the code that caused non-statement lines to be
    ignored.
   
    A result of this change is that GDB will now potentially include new
    line numbers in the 'disassemble /m' output, lines that previously
    were only in the line table as non-statement lines will now appear in
    the disassembly output.  This feels like an improvement though.
   
    gdb/ChangeLog:
   
            * disasm.c (do_mixed_source_and_assembly_deprecated): Don't
            exclude non-statement entries.
   
    gdb/testsuite/ChangeLog:
   
            * gdb.dwarf2/dw2-disasm-over-non-stmt.exp: New file.

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 143ba2f59b9..e45c8400689 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -376,12 +376,6 @@ do_mixed_source_and_assembly_deprecated
       if (le[i].line == le[i + 1].line && le[i].pc == le[i + 1].pc)
  continue; /* Ignore duplicates.  */
 
-      /* Ignore non-statement line table entries.  This means we print the
- source line at the place where GDB would insert a breakpoint for
- that line, which seems more intuitive.  */
-      if (le[i].is_stmt == 0)
- continue;
-
       /* Skip any end-of-function markers.  */
       if (le[i].line == 0)
  continue;
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-disasm-over-non-stmt.exp b/gdb/testsuite/gdb.dwarf2/dw2-disasm-over-non-stmt.exp
new file mode 100644
index 00000000000..6df275f1b11
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-disasm-over-non-stmt.exp
@@ -0,0 +1,206 @@
+# Copyright 2020 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/>.
+
+# Create an example function that contains both addresses marked as
+# statements and addresses marked as non-statements, and then
+# disassemble the function.
+#
+# Of particular interest is how 'disassemble /m' handles the
+# non-statement addresses, we want to ensure that these addresses are
+# included in the disassembly output.  For completeness we test both
+# 'disassemble /m' and 'disassemble /s'.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+# The .c files use __attribute__.
+if [get_compiler_info] {
+    return -1
+}
+if !$gcc_compiled {
+    return 0
+}
+
+# Reuse many of the test source files from dw2-inline-header-1.exp.
+standard_testfile dw2-inline-header-lbls.c dw2-inline-header.S \
+    dw2-inline-header.c
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile srcfile3
+    declare_labels lines_label
+
+    get_func_info main
+
+    cu {} {
+ compile_unit {
+    {producer "gcc" }
+    {language @DW_LANG_C}
+    {name ${srcfile3}}
+    {low_pc 0 addr}
+    {stmt_list ${lines_label} DW_FORM_sec_offset}
+ } {
+    subprogram {
+ {external 1 flag}
+ {MACRO_AT_func {main}}
+    }
+ }
+    }
+
+    lines {version 2 default_is_stmt 1} lines_label {
+ include_dir "${srcdir}/${subdir}"
+ file_name "$srcfile3" 1
+
+ program {
+    {DW_LNE_set_address $main_start}
+    {DW_LNS_advance_line 15}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_2}
+    {DW_LNS_negate_stmt}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_3}
+    {DW_LNS_advance_line 1}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_4}
+    {DW_LNS_negate_stmt}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_5}
+    {DW_LNS_negate_stmt}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address line_label_6}
+    {DW_LNS_advance_line 1}
+    {DW_LNS_negate_stmt}
+    {DW_LNS_copy}
+
+    {DW_LNE_set_address $main_end}
+    {DW_LNS_copy}
+    {DW_LNE_end_sequence}
+ }
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+  [list $srcfile $asm_file] {nodebug} ] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+# Global lines array, maps lines numbers to the list of addresses
+# associated with that line in the debug output.
+array set lines {}
+
+# Look in the global LINES array and check that the disassembly for
+# line LINENUM includes the address of LABEL.
+proc check_disassembly_results { linenum label } {
+    global lines
+
+    set address [get_hexadecimal_valueof "&${label}" "__unknown__"]
+    set testname "check_disassembly_results $linenum $label"
+    if {![info exists lines($linenum)]} {
+ fail "$testname (no disassembly for $linenum)"
+ return
+    }
+
+    # Use a loop to compare the addresses as the addresses extracted
+    # from the disassembly output can be padded with zeros, while the
+    # address of the label will not be padded.
+    set addrs $lines($linenum)
+    foreach a $addrs {
+ if { $a == $address } {
+    pass $testname
+    return
+ }
+    }
+    fail $testname
+}
+
+foreach_with_prefix opt { m s } {
+    # Disassemble 'main' and split up the disassembly output.  We
+    # build an associative array, for each line number store the list
+    # of addresses that were part of its disassembly output.
+    #
+    # LINENUM is the line we are currently collecting the disassembly
+    # addresses for, and ADDRS is the list of addresses collected for
+    # this line.
+    set linenum -1
+    set addrs {}
+
+    # Clear the global associative array used to hold the results.
+    unset lines
+    array set lines {}
+
+    gdb_test_multiple "disassemble /${opt} main" "" {
+ -re "Dump of assembler code for function main:\r\n" {
+    exp_continue
+ }
+
+ -re "^\[^\r\n\]+${srcfile3}:" {
+    exp_continue
+ }
+
+ -re "^(\\d+)\\s+\[^\r\n\]+\r\n" {
+    if { $linenum != -1 } {
+ set lines($linenum) $addrs
+ set addrs {}
+    }
+    set linenum $expect_out(1,string)
+    exp_continue
+ }
+
+ -re "^(?:=>)?\\s*($hex)\\s*\[^\r\n\]+\r\n" {
+    set address $expect_out(1,string)
+    lappend addrs $address
+    exp_continue
+ }
+
+ -re "^\\s*\r\n" {
+    exp_continue
+ }
+
+ -re "^End of assembler dump\\.\r\n" {
+    if { $linenum != -1 } {
+ set lines($linenum) $addrs
+ set linenum -1
+ set addrs {}
+    }
+    exp_continue
+ }
+
+ -re "^$gdb_prompt $" {
+    # All done.
+ }
+    }
+
+    # Now check that each label we expect to be associated with each line
+    # shows up in the disassembly output.
+    check_disassembly_results 16 "line_label_1"
+    check_disassembly_results 16 "line_label_2"
+    check_disassembly_results 17 "line_label_3"
+    check_disassembly_results 17 "line_label_4"
+    check_disassembly_results 17 "line_label_5"
+    check_disassembly_results 18 "line_label_6"
+}
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Simon Marchi-4
In reply to this post by Andrew Burgess
On 2020-07-23 4:48 a.m., Andrew Burgess wrote:
> I'll extend the test case, but the bug and fix is only in the /m path
> as it's a flaw in the logic for how instructions are associated with
> lines.

I don't understand... /s also associates instructions to line, no?

> FYI, I don't really consider /m "deprecated".  We don't have any plan
> for how we could ever remove /m from GDB, so it's probably going to
> live on forever.  That doesn't really feel like something that's
> deprecated to me, just something that got known issues.

You are right (unless we do decide to remove it :)).

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Pedro Alves-2
In reply to this post by Andrew Burgess
On 7/23/20 9:48 AM, Andrew Burgess wrote:

> FYI, I don't really consider /m "deprecated".  We don't have any plan
> for how we could ever remove /m from GDB, so it's probably going to
> live on forever.  That doesn't really feel like something that's
> deprecated to me, just something that got known issues.

Note that "help disassemble" explicitly says that it is deprecated:

 With a /m modifier, source lines are included (if available).
 This view is "source centric": the output is in source line order,
 regardless of any optimization that is present.  Only the main source file
 is displayed, not those of, e.g., any inlined functions.
 This modifier hasn't proved useful in practice and is deprecated
                                                       ^^^^^^^^^^
 in favor of /s.

 With a /s modifier, source lines are included (if available).
 This differs from /m in two important respects:
 - the output is still in pc address order, and
 - file names and contents for all relevant source files are displayed.

I've always thought that these two paragraphs are backwards -- i.e.,
that we should describe /s first since it's the preferred format,
and then /m should be described in comparison to /s.

> FYI(2), I only use /s myself because /m is junk :)


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Andrew Burgess
* Pedro Alves <[hidden email]> [2020-07-23 13:01:30 +0100]:

> On 7/23/20 9:48 AM, Andrew Burgess wrote:
>
> > FYI, I don't really consider /m "deprecated".  We don't have any plan
> > for how we could ever remove /m from GDB, so it's probably going to
> > live on forever.  That doesn't really feel like something that's
> > deprecated to me, just something that got known issues.
>
> Note that "help disassemble" explicitly says that it is deprecated:
>
>  With a /m modifier, source lines are included (if available).
>  This view is "source centric": the output is in source line order,
>  regardless of any optimization that is present.  Only the main source file
>  is displayed, not those of, e.g., any inlined functions.
>  This modifier hasn't proved useful in practice and is deprecated
>                                                        ^^^^^^^^^^
>  in favor of /s.
>
>  With a /s modifier, source lines are included (if available).
>  This differs from /m in two important respects:
>  - the output is still in pc address order, and
>  - file names and contents for all relevant source files are displayed.
>
> I've always thought that these two paragraphs are backwards -- i.e.,
> that we should describe /s first since it's the preferred format,
> and then /m should be described in comparison to /s.

Sure, I'm absolutely not arguing about whether it is, or is not
deprecated.  I guess what I'm suggesting is that without a formal
project wide definition of what "deprecated" means then it's a pretty
meaningless label to apply to something (in my opinion).

For example, in this case, we have a command that's deprecated.

Is that command going to be removed at some point?  If we do, what
about user scripts that depend on it?  This particular command is
easily available through the MI too, so there are probably frontends
that depend on this command.

Does that mean we should never fix bugs in it, and it should be
allowed to bit-rot away until eventually it's so useless everyone
agrees we can remove it?

Does it mean it must produce the exact same output, given the same
input, for all time, even if we have to jump though hoops to maintain
that output?

Or can the command continue to evolve just like non-deprecated GDB
commands?

My suspicion is that the command will never be removed unless keeping
it in GDB somehow blocks the addition of some other, more useful
command, which feels pretty unlikely in this case.

And the reality is the command continues to evolve just like any other
command.

So for me, this should never have been deprecated at all.  It's a
command, it exists, it has some known limitations which should be
documented, the alternative (/s) command should (as you suggest) be
pushed as the "preferred" approach, but until we actually have a plan
to remove this from GDB, it shouldn't be marked as deprecated.

Thanks,
Andrew




Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Andrew Burgess
In reply to this post by Simon Marchi-4
* Simon Marchi <[hidden email]> [2020-07-23 06:43:38 -0400]:

> On 2020-07-23 4:48 a.m., Andrew Burgess wrote:
> > I'll extend the test case, but the bug and fix is only in the /m path
> > as it's a flaw in the logic for how instructions are associated with
> > lines.
>
> I don't understand... /s also associates instructions to line, no?

Yes, but not in the same way.  /m creates a map of line numbers to
lists of addresses, then sorts all the map entries by line number,
then by address, and finally prints the result.

By contrast /s just walks the addresses and prints which line each
address maps to.

The bug in this case was that non-is-stmt lines were specifically NOT
added into the map as a result those instructions were just missing
from the output.

This is basically the same bug that resulting in /m being marked
deprecated in the first place.  Inline code blocks already result in
"missing" instructions from the disassembly output, but dropping
non-is-stmt instructions just made this even worse, and was a
regression from what came before.

The /m output is now (with this fix) back to how it was before I added
the is-stmt support.

Thanks,
Andrew



>
> > FYI, I don't really consider /m "deprecated".  We don't have any plan
> > for how we could ever remove /m from GDB, so it's probably going to
> > live on forever.  That doesn't really feel like something that's
> > deprecated to me, just something that got known issues.
>
> You are right (unless we do decide to remove it :)).
>
> Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Pedro Alves-2
In reply to this post by Andrew Burgess
On 7/23/20 2:52 PM, Andrew Burgess wrote:

> * Pedro Alves <[hidden email]> [2020-07-23 13:01:30 +0100]:
>
>> On 7/23/20 9:48 AM, Andrew Burgess wrote:
>>
>>> FYI, I don't really consider /m "deprecated".  We don't have any plan
>>> for how we could ever remove /m from GDB, so it's probably going to
>>> live on forever.  That doesn't really feel like something that's
>>> deprecated to me, just something that got known issues.
>>
>> Note that "help disassemble" explicitly says that it is deprecated:
>>
>>  With a /m modifier, source lines are included (if available).
>>  This view is "source centric": the output is in source line order,
>>  regardless of any optimization that is present.  Only the main source file
>>  is displayed, not those of, e.g., any inlined functions.
>>  This modifier hasn't proved useful in practice and is deprecated
>>                                                        ^^^^^^^^^^
>>  in favor of /s.
>>
>>  With a /s modifier, source lines are included (if available).
>>  This differs from /m in two important respects:
>>  - the output is still in pc address order, and
>>  - file names and contents for all relevant source files are displayed.
>>
>> I've always thought that these two paragraphs are backwards -- i.e.,
>> that we should describe /s first since it's the preferred format,
>> and then /m should be described in comparison to /s.
>
> Sure, I'm absolutely not arguing about whether it is, or is not
> deprecated.  

And I'm just pointing out that we tell users that it is deprecated.

Don't shoot the messenger!  :-)

> I guess what I'm suggesting is that without a formal
> project wide definition of what "deprecated" means then it's a pretty
> meaningless label to apply to something (in my opinion).

It seems well established that "deprecated" means "don't rely on it,
because it will be removed at some point".

>
> For example, in this case, we have a command that's deprecated.
>
> Is that command going to be removed at some point?  If we do, what
> about user scripts that depend on it?  This particular command is
> easily available through the MI too, so there are probably frontends
> that depend on this command.

AFAICS, MI frontends that use the CLI disassemble command was the
reason /s was introduced over just changing /m.

 https://sourceware.org/legacy-ml/gdb-patches/2015-08/msg00057.html

Doug didn't say which frontend it was that he found was using
the CLI disassemble command...  But who knows what scripts are
out there using /m.  Not that I'm super sympathetic to scripts
parsing CLI output.  I think scripts could get the same info with
Python's Architecture.disassemble and LineTable APIs.
Maybe it could be made easier by making Architecture.disassemble
support including the line numbers in its output.

Anyhow, I'm not looking at coming up with a plan myself right now.
I was just pointing out what we tell users.  Maybe yeah,
we should stop telling users (both online help and user manual)
that /m is deprecated, and instead tweak the docs to
better guide them toward /s instead.  And then maybe someday, if
somebody comes with the patience to champion it, we might be
able to make /m an alias for /s.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Tom Tromey-2
In reply to this post by Pedro Alves-2
Pedro> I've always thought that these two paragraphs are backwards -- i.e.,
Pedro> that we should describe /s first since it's the preferred format,
Pedro> and then /m should be described in comparison to /s.

How about the appended?

Tom

commit 007c27fe2c11a35d7f8b6b13077b6d42dffb06bd
Author: Tom Tromey <[hidden email]>
Date:   Thu Jul 23 14:43:11 2020 -0600

    Update "disassemble" help
   
    Pedro pointed out that disassemble/m should be documented after
    disassemble/s, because /m is deprecated.  This patch does so, and adds
    a usage line.
   
    Regression tested on x86-64 Fedora 32.
   
    gdb/ChangeLog
    2020-07-23  Tom Tromey  <[hidden email]>
   
            * cli/cli-cmds.c (_initialize_cli_cmds): Rearrange "disassemble"
            help.  Add usage.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 37035206f49..f0732ef0331 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2020-07-23  Tom Tromey  <[hidden email]>
+
+ * cli/cli-cmds.c (_initialize_cli_cmds): Rearrange "disassemble"
+ help.  Add usage.
+
 2020-07-23  Tom de Vries  <[hidden email]>
 
  PR tui/26282
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 14718d1181a..300f7725cb0 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -2528,8 +2528,13 @@ can be shown using \"show listsize\"."));
 
   c = add_com ("disassemble", class_vars, disassemble_command, _("\
 Disassemble a specified section of memory.\n\
+Usage: disassemble[/m|/r|/s] START [, END]\n\
 Default is the function surrounding the pc of the selected frame.\n\
 \n\
+With a /s modifier, source lines are included (if available).\n\
+In this mode, the output is still in PC address order, and\n\
+file names and contents for all relevant source files are displayed.\n\
+\n\
 With a /m modifier, source lines are included (if available).\n\
 This view is \"source centric\": the output is in source line order,\n\
 regardless of any optimization that is present.  Only the main source file\n\
@@ -2537,11 +2542,6 @@ is displayed, not those of, e.g., any inlined functions.\n\
 This modifier hasn't proved useful in practice and is deprecated\n\
 in favor of /s.\n\
 \n\
-With a /s modifier, source lines are included (if available).\n\
-This differs from /m in two important respects:\n\
-- the output is still in pc address order, and\n\
-- file names and contents for all relevant source files are displayed.\n\
-\n\
 With a /r modifier, raw instructions in hex are included.\n\
 \n\
 With a single argument, the function surrounding that address is dumped.\n\
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Andrew Burgess
* Tom Tromey <[hidden email]> [2020-07-23 14:44:40 -0600]:

> Pedro> I've always thought that these two paragraphs are backwards -- i.e.,
> Pedro> that we should describe /s first since it's the preferred format,
> Pedro> and then /m should be described in comparison to /s.
>
> How about the appended?
>
> Tom
>
> commit 007c27fe2c11a35d7f8b6b13077b6d42dffb06bd
> Author: Tom Tromey <[hidden email]>
> Date:   Thu Jul 23 14:43:11 2020 -0600
>
>     Update "disassemble" help
>    
>     Pedro pointed out that disassemble/m should be documented after
>     disassemble/s, because /m is deprecated.  This patch does so, and adds
>     a usage line.
>    
>     Regression tested on x86-64 Fedora 32.
>    
>     gdb/ChangeLog
>     2020-07-23  Tom Tromey  <[hidden email]>
>    
>             * cli/cli-cmds.c (_initialize_cli_cmds): Rearrange "disassemble"
>             help.  Add usage.
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 37035206f49..f0732ef0331 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2020-07-23  Tom Tromey  <[hidden email]>
> +
> + * cli/cli-cmds.c (_initialize_cli_cmds): Rearrange "disassemble"
> + help.  Add usage.
> +
>  2020-07-23  Tom de Vries  <[hidden email]>
>  
>   PR tui/26282
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index 14718d1181a..300f7725cb0 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -2528,8 +2528,13 @@ can be shown using \"show listsize\"."));
>  
>    c = add_com ("disassemble", class_vars, disassemble_command, _("\
>  Disassemble a specified section of memory.\n\
> +Usage: disassemble[/m|/r|/s] START [, END]\n\
>  Default is the function surrounding the pc of the selected frame.\n\
>  \n\
> +With a /s modifier, source lines are included (if available).\n\
> +In this mode, the output is still in PC address order, and\n\
> +file names and contents for all relevant source files are displayed.\n\

Doesn't really make sense any more, "output is still in PC address",
the use of 'still' seems odd as this is the first description for this
command.  How about replacing it with 'displayed'.

> +\n\
>  With a /m modifier, source lines are included (if available).\n\
>  This view is \"source centric\": the output is in source line order,\n\
>  regardless of any optimization that is present.  Only the main source file\n\
> @@ -2537,11 +2542,6 @@ is displayed, not those of, e.g., any inlined functions.\n\
>  This modifier hasn't proved useful in practice and is deprecated\n\
>  in favor of /s.\n\
>  \n\
> -With a /s modifier, source lines are included (if available).\n\
> -This differs from /m in two important respects:\n\
> -- the output is still in pc address order, and\n\
> -- file names and contents for all relevant source files are displayed.\n\
> -\n\
>  With a /r modifier, raw instructions in hex are included.\n\
>  \n\
>  With a single argument, the function surrounding that address is dumped.\n\
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Andrew Burgess
In reply to this post by Pedro Alves-2
* Pedro Alves <[hidden email]> [2020-07-23 19:23:40 +0100]:

> On 7/23/20 2:52 PM, Andrew Burgess wrote:
> > * Pedro Alves <[hidden email]> [2020-07-23 13:01:30 +0100]:
> >
> >> On 7/23/20 9:48 AM, Andrew Burgess wrote:
> >>
> >>> FYI, I don't really consider /m "deprecated".  We don't have any plan
> >>> for how we could ever remove /m from GDB, so it's probably going to
> >>> live on forever.  That doesn't really feel like something that's
> >>> deprecated to me, just something that got known issues.
> >>
> >> Note that "help disassemble" explicitly says that it is deprecated:
> >>
> >>  With a /m modifier, source lines are included (if available).
> >>  This view is "source centric": the output is in source line order,
> >>  regardless of any optimization that is present.  Only the main source file
> >>  is displayed, not those of, e.g., any inlined functions.
> >>  This modifier hasn't proved useful in practice and is deprecated
> >>                                                        ^^^^^^^^^^
> >>  in favor of /s.
> >>
> >>  With a /s modifier, source lines are included (if available).
> >>  This differs from /m in two important respects:
> >>  - the output is still in pc address order, and
> >>  - file names and contents for all relevant source files are displayed.
> >>
> >> I've always thought that these two paragraphs are backwards -- i.e.,
> >> that we should describe /s first since it's the preferred format,
> >> and then /m should be described in comparison to /s.
> >
> > Sure, I'm absolutely not arguing about whether it is, or is not
> > deprecated.  
>
> And I'm just pointing out that we tell users that it is deprecated.
>
> Don't shoot the messenger!  :-)

Sorry if I gave that impression.

We only ended up on this path because it was originally mentioned on a
patch to fix a bug in /m - "hey this command is deprecated".

At no point was I told, "hey this command is deprecated, so don't fix
bugs here".  But nor was any particular meaning attached to "hey this
command is deprecated" - so I looked for some meaning, and the only
meaning I could find (in my own mind) was " ... so why fix bugs here".

My reply was intended to mean, sure its marked deprecated, BUT, until
there's a plan to remove something, that means nothing (beyond we
might gently nudge users towards other options).

If my poor communication has given the impression that I was disputing
whether this is deprecated then I really do apologise, that was never
my intention.  I'll try harder to be clearer in future.

.... and maybe just ask when people way "hey, this command is
deprecated", rather than hunting for hidden meaning :)

Thanks for all your time,

Andrew

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Pedro Alves-2
On 7/24/20 10:22 AM, Andrew Burgess wrote:

> * Pedro Alves <[hidden email]> [2020-07-23 19:23:40 +0100]:
>
>> On 7/23/20 2:52 PM, Andrew Burgess wrote:
>>> * Pedro Alves <[hidden email]> [2020-07-23 13:01:30 +0100]:
>>>
>>>> On 7/23/20 9:48 AM, Andrew Burgess wrote:
>>>>
>>>>> FYI, I don't really consider /m "deprecated".  We don't have any plan
>>>>> for how we could ever remove /m from GDB, so it's probably going to
>>>>> live on forever.  That doesn't really feel like something that's
>>>>> deprecated to me, just something that got known issues.
>>>>
>>>> Note that "help disassemble" explicitly says that it is deprecated:
>>>>
>>>>  With a /m modifier, source lines are included (if available).
>>>>  This view is "source centric": the output is in source line order,
>>>>  regardless of any optimization that is present.  Only the main source file
>>>>  is displayed, not those of, e.g., any inlined functions.
>>>>  This modifier hasn't proved useful in practice and is deprecated
>>>>                                                        ^^^^^^^^^^
>>>>  in favor of /s.
>>>>
>>>>  With a /s modifier, source lines are included (if available).
>>>>  This differs from /m in two important respects:
>>>>  - the output is still in pc address order, and
>>>>  - file names and contents for all relevant source files are displayed.
>>>>
>>>> I've always thought that these two paragraphs are backwards -- i.e.,
>>>> that we should describe /s first since it's the preferred format,
>>>> and then /m should be described in comparison to /s.
>>>
>>> Sure, I'm absolutely not arguing about whether it is, or is not
>>> deprecated.  
>>
>> And I'm just pointing out that we tell users that it is deprecated.
>>
>> Don't shoot the messenger!  :-)
>
> Sorry if I gave that impression.
>
> We only ended up on this path because it was originally mentioned on a
> patch to fix a bug in /m - "hey this command is deprecated".
>
> At no point was I told, "hey this command is deprecated, so don't fix
> bugs here".  But nor was any particular meaning attached to "hey this
> command is deprecated" - so I looked for some meaning, and the only
> meaning I could find (in my own mind) was " ... so why fix bugs here".
>
> My reply was intended to mean, sure its marked deprecated, BUT, until
> there's a plan to remove something, that means nothing (beyond we
> might gently nudge users towards other options).
>
> If my poor communication has given the impression that I was disputing
> whether this is deprecated then I really do apologise, that was never
> my intention.  I'll try harder to be clearer in future.

I understood it (or maybe misunderstood it) that you hadn't
realized that the option was explicitly documented as deprecated,
so I was just trying to point out that regardless of our current
opinion, we have been telling users that it it indeed is (deprecated).

>
> .... and maybe just ask when people way "hey, this command is
> deprecated", rather than hunting for hidden meaning :)
>

IMO, if we decide to make a command or option deprecated rather than
just removing it completely, it must mean that we expect that users
are still using it.  So I think we should make sure deprecated
features don't regress as long as we have them.

> Thanks for all your time,

Likewise, thanks to you too.

Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Pedro Alves-2
In reply to this post by Andrew Burgess
On 7/24/20 10:13 AM, Andrew Burgess wrote:

> * Tom Tromey <[hidden email]> [2020-07-23 14:44:40 -0600]:
>
>> Pedro> I've always thought that these two paragraphs are backwards -- i.e.,
>> Pedro> that we should describe /s first since it's the preferred format,
>> Pedro> and then /m should be described in comparison to /s.
>>
>> How about the appended?
>>
>> Tom
>>
>> commit 007c27fe2c11a35d7f8b6b13077b6d42dffb06bd
>> Author: Tom Tromey <[hidden email]>
>> Date:   Thu Jul 23 14:43:11 2020 -0600
>>
>>     Update "disassemble" help
>>    
>>     Pedro pointed out that disassemble/m should be documented after
>>     disassemble/s, because /m is deprecated.  This patch does so, and adds
>>     a usage line.
>>    
>>     Regression tested on x86-64 Fedora 32.
>>    
>>     gdb/ChangeLog
>>     2020-07-23  Tom Tromey  <[hidden email]>
>>    
>>             * cli/cli-cmds.c (_initialize_cli_cmds): Rearrange "disassemble"
>>             help.  Add usage.
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 37035206f49..f0732ef0331 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2020-07-23  Tom Tromey  <[hidden email]>
>> +
>> + * cli/cli-cmds.c (_initialize_cli_cmds): Rearrange "disassemble"
>> + help.  Add usage.
>> +
>>  2020-07-23  Tom de Vries  <[hidden email]>
>>  
>>   PR tui/26282
>> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
>> index 14718d1181a..300f7725cb0 100644
>> --- a/gdb/cli/cli-cmds.c
>> +++ b/gdb/cli/cli-cmds.c
>> @@ -2528,8 +2528,13 @@ can be shown using \"show listsize\"."));
>>  
>>    c = add_com ("disassemble", class_vars, disassemble_command, _("\
>>  Disassemble a specified section of memory.\n\
>> +Usage: disassemble[/m|/r|/s] START [, END]\n\
>>  Default is the function surrounding the pc of the selected frame.\n\
>>  \n\
>> +With a /s modifier, source lines are included (if available).\n\
>> +In this mode, the output is still in PC address order, and\n\
>> +file names and contents for all relevant source files are displayed.\n\
>
> Doesn't really make sense any more, "output is still in PC address",
> the use of 'still' seems odd as this is the first description for this
> command.  How about replacing it with 'displayed'.

+1

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Tom Tromey-2
In reply to this post by Andrew Burgess
>> +With a /s modifier, source lines are included (if available).\n\
>> +In this mode, the output is still in PC address order, and\n\
>> +file names and contents for all relevant source files are displayed.\n\

Andrew> Doesn't really make sense any more, "output is still in PC address",
Andrew> the use of 'still' seems odd as this is the first description for this
Andrew> command.  How about replacing it with 'displayed'.

Works for me, I made this change locally.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] gdb/disassembly: Update to handle non-statement addresses

Tom Tromey-2
>>>>> "Tom" == Tom Tromey <[hidden email]> writes:

>>> +With a /s modifier, source lines are included (if available).\n\
>>> +In this mode, the output is still in PC address order, and\n\
>>> +file names and contents for all relevant source files are displayed.\n\

Andrew> Doesn't really make sense any more, "output is still in PC address",
Andrew> the use of 'still' seems odd as this is the first description for this
Andrew> command.  How about replacing it with 'displayed'.

Tom> Works for me, I made this change locally.

I'm checking in the updated patch.

Tom