[RFC][gdb] Add maint set skip-prologue

[RFC][gdb] Add maint set skip-prologue

[Tom de Vries]

Hi,

With aarch64, I run into:
...
(gdb) break callee.adb:22^M
Breakpoint 1 at 0x401f78: file caller.adb, line 21.^M
(gdb) FAIL: gdb.ada/inline-section-gc.exp: break callee.adb:22
...
while this is expected (copied from x86_64):
...
(gdb) break callee.adb:22^M
Breakpoint 1 at 0x401f10: file callee.adb, line 22.^M
(gdb) PASS: gdb.ada/inline-section-gc.exp: break callee.adb:22
...

The difference comes from the gdbarch_skip_prologue implementation.

For x86_64, we have:
...
  if (find_pc_partial_function (start_pc, NULL, &func_addr, NULL))
    {
      CORE_ADDR post_prologue_pc
	= skip_prologue_using_sal (gdbarch, func_addr);
      struct compunit_symtab *cust = find_pc_compunit_symtab (func_addr);

      /* LLVM backend (Clang/Flang) always emits a line note before the
	 prologue and another one after.  We trust clang and newer Intel
	 compilers to emit usable line notes.  */
      if (post_prologue_pc
	  && (cust != NULL
	      && cust->producer () != nullptr
	      && (producer_is_llvm (cust->producer ())
	      || producer_is_icc_ge_19 (cust->producer ()))))
        return std::max (start_pc, post_prologue_pc);
    }
...
and because the producer is GCC, we don't trust the line table result, and use the
architecture-specific prologue analyzer, which skips no insns.

For aarch64, we have:
...
  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
    {
      CORE_ADDR post_prologue_pc
	= skip_prologue_using_sal (gdbarch, func_addr);

      if (post_prologue_pc != 0)
	return std::max (pc, post_prologue_pc);
    }
...
and consequently we trust the line table result and skip past a few insns,
which breaks test-case assumptions.

Add a command "maint set skip-prologue line-table", that forces usage of the
line-table result if available, which allows us to reproduce the same problem
on x86_64-linux.

Also add a command "maint set skip-prologue none", that forces skipping no
insn, which allows us to fix the FAIL on aarch64.

Fix a few more test-cases likewise, and fix a few other test-cases by
specifying the breakpoint location in a way that's independent from the
prologue.

Tested on x86_64-linux and aarch64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29419
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29409

Any comments?

Thanks,
- Tom

[gdb] Add maint set skip-prologue

---
 gdb/arch-utils.c                              | 27 +++++++++++-
 gdb/maint.c                                   | 59 +++++++++++++++++++++++++++
 gdb/maint.h                                   |  9 ++++
 gdb/testsuite/gdb.ada/access_tagged_param.exp |  2 +-
 gdb/testsuite/gdb.ada/inline-section-gc.exp   |  2 +
 gdb/testsuite/gdb.ada/ptype_tagged_param.exp  |  2 +-
 gdb/testsuite/gdb.ada/ref_param.exp           |  2 +-
 gdb/testsuite/gdb.opt/inline-small-func.exp   |  2 +
 8 files changed, 101 insertions(+), 4 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 9c6b9268a69..852d9236236 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -36,6 +36,7 @@
 #include "reggroups.h"
 #include "auxv.h"
 #include "observable.h"
+#include "maint.h"
 
 #include "gdbsupport/version.h"
 
@@ -1041,6 +1042,23 @@ default_print_insn (bfd_vma memaddr, disassemble_info *info)
   return (*disassemble_fn) (memaddr, info);
 }
 
+/* Helper function for gdbarch_skip_prologue_noexcept.  */
+
+static CORE_ADDR
+gdbarch_skip_prologue_line_table (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  CORE_ADDR func_addr;
+
+  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
+    {
+      CORE_ADDR post_prologue_pc
+	= skip_prologue_using_sal (gdbarch, func_addr);
+      return std::max (pc, post_prologue_pc);
+    }
+
+  return gdbarch_skip_prologue (gdbarch, pc);
+}
+
 /* See arch-utils.h.  */
 
 CORE_ADDR
@@ -1050,7 +1068,14 @@ gdbarch_skip_prologue_noexcept (gdbarch *gdbarch, CORE_ADDR pc) noexcept
 
   try
     {
-      new_pc = gdbarch_skip_prologue (gdbarch, pc);
+      if (skip_prologue_method == spm_default)
+	new_pc = gdbarch_skip_prologue (gdbarch, pc);
+      else if (skip_prologue_method == spm_line_table)
+	new_pc = gdbarch_skip_prologue_line_table (gdbarch, pc);
+      else if (skip_prologue_method == spm_none)
+	new_pc = pc;
+      else
+	gdb_assert_not_reached ("");
     }
   catch (const gdb_exception &ex)
     {}
diff --git a/gdb/maint.c b/gdb/maint.c
index 76ac7bece38..1a04d777bdc 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1204,6 +1204,55 @@ Selftests have been disabled for this build.\n"));
 #endif
 }
 
+/* See maint.h.  */
+
+spm_value skip_prologue_method = spm_default;
+
+/* Valid string arguments for maint set skip-prologue.  */
+
+static const char skip_prologue_method_default[] = "default";
+static const char skip_prologue_method_line_table[] = "line-table";
+static const char skip_prologue_method_line_none[] = "none";
+
+/* List of all valid string arguments for maint set skip-prologue.  */
+
+static const char *skip_prologue_method_enum[] = {
+  skip_prologue_method_default,
+  skip_prologue_method_line_table,
+  skip_prologue_method_line_none,
+  nullptr
+};
+
+/* Current string argument for maint set skip-prologue.  */
+
+static const char *skip_prologue_method_value = skip_prologue_method_default;
+
+/* Set skip_prologue_method.  */
+
+static void
+set_skip_prologue_method_command (const char *args, int from_tty,
+				  cmd_list_element *c)
+{
+  if (skip_prologue_method_value == skip_prologue_method_default)
+    skip_prologue_method = spm_default;
+  else if (skip_prologue_method_value == skip_prologue_method_line_table)
+    skip_prologue_method = spm_line_table;
+  else if (skip_prologue_method_value == skip_prologue_method_line_none)
+    skip_prologue_method = spm_none;
+  else
+    gdb_assert_not_reached ("Invalid skip_prologue_method kind value.");
+}
+
+/* Show skip_prologue_method.  */
+
+static void
+show_skip_prologue_method_command (struct ui_file *file, int from_tty,
+				   struct cmd_list_element *cmd,
+				   const char *value)
+{
+  gdb_printf (file, _("Skip prologue method is %s.\n"), value);
+}
+
 \f
 void _initialize_maint_cmds ();
 void
@@ -1475,5 +1524,15 @@ such as demangling symbol names."),
 					     &set_selftest_cmdlist,
 					     &show_selftest_cmdlist);
 
+  add_setshow_enum_cmd ("skip-prologue", class_maintenance,
+			skip_prologue_method_enum,
+			&skip_prologue_method_value,
+			_("Set skip-prologue method."),
+			_("Show skip-prologue method."),
+			_("Control the skip-prologue method."),
+			set_skip_prologue_method_command,
+			show_skip_prologue_method_command,
+			&maintenance_set_cmdlist, &maintenance_show_cmdlist);
+
   update_thread_pool_size ();
 }
diff --git a/gdb/maint.h b/gdb/maint.h
index 00b1e981ea6..0955bd9d76e 100644
--- a/gdb/maint.h
+++ b/gdb/maint.h
@@ -66,4 +66,13 @@ class scoped_command_stats
 extern obj_section *maint_obj_section_from_bfd_section (bfd *abfd,
 							asection *asection,
 							objfile *ofile);
+
+/* Values for maint set skip-prologue.  */
+
+enum spm_value { spm_default, spm_line_table, spm_none };
+
+/* Current value of maint set skip-prologue.  */
+
+extern spm_value skip_prologue_method;
+
 #endif /* MAINT_H */
diff --git a/gdb/testsuite/gdb.ada/access_tagged_param.exp b/gdb/testsuite/gdb.ada/access_tagged_param.exp
index 2b8e8ef172f..931c7fb12a9 100644
--- a/gdb/testsuite/gdb.ada/access_tagged_param.exp
+++ b/gdb/testsuite/gdb.ada/access_tagged_param.exp
@@ -33,7 +33,7 @@ if ![runto "foo"] then {
   return
 }
 
-gdb_breakpoint "pck.inspect"
+gdb_breakpoint "pck.adb:19"
 
 # Continue until we reach the breakpoint, and verify that we can print
 # the value of all the parameters.
diff --git a/gdb/testsuite/gdb.ada/inline-section-gc.exp b/gdb/testsuite/gdb.ada/inline-section-gc.exp
index 1f6ef667a87..20d181b1f44 100644
--- a/gdb/testsuite/gdb.ada/inline-section-gc.exp
+++ b/gdb/testsuite/gdb.ada/inline-section-gc.exp
@@ -34,6 +34,8 @@ if {[gdb_compile_ada "${srcfile}" "${binfile}" executable $options] != ""} {
 
 clean_restart ${testfile}
 
+gdb_test_no_output "maint set skip-prologue none"
+
 set bp_location [gdb_get_line_number "BREAK" ${testdir}/callee.adb]
 # The bug here was that gdb would set a breakpoint with two locations,
 # one of them at 0x0.
diff --git a/gdb/testsuite/gdb.ada/ptype_tagged_param.exp b/gdb/testsuite/gdb.ada/ptype_tagged_param.exp
index 0972e02a636..3a4a84a22ce 100644
--- a/gdb/testsuite/gdb.ada/ptype_tagged_param.exp
+++ b/gdb/testsuite/gdb.ada/ptype_tagged_param.exp
@@ -27,7 +27,7 @@ set has_runtime_debug_info [gnat_runtime_has_debug_info]
 
 clean_restart ${testfile}
 
-if ![runto "position_x" ] then {
+if ![runto "pck.adb:20" ] then {
   return -1
 }
 
diff --git a/gdb/testsuite/gdb.ada/ref_param.exp b/gdb/testsuite/gdb.ada/ref_param.exp
index cbe0c065d34..56347932564 100644
--- a/gdb/testsuite/gdb.ada/ref_param.exp
+++ b/gdb/testsuite/gdb.ada/ref_param.exp
@@ -25,7 +25,7 @@ if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" }
 
 clean_restart ${testfile}
 
-if ![runto call_me] then {
+if ![runto pck.adb:20] then {
   perror "Couldn't run ${testfile}"
   return
 } 
diff --git a/gdb/testsuite/gdb.opt/inline-small-func.exp b/gdb/testsuite/gdb.opt/inline-small-func.exp
index 3be1955d3b6..7af5f1a31a0 100644
--- a/gdb/testsuite/gdb.opt/inline-small-func.exp
+++ b/gdb/testsuite/gdb.opt/inline-small-func.exp
@@ -49,6 +49,8 @@ if ![runto_main] {
 # below will only contain a single breakpoint.
 delete_breakpoints
 
+gdb_test_no_output "maint set skip-prologue none"
+
 # Place a breakpoint within the function in the header file.
 set linenum [gdb_get_line_number "callee: body" $srcfile2]
 gdb_breakpoint "${srcfile2}:${linenum}"

Re: [RFC][gdb] Add maint set skip-prologue

[Tom de Vries]

On 8/10/22 15:48, Tom de Vries via Gdb-patches wrote:
> and fix a few other test-cases by
> specifying the breakpoint location in a way that's independent from the
> prologue.
> 
>   gdb/testsuite/gdb.ada/access_tagged_param.exp |  2 +-
>   gdb/testsuite/gdb.ada/ptype_tagged_param.exp  |  2 +-
>   gdb/testsuite/gdb.ada/ref_param.exp           |  2 +-

I've committed this separately at 
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=154f2735ad4c2eb0985086b113825e6f4f3abcf1

Thanks,
- Tom

[PING][RFC][gdb] Add maint set skip-prologue

[Tom de Vries]

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

On 9/7/22 11:33, Tom de Vries via Gdb-patches wrote:
> On 8/10/22 15:48, Tom de Vries via Gdb-patches wrote:
>> and fix a few other test-cases by
>> specifying the breakpoint location in a way that's independent from the
>> prologue.
>>
>>   gdb/testsuite/gdb.ada/access_tagged_param.exp |  2 +-
>>   gdb/testsuite/gdb.ada/ptype_tagged_param.exp  |  2 +-
>>   gdb/testsuite/gdb.ada/ref_param.exp           |  2 +-
> 
> I've committed this separately at 
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=154f2735ad4c2eb0985086b113825e6f4f3abcf1 

Ping, and resubmit with update patch.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-Add-maint-set-skip-prologue.patch --]
[-- Type: text/x-patch, Size: 7965 bytes --]

[gdb] Add maint set skip-prologue

With aarch64, I run into:
...
(gdb) break callee.adb:22^M
Breakpoint 1 at 0x401f78: file caller.adb, line 21.^M
(gdb) FAIL: gdb.ada/inline-section-gc.exp: break callee.adb:22
...
while this is expected (copied from x86_64):
...
(gdb) break callee.adb:22^M
Breakpoint 1 at 0x401f10: file callee.adb, line 22.^M
(gdb) PASS: gdb.ada/inline-section-gc.exp: break callee.adb:22
...

The difference comes from the gdbarch_skip_prologue implementation.

For x86_64, we have:
...
  if (find_pc_partial_function (start_pc, NULL, &func_addr, NULL))
    {
      CORE_ADDR post_prologue_pc
	= skip_prologue_using_sal (gdbarch, func_addr);
      struct compunit_symtab *cust = find_pc_compunit_symtab (func_addr);

      /* LLVM backend (Clang/Flang) always emits a line note before the
	 prologue and another one after.  We trust clang and newer Intel
	 compilers to emit usable line notes.  */
      if (post_prologue_pc
	  && (cust != NULL
	      && cust->producer () != nullptr
	      && (producer_is_llvm (cust->producer ())
	      || producer_is_icc_ge_19 (cust->producer ()))))
        return std::max (start_pc, post_prologue_pc);
    }
...
and because the producer is GCC, we don't trust the line table result, and use the
architecture-specific prologue analyzer, which skips no insns.

For aarch64, we have:
...
  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
    {
      CORE_ADDR post_prologue_pc
	= skip_prologue_using_sal (gdbarch, func_addr);

      if (post_prologue_pc != 0)
	return std::max (pc, post_prologue_pc);
    }
...
and consequently we trust the line table result and skip past a few insns,
which breaks test-case assumptions.

Add a command "maint set skip-prologue line-table", that forces usage of the
line-table result if available, which allows us to reproduce the same problem
on x86_64-linux.

Also add a command "maint set skip-prologue none", that forces skipping no
insn, which allows us to fix the FAIL on aarch64.

Likewise, fix one more test-case.

Tested on x86_64-linux and aarch64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29419
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29409

---
 gdb/arch-utils.c                            | 27 ++++++++++++-
 gdb/maint.c                                 | 59 +++++++++++++++++++++++++++++
 gdb/maint.h                                 |  9 +++++
 gdb/testsuite/gdb.ada/inline-section-gc.exp |  2 +
 gdb/testsuite/gdb.opt/inline-small-func.exp |  2 +
 5 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 9bd4f0ddae6..02520e49861 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -36,6 +36,7 @@
 #include "reggroups.h"
 #include "auxv.h"
 #include "observable.h"
+#include "maint.h"
 
 #include "gdbsupport/version.h"
 
@@ -1041,6 +1042,23 @@ default_print_insn (bfd_vma memaddr, disassemble_info *info)
   return (*disassemble_fn) (memaddr, info);
 }
 
+/* Helper function for gdbarch_skip_prologue_noexcept.  */
+
+static CORE_ADDR
+gdbarch_skip_prologue_line_table (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  CORE_ADDR func_addr;
+
+  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
+    {
+      CORE_ADDR post_prologue_pc
+	= skip_prologue_using_sal (gdbarch, func_addr);
+      return std::max (pc, post_prologue_pc);
+    }
+
+  return gdbarch_skip_prologue (gdbarch, pc);
+}
+
 /* See arch-utils.h.  */
 
 CORE_ADDR
@@ -1050,7 +1068,14 @@ gdbarch_skip_prologue_noexcept (gdbarch *gdbarch, CORE_ADDR pc) noexcept
 
   try
     {
-      new_pc = gdbarch_skip_prologue (gdbarch, pc);
+      if (skip_prologue_method == spm_default)
+	new_pc = gdbarch_skip_prologue (gdbarch, pc);
+      else if (skip_prologue_method == spm_line_table)
+	new_pc = gdbarch_skip_prologue_line_table (gdbarch, pc);
+      else if (skip_prologue_method == spm_none)
+	new_pc = pc;
+      else
+	gdb_assert_not_reached ("");
     }
   catch (const gdb_exception &ex)
     {}
diff --git a/gdb/maint.c b/gdb/maint.c
index 76ac7bece38..1a04d777bdc 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1204,6 +1204,55 @@ Selftests have been disabled for this build.\n"));
 #endif
 }
 
+/* See maint.h.  */
+
+spm_value skip_prologue_method = spm_default;
+
+/* Valid string arguments for maint set skip-prologue.  */
+
+static const char skip_prologue_method_default[] = "default";
+static const char skip_prologue_method_line_table[] = "line-table";
+static const char skip_prologue_method_line_none[] = "none";
+
+/* List of all valid string arguments for maint set skip-prologue.  */
+
+static const char *skip_prologue_method_enum[] = {
+  skip_prologue_method_default,
+  skip_prologue_method_line_table,
+  skip_prologue_method_line_none,
+  nullptr
+};
+
+/* Current string argument for maint set skip-prologue.  */
+
+static const char *skip_prologue_method_value = skip_prologue_method_default;
+
+/* Set skip_prologue_method.  */
+
+static void
+set_skip_prologue_method_command (const char *args, int from_tty,
+				  cmd_list_element *c)
+{
+  if (skip_prologue_method_value == skip_prologue_method_default)
+    skip_prologue_method = spm_default;
+  else if (skip_prologue_method_value == skip_prologue_method_line_table)
+    skip_prologue_method = spm_line_table;
+  else if (skip_prologue_method_value == skip_prologue_method_line_none)
+    skip_prologue_method = spm_none;
+  else
+    gdb_assert_not_reached ("Invalid skip_prologue_method kind value.");
+}
+
+/* Show skip_prologue_method.  */
+
+static void
+show_skip_prologue_method_command (struct ui_file *file, int from_tty,
+				   struct cmd_list_element *cmd,
+				   const char *value)
+{
+  gdb_printf (file, _("Skip prologue method is %s.\n"), value);
+}
+
 \f
 void _initialize_maint_cmds ();
 void
@@ -1475,5 +1524,15 @@ such as demangling symbol names."),
 					     &set_selftest_cmdlist,
 					     &show_selftest_cmdlist);
 
+  add_setshow_enum_cmd ("skip-prologue", class_maintenance,
+			skip_prologue_method_enum,
+			&skip_prologue_method_value,
+			_("Set skip-prologue method."),
+			_("Show skip-prologue method."),
+			_("Control the skip-prologue method."),
+			set_skip_prologue_method_command,
+			show_skip_prologue_method_command,
+			&maintenance_set_cmdlist, &maintenance_show_cmdlist);
+
   update_thread_pool_size ();
 }
diff --git a/gdb/maint.h b/gdb/maint.h
index 00b1e981ea6..0955bd9d76e 100644
--- a/gdb/maint.h
+++ b/gdb/maint.h
@@ -66,4 +66,13 @@ class scoped_command_stats
 extern obj_section *maint_obj_section_from_bfd_section (bfd *abfd,
 							asection *asection,
 							objfile *ofile);
+
+/* Values for maint set skip-prologue.  */
+
+enum spm_value { spm_default, spm_line_table, spm_none };
+
+/* Current value of maint set skip-prologue.  */
+
+extern spm_value skip_prologue_method;
+
 #endif /* MAINT_H */
diff --git a/gdb/testsuite/gdb.ada/inline-section-gc.exp b/gdb/testsuite/gdb.ada/inline-section-gc.exp
index 1f6ef667a87..20d181b1f44 100644
--- a/gdb/testsuite/gdb.ada/inline-section-gc.exp
+++ b/gdb/testsuite/gdb.ada/inline-section-gc.exp
@@ -34,6 +34,8 @@ if {[gdb_compile_ada "${srcfile}" "${binfile}" executable $options] != ""} {
 
 clean_restart ${testfile}
 
+gdb_test_no_output "maint set skip-prologue none"
+
 set bp_location [gdb_get_line_number "BREAK" ${testdir}/callee.adb]
 # The bug here was that gdb would set a breakpoint with two locations,
 # one of them at 0x0.
diff --git a/gdb/testsuite/gdb.opt/inline-small-func.exp b/gdb/testsuite/gdb.opt/inline-small-func.exp
index 3be1955d3b6..7af5f1a31a0 100644
--- a/gdb/testsuite/gdb.opt/inline-small-func.exp
+++ b/gdb/testsuite/gdb.opt/inline-small-func.exp
@@ -49,6 +49,8 @@ if ![runto_main] {
 # below will only contain a single breakpoint.
 delete_breakpoints
 
+gdb_test_no_output "maint set skip-prologue none"
+
 # Place a breakpoint within the function in the header file.
 set linenum [gdb_get_line_number "callee: body" $srcfile2]
 gdb_breakpoint "${srcfile2}:${linenum}"

Re: [PING][RFC][gdb] Add maint set skip-prologue

[Tom de Vries]

On 9/7/22 12:04, Tom de Vries wrote:
> On 9/7/22 11:33, Tom de Vries via Gdb-patches wrote:
>> On 8/10/22 15:48, Tom de Vries via Gdb-patches wrote:
>>> and fix a few other test-cases by
>>> specifying the breakpoint location in a way that's independent from the
>>> prologue.
>>>
>>>   gdb/testsuite/gdb.ada/access_tagged_param.exp |  2 +-
>>>   gdb/testsuite/gdb.ada/ptype_tagged_param.exp  |  2 +-
>>>   gdb/testsuite/gdb.ada/ref_param.exp           |  2 +-
>>
>> I've committed this separately at 
>> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=154f2735ad4c2eb0985086b113825e6f4f3abcf1 
> 
> 
> Ping, and resubmit with update patch.

And, FTR, this fixes the last two FAILs I see with openSUSE Leap 15.4 
aarch64.

Thanks,
- Tom