diff mbox

[RFC] gdb: Give a better error when a breakpoint can't be placed in a PIE

Message ID 20200202011022.11357-1-andrew.burgess@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Feb. 2, 2020, 1:10 a.m. UTC
I'd be interested in feedback on the patch below.  Do people think
this is a good/useful idea?

Would there be a better error message to give to the user?  Or a
better time/place to present the error message?

All feedback welcome.

Thanks,
Andrew

---

One issue that seems to crop up fairly regularly on help sites like
stackoverflow, is a use complaining why something like this doesn't
work:

  $ gdb app.exe
  (gdb) disassemble main
  Dump of assembler code for function main:
     0x00000000000005e5 <+0>:	push   %rbp
     0x00000000000005e6 <+1>:	mov    %rsp,%rbp
     0x00000000000005e9 <+4>:	mov    $0x0,%eax
     0x00000000000005ee <+9>:	pop    %rbp
     0x00000000000005ef <+10>:	retq
  (gdb) break *0x5e5
  Breakpoint 1 at 0x5e5: file app.c, line 20.
  (gdb) run
  Starting program: app.exe
  Warning:
  Cannot insert breakpoint 1.
  Cannot access memory at address 0x5e5

Everything appears to be going fine until the user tries to run GDB,
which can, understandably be a little confusing.

The problem is that the executable was compiled as PIE, and the
dynamic linker has placed main at some address that doesn't match the
addresses (or offsets as they really are now) in the applications
ELF.

The problem is that if the user isn't really that aware of PIC/PIE, or
how it works, then they might not realise they what the problem is.

This commit tries to detect when the user has made this mistake, and
gives a warning message that (hopefully) is more helpful, the user now
sees this:

  (gdb) run
  Starting program: app.exe
  Warning:
  Cannot insert breakpoint 1.
  Cannot access memory at address 0x5e5
  The address is located within a code region of `app.exe' which was dynamically
  relocated at run time.  The code region `0x0000000000000500 ... 0x0000000000000662' is now located
  at `0x0000555555554500 ... 0x0000555555554662', you may need to adjust your breakpoints.

gdb/ChangeLog:

	* breakpoint.c (pic_code_error_check): New function.
	(insert_bp_location): Call new function.

gdb/testsuite/ChangeLog:

	* gdb.base/pie-bp-addr-error.c: New file.
	* gdb.base/pie-bp-addr-error.exp: New file.

Change-Id: Iedea261a54ab0a6ed03ec9b8df06fdbe459f28be
---
 gdb/ChangeLog                                |  5 +++
 gdb/breakpoint.c                             | 65 ++++++++++++++++++++++++++++
 gdb/testsuite/ChangeLog                      |  5 +++
 gdb/testsuite/gdb.base/pie-bp-addr-error.c   | 22 ++++++++++
 gdb/testsuite/gdb.base/pie-bp-addr-error.exp | 61 ++++++++++++++++++++++++++
 5 files changed, 158 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/pie-bp-addr-error.c
 create mode 100644 gdb/testsuite/gdb.base/pie-bp-addr-error.exp
diff mbox

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5a9352c26fe..1d7926dcefb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2405,6 +2405,69 @@  breakpoint_kind (struct bp_location *bl, CORE_ADDR *addr)
     return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr);
 }
 
+/* This should be called when we fail to insert a breakpoint, with ADDRESS
+   set to the location at which we tried to insert the breakpoint.
+
+   This function checks to see if it is possible the user tried to place
+   the breakpoint within an executable section of an object file that has
+   been relocated at run time.  If this is the case then an extra warning
+   is printed to TMP_ERROR_STREAM.  */
+
+static void
+pic_code_error_check (struct ui_file *tmp_error_stream,
+		      CORE_ADDR address)
+{
+  for (objfile *objfile : current_program_space->objfiles ())
+    {
+      struct obj_section *osect;
+
+      /* If the primary text section is not relocated then nothing should
+	 be relocated - maybe?  */
+      if (objfile->text_section_offset () == 0)
+	continue;
+
+      /* We know this object file was relocated, but was the breakpoint
+	 inside one of the code sections before relocation?  */
+      ALL_OBJFILE_OSECTIONS (objfile, osect)
+	{
+	  /* Ignore non-code sections.  */
+	  if ((bfd_section_flags (osect->the_bfd_section) & SEC_CODE) == 0)
+	    continue;
+
+	  /* Was ADDRESS within this section before relocation?  */
+	  if (!(address >= bfd_section_vma (osect->the_bfd_section)
+		&& address < (bfd_section_vma (osect->the_bfd_section)
+			      + bfd_section_size (osect->the_bfd_section))))
+	    continue;
+
+	  /* Final check that this section was relocated.  */
+	  int idx = osect - objfile->sections;
+	  CORE_ADDR offset = objfile->section_offsets[idx];
+	  if (offset == 0)
+	    continue;
+
+	  /* Let the user know that they may have made a mistake.  One
+	     final check that it might be nice to do is to only give this
+	     warning if the user placed the breakpoint by address, that is
+	     using 'break *0x.......', if they did anything else and we
+	     still have this issue then I'm not sure what the user could
+	     reasonably do about it.  However, for now, we just always
+	     give this error.  */
+	  CORE_ADDR len = bfd_section_size (osect->the_bfd_section);
+	  CORE_ADDR start = bfd_section_vma (osect->the_bfd_section);
+	  fprintf_unfiltered (tmp_error_stream, "\
+The address is located within a code region of `%s' which was dynamically\n\
+relocated at run time.  The code region `%s ... %s' is now located\n\
+at `%s ... %s', you may need to adjust your breakpoints.\n",
+			      objfile_name (objfile),
+			      core_addr_to_string (start),
+			      core_addr_to_string (start + len),
+			      core_addr_to_string (offset + start),
+			      core_addr_to_string (offset + start + len));
+	}
+    }
+}
+
 /* Insert a low-level "breakpoint" of some type.  BL is the breakpoint
    location.  Any error messages are printed to TMP_ERROR_STREAM; and
    DISABLED_BREAKS, and HW_BREAKPOINT_ERROR are used to report problems.
@@ -2670,6 +2733,8 @@  insert_bp_location (struct bp_location *bl,
 					  "Cannot insert breakpoint %d.\n"
 					  "%s\n",
 					  bl->owner->number, message.c_str ());
+
+		      pic_code_error_check (tmp_error_stream, bl->address);
 		    }
 		  else
 		    {
diff --git a/gdb/testsuite/gdb.base/pie-bp-addr-error.c b/gdb/testsuite/gdb.base/pie-bp-addr-error.c
new file mode 100644
index 00000000000..f4825c8a7c1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pie-bp-addr-error.c
@@ -0,0 +1,22 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/pie-bp-addr-error.exp b/gdb/testsuite/gdb.base/pie-bp-addr-error.exp
new file mode 100644
index 00000000000..a54320c0d7f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pie-bp-addr-error.exp
@@ -0,0 +1,61 @@ 
+# 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/>.
+
+# Check that, if the user places a breakpoint on an address within a
+# PIE before relocation, then we get a suitable error.
+
+global inferior_spawn_id
+global gdb_spawn_id
+
+if ![istarget *-linux*] {
+    continue
+}
+
+standard_testfile .c
+
+set opts [list debug additional_flags=-fPIE ldflags=-pie]
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile $opts] } {
+    return -1
+}
+
+# Figure out the address of main before sectionns are relocated.
+set main_addr_before_reloc \
+    [get_hexadecimal_valueof "&main" 0 "address of main"]
+
+# Start the inferior and run to main, this will relocate the sections.
+if ![runto_main] {
+    return -1
+}
+
+# First confirm that main has moved.
+set main_relocated_address \
+    [get_hexadecimal_valueof "&main" 0 "runtime address of main"]
+gdb_assert { ${main_addr_before_reloc} != ${main_relocated_address} } \
+    "address of main has changed"
+
+# Now place a breakpoint using the before relocation address.
+gdb_breakpoint "*${main_addr_before_reloc}"
+
+# Continue the inferior, inserting all breakpoints - which should
+# error due to our use of an invalid address.
+gdb_test "continue" \
+    [multi_line \
+	 "Cannot insert breakpoint $decimal." \
+	 "Cannot access memory at address $hex" \
+	 "The address is located within a code region of `\[^\r\n\]+' which was dynamically" \
+	 "relocated at run time.  The code region `$hex ... $hex' is now located" \
+	 "at `$hex ... $hex', you may need to adjust your breakpoints." \
+	 "" \
+	 "Command aborted." ]