Re: [patch] Fix gdb.cp/gdb2495.exp regression with gcc-4.7

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Tue, 27 Dec 2011 05:56:06 +0100, Joel Brobecker wrote:
> How can we convince ourselves that we are not exchanging one wrong
> by another wrong which just happens to work better for the example
> at hand? (real question, not rethorical)

_start at least on GNU/Linux does not have .eh_frame FDE for itself.
One can suppress .eh_frame FDE for regular code with
`gcc -fno-asynchronous-unwind-tables'.
BTW .debug_frame is irrelevant for the system exceptions unwinder.

I have tested that if one supplies FDE with
	.cfi_undefined rip
for the _start function (from glibc) the new GDB behavior still works
correctly.

There could be also some future glibc fix to properly unwind back into ld.so
(ld.so jumps into _start).  I have not tested such case but (1) I doubt such
fix will happen because (1a) the _start function is common for -static and
dynamic build and (1b) ld.so already does not expect it could be unwound back
into it as it just drops its caller context in
glibc/sysdeps/x86_64/dl-machine.h:
	# And make sure %rsp points to argc stored on the stack.\n\
	movq %r13, %rsp\n\
	# Jump to the user's entry point.\n\
	jmp *%r12\n\
and (2) even if such unwind happens it should also work, GDB has no problem
with any unwound frames, as long as the unwinding is kept sane and the
unwinding does not crash by invalid data.

So after all I still find the _start function as the safe place for GDB.


>         /* If we cannot read INSN_LEN bytes (INSN_LEN might be larger
>            than the entry_point_address code, for instance), it is
>            probably fine to leave the return address at the entry
>            point.  */
> 
> Why would we think that, though? This might be related to may question
> at the start of this email...

I cannot much imaging a case when there are not 31 bytes of _start, that may
happen only with some embedded target (using entry point at its end of ROM).

My primary decision GDB should not error out in such case that it is just the
original GDB behavior which is mostly usable and therefore it is not
a regression.

Thanks for the text updates, I have included them below.


Still this patch is wrong as it currently conflicts with
displaced_step_at_entry_point which expected the original behavior.
We now need to possibly have in _start enough room not just for
gdb_arch_max_insn_length but for 3x gdb_arch_max_insn_length.
0x5d size is too large for _start and it may mess with other functions placed
after _start.

I will yet try to adjust it somehow.


Thanks,
Jan


FYI, not to be checked in yet:

2011-12-22  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix regression for gdb.cp/gdb2495.exp with gcc-4.7.
	* gdbarch.sh (max_insn_length): Set the default length to 31.  Extend
	comment by the unit.
	* gdbarch.c: Regenerate.
	* gdbarch.h: Regenerate.
	* infcall.c: Include disasm.h.
	(call_function_by_hand) <AT_ENTRY_POINT>: New variables insn and
	insn_len.  Adjust DUMMY_ADDR with them if possible.

--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -654,8 +654,9 @@ v:int:vbit_in_delta:::0:0::0
 # Advance PC to next instruction in order to skip a permanent breakpoint.
 F:void:skip_permanent_breakpoint:struct regcache *regcache:regcache
 
-# The maximum length of an instruction on this architecture.
-V:ULONGEST:max_insn_length:::0:0
+# The maximum length of an instruction on this architecture in bytes.
+# It should be at least as maximum of all the supported architectures.
+V:ULONGEST:max_insn_length:::31:31
 
 # Copy the instruction at FROM to TO, and make any adjustments
 # necessary to single-step it at that address.
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -38,6 +38,7 @@
 #include "ada-lang.h"
 #include "gdbthread.h"
 #include "exceptions.h"
+#include "disasm.h"
 
 /* If we can't find a function's name from its address,
    we print this instead.  */
@@ -627,26 +628,16 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
   switch (gdbarch_call_dummy_location (gdbarch))
     {
     case ON_STACK:
+      /* ON_STACK has problems on some targets featuring security policies
+	 disabling target stack executability.  */
       sp = push_dummy_code (gdbarch, sp, funaddr,
 				args, nargs, target_values_type,
 				&real_pc, &bp_addr, get_current_regcache ());
       break;
-    case AT_ENTRY_POINT:
-      {
-	CORE_ADDR dummy_addr;
-
-	real_pc = funaddr;
-	dummy_addr = entry_point_address ();
-	/* A call dummy always consists of just a single breakpoint, so
-	   its address is the same as the address of the dummy.  */
-	bp_addr = dummy_addr;
-	break;
-      }
     case AT_SYMBOL:
       /* Some executables define a symbol __CALL_DUMMY_ADDRESS whose
 	 address is the location where the breakpoint should be
-	 placed.  Once all targets are using the overhauled frame code
-	 this can be deleted - ON_STACK is a better option.  */
+	 placed.  */
       {
 	struct minimal_symbol *sym;
 	CORE_ADDR dummy_addr;
@@ -661,11 +652,50 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 	    dummy_addr = gdbarch_convert_from_func_ptr_addr (gdbarch,
 							     dummy_addr,
 							     &current_target);
+	    /* A call dummy always consists of just a single breakpoint,
+	       so it's address is the same as the address of the dummy.  */
+	    bp_addr = dummy_addr;
+	    break;
 	  }
+	break;
+      }
+      /* FALLTHROUGH */
+    case AT_ENTRY_POINT:
+      {
+	CORE_ADDR dummy_addr;
+	gdb_byte *insn;
+	CORE_ADDR insn_len;
+
+	real_pc = funaddr;
+	dummy_addr = entry_point_address ();
+
+	/* If the inferior call throws an uncaught C++ exception,
+	   the inferior unwinder tries to unwind all frames, including
+	   our dummy frame.  The unwinder determines the address of
+	   the calling instruction by subtracting 1 to the return
+	   address.  So, using the entry point's address as the return
+	   address would lead the unwinder to use the unwinding
+	   information of the code immediately preceding the entry
+	   point.  This information, if found, is invalid for the dummy
+	   frame, and can potentially crash the inferior's unwinder.
+	   Therefore, we try using the second instruction, right
+	   after the one at the entry point's address.  */
+
+	insn_len = gdbarch_max_insn_length (gdbarch);
+	insn = alloca (insn_len);
+	if (target_read_memory (dummy_addr, insn, insn_len) == 0)
+	  dummy_addr += gdb_buffered_insn_length (gdbarch, insn, insn_len,
+						  dummy_addr);
 	else
-	  dummy_addr = entry_point_address ();
-	/* A call dummy always consists of just a single breakpoint,
-	   so it's address is the same as the address of the dummy.  */
+	  {
+	    /* If we cannot read INSN_LEN bytes (INSN_LEN might be
+	       larger than the entry_point_address code, for
+	       instance), it is probably fine to leave the return
+	       address at the entry point.  */
+	  }
+
+	/* A call dummy always consists of just a single breakpoint, so
+	   its address is the same as the address of the dummy.  */
 	bp_addr = dummy_addr;
 	break;
       }