[binutils-gdb] [amd64] Invalid return address after displaced stepping

[Joel Brobecker <brobecke at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=4dafcdeb1341c4dd1a4641373bc17aab3ef2e788

commit 4dafcdeb1341c4dd1a4641373bc17aab3ef2e788
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Wed Aug 12 09:33:19 2015 -0700

    [amd64] Invalid return address after displaced stepping
    
    Making all-stop run on top of non-stop caused a small regression
    in behavior. This was observed on x86_64-linux. The attached testcase
    is in C whereas the investigation was done with an Ada program,
    but it's the same scenario, and using a C testcase allows wider testing.
    Basically: I am debugging a single-threaded program, and currently
    stopped inside a function provided by a shared-library, at a line
    calling a subprogram provided by a second shared library, and trying
    to "next" over that function call.
    
    Before we changed the default all-stop behavior, we had:
    
        7             Impl_Initialize;  -- Stop here and try "next" over this line
        (gdb) n
        8             return 5;  <<-- OK
    
    But now, "next" just stops much earlier:
    
        (gdb) n
        0x00007ffff7bd8560 in impl.initialize@plt () from /[...]/lib/libpck.so
    
    What happens is that next stops at a call instruction, which calls
    the function's PLT, and GDB fails to notice that the inferior stepped
    into a subroutine, and so decides that we're done. We can see another
    symptom of the same issue by looking at the backtrace at the point
    GDB stopped:
    
        (gdb) bt
        #0  0x00007ffff7bd8560 in impl.initialize@plt ()
           from /[...]/lib/libpck.so
        #1  0x00000000f7bd86f9 in ?? ()
        #2  0x00007fffffffdf50 in ?? ()
        #3  0x0000000000401893 in a () at /[...]/a.adb:7
        Backtrace stopped: frame did not save the PC
    
    With a functioning GDB, the backtrace looks like the following instead:
    
        #0  0x00007ffff7bd8560 in impl.initialize@plt ()
           from /[...]/lib/libpck.so
        #1  0x00007ffff7bd86f9 in sub () at /[...]/pck.adb:7
        #2  0x0000000000401893 in a () at /[...]/a.adb:7
    
    Note how, for frame #1, the address looks quite similar, except
    for the high-order bits not being set:
    
        #1  0x00007ffff7bd86f9 in sub () at /[...]/pck.adb:7   <<<--  OK
        #1  0x00000000f7bd86f9 in ?? ()                        <<<--  WRONG
                  ^^^^
                  ||||
                  Wrong
    
    Investigating this further led me to displaced stepping.
    As we are "next"-ing from a location where a breakpoint is inserted,
    we need to step out of it, and since we're on non-stop mode, we need
    to do it using displaced stepping. And looking at
    amd64-tdep.c:amd64_displaced_step_fixup, I found the code that handles
    the return address:
    
        regcache_cooked_read_unsigned (regs, AMD64_RSP_REGNUM, &rsp);
        retaddr = read_memory_unsigned_integer (rsp, retaddr_len, byte_order);
        retaddr = (retaddr - insn_offset) & 0xffffffffUL;
    
    The mask used to compute retaddr looks wrong to me, keeping only
    4 bytes instead of 8, and explains why the high order bits of
    the backtrace are unset. What happens is that, after the displaced
    stepping has completed, GDB restores that return address at the location
    where the program expects it.  But because the top half bits of
    the address have been masked out, the return address is now invalid.
    The incorrect behavior of the "next" command and the backtrace at
    that location are the first symptoms of that.  Another symptom is
    that this actually alters the behavior of the program, where a "cont"
    from there soon leads to a SEGV when the inferior tries to jump back
    to that incorrect return address:
    
        (gdb) c
        Continuing.
    
        Program received signal SIGSEGV, Segmentation fault.
        0x00000000f7bd86f9 in ?? ()
        ^^^^^^^^^^^^^^^^^^
    
    This patch fixes the issue by using a mask that seems more appropriate
    for this architecture.
    
    gdb/ChangeLog:
    
            * amd64-tdep.c (amd64_displaced_step_fixup): Fix the mask used to
            compute RETADDR.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.base/dso2dso-dso2.c, gdb.base/dso2dso-dso2.h,
            gdb.base/dso2dso-dso1.c, gdb.base/dso2dso-dso1.h, gdb.base/dso2dso.c,
            gdb.base/dso2dso.exp: New files.
    
    Tested on x86_64-linux, no regression.

Diff:
---
 gdb/ChangeLog                         |  5 +++
 gdb/amd64-tdep.c                      |  2 +-
 gdb/testsuite/ChangeLog               |  6 ++++
 gdb/testsuite/gdb.base/dso2dso-dso1.c | 26 ++++++++++++++
 gdb/testsuite/gdb.base/dso2dso-dso1.h | 23 ++++++++++++
 gdb/testsuite/gdb.base/dso2dso-dso2.c | 24 +++++++++++++
 gdb/testsuite/gdb.base/dso2dso-dso2.h | 23 ++++++++++++
 gdb/testsuite/gdb.base/dso2dso.c      | 25 +++++++++++++
 gdb/testsuite/gdb.base/dso2dso.exp    | 68 +++++++++++++++++++++++++++++++++++
 9 files changed, 201 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 337b9bf..367572a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2015-08-12  Joel Brobecker  <brobecker@adacore.com>
+
+	* amd64-tdep.c (amd64_displaced_step_fixup): Fix the mask used to
+	compute RETADDR.
+
 2015-08-12  Keith Seitz  <keiths@redhat.com>
 
 	* python/python.c (gdbpy_decode_line): Initialize `location' to NULL
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 5e63b5e..a672cde 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -1662,7 +1662,7 @@ amd64_displaced_step_fixup (struct gdbarch *gdbarch,
 
       regcache_cooked_read_unsigned (regs, AMD64_RSP_REGNUM, &rsp);
       retaddr = read_memory_unsigned_integer (rsp, retaddr_len, byte_order);
-      retaddr = (retaddr - insn_offset) & 0xffffffffUL;
+      retaddr = (retaddr - insn_offset) & 0xffffffffffffffffULL;
       write_memory_unsigned_integer (rsp, retaddr_len, byte_order, retaddr);
 
       if (debug_displaced)
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 916da1f..738c927 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2015-08-12  Joel Brobecker  <brobecker@adacore.com>
+
+	* gdb.base/dso2dso-dso2.c, gdb.base/dso2dso-dso2.h,
+	gdb.base/dso2dso-dso1.c, gdb.base/dso2dso-dso1.h, gdb.base/dso2dso.c,
+	gdb.base/dso2dso.exp: New files.
+
 2015-08-12  Sergio Durigan Junior  <sergiodj@redhat.com>
 	    Pedro Alves  <palves@redhat.com>
 	    Keith Seitz  <keiths@redhat.com>
diff --git a/gdb/testsuite/gdb.base/dso2dso-dso1.c b/gdb/testsuite/gdb.base/dso2dso-dso1.c
new file mode 100644
index 0000000..a360e20
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dso2dso-dso1.c
@@ -0,0 +1,26 @@
+/* Copyright 2015 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#include "dso2dso-dso1.h"
+#include "dso2dso-dso2.h"
+
+int
+sub1 (void)
+{
+  sub2 ();  /* STOP HERE.  */
+  return 5;
+}
diff --git a/gdb/testsuite/gdb.base/dso2dso-dso1.h b/gdb/testsuite/gdb.base/dso2dso-dso1.h
new file mode 100644
index 0000000..2423360
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dso2dso-dso1.h
@@ -0,0 +1,23 @@
+/* Copyright 2015 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef DSO2DSO_DSO1_H
+#define DSO2DSO_DSO1_H
+
+extern int sub1 (void);
+
+#endif
diff --git a/gdb/testsuite/gdb.base/dso2dso-dso2.c b/gdb/testsuite/gdb.base/dso2dso-dso2.c
new file mode 100644
index 0000000..14de6a8
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dso2dso-dso2.c
@@ -0,0 +1,24 @@
+/* Copyright 2015 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#include "dso2dso-dso2.h"
+
+void
+sub2 (void)
+{
+  /* Do nothing.  */
+}
diff --git a/gdb/testsuite/gdb.base/dso2dso-dso2.h b/gdb/testsuite/gdb.base/dso2dso-dso2.h
new file mode 100644
index 0000000..e33ca0a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dso2dso-dso2.h
@@ -0,0 +1,23 @@
+/* Copyright 2015 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef DSO2DSO_DSO2_H
+#define DSO2DSO_DSO2_H
+
+extern void sub2 (void);
+
+#endif
diff --git a/gdb/testsuite/gdb.base/dso2dso.c b/gdb/testsuite/gdb.base/dso2dso.c
new file mode 100644
index 0000000..563bd96
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dso2dso.c
@@ -0,0 +1,25 @@
+/* Copyright 2015 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#include "dso2dso-dso1.h"
+
+int
+main (void)
+{
+  int ignored = sub1 ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/dso2dso.exp b/gdb/testsuite/gdb.base/dso2dso.exp
new file mode 100644
index 0000000..b604012
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dso2dso.exp
@@ -0,0 +1,68 @@
+# Copyright 2015 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/>.
+
+if { [skip_shlib_tests] } {
+    return 0
+}
+
+standard_testfile
+
+set output_dir [standard_output_file {}]
+
+set libdso2 $testfile-dso2
+set srcfile_libdso2 $srcdir/$subdir/$libdso2.c
+set binfile_libdso2 [standard_output_file $libdso2.so]
+
+set libdso1 $testfile-dso1
+set srcfile_libdso1 $srcdir/$subdir/$libdso1.c
+set binfile_libdso1 [standard_output_file $libdso1.so]
+
+if { [gdb_compile_shlib $srcfile_libdso2 $binfile_libdso2 \
+	[list debug additional_flags=-fPIC]] != "" } {
+  untested "Could not compile $binfile_libdso2."
+  return -1
+}
+
+if { [gdb_compile_shlib $srcfile_libdso1 $binfile_libdso1 \
+	[list debug additional_flags=-fPIC]] != "" } {
+  untested "Could not compile $binfile_libdso1."
+  return -1
+}
+
+if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable \
+	[list debug shlib=$binfile_libdso2 shlib=$binfile_libdso1]] != "" } {
+  return -1
+}
+
+clean_restart $binfile
+gdb_load_shlibs $binfile_libdso2 $binfile_libdso1
+
+if { ![runto_main] } {
+  return -1
+}
+
+# Verify that we can "next" over the call to sub2 (provided by
+# libdso2) make from sub1 (provided by libdso1), and land at
+# the expected location.
+
+set bp_location [gdb_get_line_number "STOP HERE" $srcfile_libdso1]
+gdb_breakpoint ${srcfile_libdso1}:${bp_location}
+
+gdb_continue_to_breakpoint "at call to sub2" \
+    ".*sub2 ().*"
+
+gdb_test "next" \
+    ".*return 5;.*" \
+    "next over call to sub2"