[review] Make 'target_{short,long}name' inline functions and check if 'current...

Message ID gerrit.1572586994000.I8a9a4dd6edc3784dc089f37d56f2d020567b4aee@gnutoolchain-gerrit.osci.io
State New
Headers show
Series
  • [review] Make 'target_{short,long}name' inline functions and check if 'current...
Related show

Commit Message

Simon Marchi (Code Review) Nov. 1, 2019, 5:43 a.m.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481
......................................................................

Make 'target_{short,long}name' inline functions and check if 'current_top_target ()' is not NULL

Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117

A segfault can happen in a specific scenario when using TUI + a
corefile, as explained in the bug mentioned above.  The problem
happens when opening a corefile on GDB:

  $ gdb ./core program

entering TUI (C-x a), and then issuing a "run" command.  GDB segfaults
with the following stack trace:

  (top-gdb) bt
  #0  0x00000000004cd5da in target_ops::shortname (this=0x0) at ../../binutils-gdb/gdb/target.h:449
  #1  0x0000000000ac08fb in target_shortname () at ../../binutils-gdb/gdb/target.h:1323
  #2  0x0000000000ac09ae in tui_locator_window::make_status_line[abi:cxx11]() const (this=0x23e1fa0 <_locator>) at ../../binutils-gdb/gdb/tui/tui-stack.c:86
  #3  0x0000000000ac1043 in tui_locator_window::rerender (this=0x23e1fa0 <_locator>) at ../../binutils-gdb/gdb/tui/tui-stack.c:231
  #4  0x0000000000ac1632 in tui_show_locator_content () at ../../binutils-gdb/gdb/tui/tui-stack.c:369
  #5  0x0000000000ac63b6 in tui_set_key_mode (mode=TUI_COMMAND_MODE) at ../../binutils-gdb/gdb/tui/tui.c:321
  #6  0x0000000000aaf9be in tui_inferior_exit (inf=0x2d446a0) at ../../binutils-gdb/gdb/tui/tui-hooks.c:181
  #7  0x000000000044cddf in std::_Function_handler<void (inferior*), void (*)(inferior*)>::_M_invoke(std::_Any_data const&, inferior*&&) (__functor=..., __args#0=@0x7fffffffd650: 0x2d446a0)
      at /usr/include/c++/9/bits/std_function.h:300
  #8  0x0000000000757db9 in std::function<void (inferior*)>::operator()(inferior*) const (this=0x2cf3168, __args#0=0x2d446a0) at /usr/include/c++/9/bits/std_function.h:690
  #9  0x0000000000757876 in gdb::observers::observable<inferior*>::notify (this=0x23de0c0 <gdb::observers::inferior_exit>, args#0=0x2d446a0)
      at ../../binutils-gdb/gdb/gdbsupport/observable.h:106
  #10 0x000000000075532d in exit_inferior_1 (inftoex=0x2d446a0, silent=1) at ../../binutils-gdb/gdb/inferior.c:191
  #11 0x0000000000755460 in exit_inferior_silent (inf=0x2d446a0) at ../../binutils-gdb/gdb/inferior.c:234
  #12 0x000000000059f47c in core_target::close (this=0x2d68590) at ../../binutils-gdb/gdb/corelow.c:265
  #13 0x0000000000a7688c in target_close (targ=0x2d68590) at ../../binutils-gdb/gdb/target.c:3293
  #14 0x0000000000a63d74 in target_stack::push (this=0x23e1800 <g_target_stack>, t=0x23c38c8 <the_amd64_linux_nat_target>) at ../../binutils-gdb/gdb/target.c:568
  #15 0x0000000000a63dbf in push_target (t=0x23c38c8 <the_amd64_linux_nat_target>) at ../../binutils-gdb/gdb/target.c:583
  #16 0x0000000000748088 in inf_ptrace_target::create_inferior (this=0x23c38c8 <the_amd64_linux_nat_target>, exec_file=0x2d58d30 "/usr/bin/cat", allargs="", env=0x25f12b0, from_tty=1)
      at ../../binutils-gdb/gdb/inf-ptrace.c:128
  #17 0x0000000000795ccb in linux_nat_target::create_inferior (this=0x23c38c8 <the_amd64_linux_nat_target>, exec_file=0x2d58d30 "/usr/bin/cat", allargs="", env=0x25f12b0, from_tty=1)
      at ../../binutils-gdb/gdb/linux-nat.c:1094
  #18 0x000000000074eae9 in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at ../../binutils-gdb/gdb/infcmd.c:639
  ...

The problem happens because 'tui_locator_window::make_status_line'
needs the value of 'target_shortname' in order to update the status
line.  'target_shortname' is a macro which expands to:

  #define	target_shortname	(current_top_target ()->shortname ())

and, in our scenario, 'current_top_target ()' returns NULL, which
obviously causes a segfault.  But why does it return NULL?

What is happening is that we're being caught in the middle of a
"target switch".  We had the 'core_target' on top, because we were
inspecting a corefile, but when the user decided to invoke "run" GDB
had to actually create the inferior, which ends up detecting that we
have a target already, and tries to close it (from target.c):

  /* See target.h.  */

  void
  target_stack::push (target_ops *t)
  {
    /* If there's already a target at this stratum, remove it.  */
    strata stratum = t->stratum ();

    if (m_stack[stratum] != NULL)
      {
	target_ops *prev = m_stack[stratum];
	m_stack[stratum] = NULL;
	target_close (prev); // <-- here
      }
  ...

When the current target ('core_target') is being closed, it checks for
possible observers registered with it and calls them.  TUI is one of
those observers, it gets called, tries to update the status line, and
GDB crashes.

I thought that there might be something deeper here, like perhaps a
way to avoid having an empty target stack, but it seemed to me that
the simpler fix is indeed the correct one: just check if
'current_top_target ()' returns NULL, and if it does, return an empty
string (I also considered returning something like "<none>", but I
don't know).

Since expanding the macros would be kind of ugly, I decided to turn
them into inline functions instead.  I think it's clearer this way.

This patch has been tested on the Buildbot and no regressions have
been found.  I'm also submitting a testcase for it.

gdb/ChangeLog:
2019-11-01  Sergio Durigan Junior  <sergiodj@redhat.com>

	* bfin-tdep.c (bfin_sw_breakpoint_from_kind):
	'target_shortname' and 'target_longname' is a function now.
	* infcmd.c (kill_command): Likewise.
	* infrun.c (set_schedlock_func): Likewise.
	* mi/mi-main.c (exec_reverse_continue): Likewise.
	* remote-sim.c (gdbsim_target::detach): Likewise.
	(gdbsim_target::files_info): Likewise.
	* reverse.c (exec_reverse_once): Likewise.
	* sh-tdep.c (sh_sw_breakpoint_from_kind): Likewise.
	* target.h (target_shortname): Make it a function.
	(target_longname): Likewise.
	* tui/tui-stack.c (tui_locator_window::make_status_line):
	'target_shortname' and 'target_longname' is a function now.

gdb/testsuite/ChangeLog:
2019-11-01  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.tui/segfault-corefile-run.exp: New file.

Change-Id: I8a9a4dd6edc3784dc089f37d56f2d020567b4aee
---
M gdb/bfin-tdep.c
M gdb/infcmd.c
M gdb/infrun.c
M gdb/mi/mi-main.c
M gdb/remote-sim.c
M gdb/reverse.c
M gdb/sh-tdep.c
M gdb/target.h
A gdb/testsuite/gdb.tui/segfault-corefile-run.exp
M gdb/tui/tui-stack.c
10 files changed, 86 insertions(+), 12 deletions(-)




-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8a9a4dd6edc3784dc089f37d56f2d020567b4aee
Gerrit-Change-Number: 481
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-MessageType: newchange

Comments

Simon Marchi (Code Review) Nov. 1, 2019, 8:40 a.m. | #1
Andrew Burgess has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481
......................................................................


Patch Set 1:

(1 comment)

I haven't had time to play with this code yet, but I had one question...

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481/1/gdb/target.h 
File gdb/target.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481/1/gdb/target.h@1312 
PS1, Line 1312: 
1307 |   target_ops *m_stack[(int) debug_stratum + 1] {};
1308 | };
1309 | 
1310 | /* The ops structure for our "current" target process.  This should
1311 |    never be NULL.  If there is no target, it points to the dummy_target.  */
1312 > 
1313 | extern target_ops *current_top_target ();
1314 | 
1315 | /* Define easy words for doing these operations on our current target.  */
1316 | 
1317 | static inline const char *

If this is the correct way to resolve this issue - I defer to others with more knowledge in this area - then you should probably change or expand this comment as its clearly not correct, right.  I mean this comment right here is the fundamental cause of your bug, it assumes current_top_target () can never return NULL, and you have found a case when it can.

That said, and I don't know the impact this would have - what if you "just" fixed current_top_target so that it really can never return NULL, but instead forces dummy_target to be returned?



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8a9a4dd6edc3784dc089f37d56f2d020567b4aee
Gerrit-Change-Number: 481
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-CC: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 08:40:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Simon Marchi (Code Review) Nov. 1, 2019, 1:18 p.m. | #2
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481
......................................................................


Patch Set 1:

(1 comment)

Probably Pedro should look at this... I wonder how it interacts with the
multi-remote series.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481/1/gdb/testsuite/gdb.tui/segfault-corefile-run.exp 
File gdb/testsuite/gdb.tui/segfault-corefile-run.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481/1/gdb/testsuite/gdb.tui/segfault-corefile-run.exp@60 
PS1, Line 60: 
55 | 
56 | Term::command "core-file $core"
57 | Term::check_contents "load corefile" "21 *return 0"
58 | 
59 | Term::command "run"
60 > Term::check_contents "run until the end" "\\\[Inferior $decimal \\\(process $decimal\\\) exited normally\\]"

This line looks over-long.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8a9a4dd6edc3784dc089f37d56f2d020567b4aee
Gerrit-Change-Number: 481
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-CC: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Fri, 01 Nov 2019 13:18:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Simon Marchi (Code Review) Nov. 1, 2019, 2:50 p.m. | #3
Sergio Durigan Junior has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481/1/gdb/target.h 
File gdb/target.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481/1/gdb/target.h@1312 
PS1, Line 1312: 
1307 |   target_ops *m_stack[(int) debug_stratum + 1] {};
1308 | };
1309 | 
1310 | /* The ops structure for our "current" target process.  This should
1311 |    never be NULL.  If there is no target, it points to the dummy_target.  */
1312 > 
1313 | extern target_ops *current_top_target ();
1314 | 
1315 | /* Define easy words for doing these operations on our current target.  */
1316 | 
1317 | static inline const char *

> If this is the correct way to resolve this issue - I defer to others with more knowledge in this are […]


You're correct, thanks for pointing this out.  Even though it seemed strange that current_top_target returns NULL, I missed the comment saying that it should never do that.  I guess this requires more investigation to see why this is happening.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8a9a4dd6edc3784dc089f37d56f2d020567b4aee
Gerrit-Change-Number: 481
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-CC: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Fri, 01 Nov 2019 14:50:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-MessageType: comment
Simon Marchi (Code Review) Nov. 1, 2019, 3:24 p.m. | #4
Sergio Durigan Junior has abandoned this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/481 )

Change subject: Make 'target_{short,long}name' inline functions and check if 'current_top_target ()' is not NULL
......................................................................


Abandoned

I will submit a different patch to fix the issue.
-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8a9a4dd6edc3784dc089f37d56f2d020567b4aee
Gerrit-Change-Number: 481
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-CC: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: abandon

Patch

diff --git a/gdb/bfin-tdep.c b/gdb/bfin-tdep.c
index 9d3e8eb..2579b7d 100644
--- a/gdb/bfin-tdep.c
+++ b/gdb/bfin-tdep.c
@@ -597,7 +597,7 @@ 
 
   *size = kind;
 
-  if (strcmp (target_shortname, "sim") == 0)
+  if (strcmp (target_shortname (), "sim") == 0)
     return bfin_sim_breakpoint;
   else
     return bfin_breakpoint;
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 5ca9933..17fb04f 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2505,7 +2505,7 @@ 
 	 so, print the state we are left in.  */
       if (target_has_stack)
 	{
-	  printf_filtered (_("In %s,\n"), target_longname);
+	  printf_filtered (_("In %s,\n"), target_longname ());
 	  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
 	}
     }
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d8a6eed..2bc4f64 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2074,7 +2074,8 @@ 
   if (!target_can_lock_scheduler)
     {
       scheduler_mode = schedlock_off;
-      error (_("Target '%s' cannot support this command."), target_shortname);
+      error (_("Target '%s' cannot support this command."),
+	     target_shortname ());
     }
 }
 
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 0e99fa3..eeb8cae 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -322,7 +322,7 @@ 
     error (_("Already in reverse mode."));
 
   if (!target_can_execute_reverse)
-    error (_("Target %s does not support this command."), target_shortname);
+    error (_("Target %s does not support this command."), target_shortname ());
 
   scoped_restore save_exec_dir = make_scoped_restore (&execution_direction,
 						      EXEC_REVERSE);
diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index 67b4690..9f8d965 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -834,7 +834,7 @@ 
 
   unpush_target (this);		/* calls gdbsim_close to do the real work */
   if (from_tty)
-    printf_filtered ("Ending simulator %s debugging\n", target_shortname);
+    printf_filtered ("Ending simulator %s debugging\n", target_shortname ());
 }
 
 /* Resume execution of the target process.  STEP says whether to single-step
@@ -1142,7 +1142,7 @@ 
   if (exec_bfd)
     {
       fprintf_unfiltered (gdb_stdlog, "\tAttached to %s running program %s\n",
-			  target_shortname, file);
+			  target_shortname (), file);
       sim_info (sim_data->gdbsim_desc, 0);
     }
 }
diff --git a/gdb/reverse.c b/gdb/reverse.c
index f2d2bf0..284ac66 100644
--- a/gdb/reverse.c
+++ b/gdb/reverse.c
@@ -45,7 +45,7 @@ 
 	   cmd);
 
   if (!target_can_execute_reverse)
-    error (_("Target %s does not support this command."), target_shortname);
+    error (_("Target %s does not support this command."), target_shortname ());
 
   std::string reverse_command = string_printf ("%s %s", cmd, args ? args : "");
   scoped_restore restore_exec_dir
diff --git a/gdb/sh-tdep.c b/gdb/sh-tdep.c
index 48a388b..cbd2972 100644
--- a/gdb/sh-tdep.c
+++ b/gdb/sh-tdep.c
@@ -432,7 +432,7 @@ 
   *size = kind;
 
   /* For remote stub targets, trapa #20 is used.  */
-  if (strcmp (target_shortname, "remote") == 0)
+  if (strcmp (target_shortname (), "remote") == 0)
     {
       static unsigned char big_remote_breakpoint[] = { 0xc3, 0x20 };
       static unsigned char little_remote_breakpoint[] = { 0x20, 0xc3 };
diff --git a/gdb/target.h b/gdb/target.h
index 1bb7276..5a85bdc 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1314,8 +1314,21 @@ 
 
 /* Define easy words for doing these operations on our current target.  */
 
-#define	target_shortname	(current_top_target ()->shortname ())
-#define	target_longname		(current_top_target ()->longname ())
+static inline const char *
+target_shortname ()
+{
+  target_ops *c = current_top_target ();
+
+  return c != nullptr ? c->shortname () : "";
+}
+
+static inline const char *
+target_longname ()
+{
+  target_ops *c = current_top_target ();
+
+  return c != nullptr ? c->longname () : "";
+}
 
 /* Does whatever cleanup is required for a target that we are no
    longer going to be calling.  This routine is automatically always
diff --git a/gdb/testsuite/gdb.tui/segfault-corefile-run.exp b/gdb/testsuite/gdb.tui/segfault-corefile-run.exp
new file mode 100644
index 0000000..532c982
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/segfault-corefile-run.exp
@@ -0,0 +1,60 @@ 
+# Copyright 2019 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 whether we can load a corefile, enable TUI and then invoke
+# "run" without having a segfault.
+#
+# Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117
+
+load_lib "tuiterm.exp"
+
+standard_testfile tui-layout.c
+
+set core "${testfile}.core"
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+    return -1
+}
+
+# Only run on native boards.
+if { [use_gdb_stub] || [target_info gdb_protocol] == "extended-remote" } {
+    untested "not supported"
+    return
+}
+
+if { ![runto_main] } {
+    untested "could not run to main"
+    return -1
+}
+
+if { ![gdb_gcore_cmd "$core" "save a corefile"] } {
+    untested "could not generate a corefile"
+    return -1
+}
+
+Term::clean_restart 24 80 $testfile
+if {![Term::enter_tui]} {
+    unsupported "TUI not supported"
+}
+
+set text [Term::get_all_lines]
+gdb_assert {![string match "No Source Available" $text]} \
+    "initial source listing"
+
+Term::command "core-file $core"
+Term::check_contents "load corefile" "21 *return 0"
+
+Term::command "run"
+Term::check_contents "run until the end" "\\\[Inferior $decimal \\\(process $decimal\\\) exited normally\\]"
diff --git a/gdb/tui/tui-stack.c b/gdb/tui/tui-stack.c
index 078f819..77be8d9 100644
--- a/gdb/tui/tui-stack.c
+++ b/gdb/tui/tui-stack.c
@@ -83,7 +83,7 @@ 
       pid_name = pid_name_holder.c_str ();
     }
 
-  target_width = strlen (target_shortname);
+  target_width = strlen (target_shortname ());
   if (target_width > MAX_TARGET_WIDTH)
     target_width = MAX_TARGET_WIDTH;
 
@@ -152,7 +152,7 @@ 
   string_file string;
 
   if (target_width > 0)
-    string.printf ("%*.*s ", -target_width, target_width, target_shortname);
+    string.printf ("%*.*s ", -target_width, target_width, target_shortname ());
   if (pid_width > 0)
     string.printf ("%*.*s ", -pid_width, pid_width, pid_name);