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

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

[Jan Kratochvil]

Hi,

(gdb) p exceptions.throw_function()
Program received signal SIGSEGV, Segmentation fault.
x86_64_fallback_frame_state (fs=0x7fffffffdc80, context=0x7fffffffdef0) at ../../../gcc/config/i386/linux-unwind.h:47
47       if (*(unsigned char *)(pc+0) == 0x48
The program being debugged was signaled while in a function called from GDB.
[...]
(gdb) FAIL: gdb.cp/gdb2495.exp: Call a function that raises an exception without a handler.

This happens with
	gcc (GCC) 4.7.0 20111222 (experimental)
on Fedora Rawhide (pre-17) x86_64 as the function before _start is PLT and
PLTs have no proper .eh_frame entries.  Still such .eh_frame PLT entry sure
does not apply for the <function called from gdb> frame.  Therefore it unwinds
into garbage and then crashes on it (PC is 0x1 abo-ve).

No regressions on {x86_64,x86_64-m32}-fedora16-linux-gnu.

Thanks,
Jan


gdb/
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.
	* 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
@@ -655,7 +655,8 @@ v:int:vbit_in_delta:::0:0::0
 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
+# 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.  */
@@ -634,9 +635,32 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
     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 will try to unwind all the frames incl. the dummy frame.
+	   Using the entry_point_address directly will try to find FDE at the
+	   function right before the entry_point_address address as the
+	   unwinder subtracts 1 to get at the call instruction.  FDE of the
+	   preceding function, if found, would be invalid for the dummy frame
+	   and it would crash the inferior's unwinder.  Therefore attempt to
+	   skip the very first instruction of entry_point_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
+	  {
+	    /* No problem probably occurs without this adjustment.  INSN_LEN
+	       may be for example larger than the entry_point_address code.  */
+	  }
+
 	/* 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;

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

[Joel Brobecker]

> 	gcc (GCC) 4.7.0 20111222 (experimental)
> on Fedora Rawhide (pre-17) x86_64 as the function before _start is PLT and
> PLTs have no proper .eh_frame entries.  Still such .eh_frame PLT entry sure
> does not apply for the <function called from gdb> frame.  Therefore it
> unwinds into garbage and then crashes on it (PC is 0x1 abo-ve).

At first sight, it makes sense to choose the next instruction as
the return address, the same way call instructions do (which is
why unwinders have to subtract 1 from the return address to get
the actual calling instruction).

The situation is a little different in this case, as the function
at the entry point is not actually really the one making the call.
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)

> +# It should be at least as maximum of all the supported architectures.
> +V:ULONGEST:max_insn_length:::31:31

Let's take this opportunity to document the unit used for this size
(bytes, right?).

> +	/* If the inferior call throws an uncaught C++ exception the inferior
> +	   unwinder will try to unwind all the frames incl. the dummy frame.
> +	   Using the entry_point_address directly will try to find FDE at the
> +	   function right before the entry_point_address address as the
> +	   unwinder subtracts 1 to get at the call instruction.  FDE of the
> +	   preceding function, if found, would be invalid for the dummy frame
> +	   and it would crash the inferior's unwinder.  Therefore attempt to
> +	   skip the very first instruction of entry_point_address.  */

Minor edit (punctuation and maybe being more explicit on who does what,
and also reformatting to keep it close to 70 chars max - as decided
during one of our discussions):

	/* 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.  */

> +	  {
> +	    /* No problem probably occurs without this adjustment.  INSN_LEN
> +	       may be for example larger than the entry_point_address code.  */
> +	  }

        /* 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...

-- 
Joel

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

[Jan Kratochvil]

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;
       }

[patch] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #2

[Jan Kratochvil]

On Wed, 28 Dec 2011 17:12:08 +0100, Jan Kratochvil wrote:
> I will yet try to adjust it somehow.

This one may work.

displaced_step_at_entry_point has been already assuming after

  gdbarch_breakpoint_from_pc (gdbarch, &addr, &bp_len);
  addr += bp_len * 2;

ADDR can be used as an executable instruction address.  Therefore the
gdb_buffered_insn_length adjustment was possibly needlessly strong
(and needlessly expensive).

Expecting _start is at least as long as 2 * BP_LEN looks definitely valid and
on archs supporting displaced stepping the assumption of _start length 3 *
BP_LEN + gdbarch_max_insn_length looks also safe (also these archs have
properly set gdbarch_max_insn_length which is therefore small enough).

I am not convinced it works on each arch but it works at least on something so
special like ia64 so it really may work everywhere.  Displaced stepping is not
implemented on each arch so the displaced_step_at_entry_point does not prove it
works on every arch.  While this proposed code applies to (almost -
AT_ENTRY_POINT) any arch, so there is still a regression risk.

No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu.


Thanks,
Jan


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

	Fix regression for gdb.cp/gdb2495.exp with gcc-4.7.
	* arch-utils.c (displaced_step_at_entry_point): Incrase BP_LEN skip to
	3 times.
	* infcall.c (call_function_by_hand) <ON_STACK>: New comment on stack
	executability.
	(call_function_by_hand) <AT_SYMBOL>: Drop comment on ON_STACK
	preference.  Move it upwards and fall through into AT_ENTRY_POINT.
	(call_function_by_hand) <AT_ENTRY_POINT>: New variable bp_len.  Adjust
	DUMMY_ADDR with it.
	* ppc-linux-tdep.c (ppc_linux_displaced_step_location): Increase
	PPC_INSN_SIZE skip to 3 times.

--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -86,7 +86,7 @@ displaced_step_at_entry_point (struct gdbarch *gdbarch)
      We don't want displaced stepping to interfere with those
      breakpoints, so leave space.  */
   gdbarch_breakpoint_from_pc (gdbarch, &addr, &bp_len);
-  addr += bp_len * 2;
+  addr += bp_len * 3;
 
   return addr;
 }
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -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,41 @@ 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;
 	  }
-	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.  */
+	break;
+      }
+      /* FALLTHROUGH */
+    case AT_ENTRY_POINT:
+      {
+	CORE_ADDR dummy_addr;
+	int bp_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 use the second byte (approximately,
+	   alignments depending on GDBARCH).  It does not matter if it
+	   is placed inside the very first instruction, nothing tries
+	   to execute it.  */
+
+	gdbarch_breakpoint_from_pc (gdbarch, &dummy_addr, &bp_len);
+	dummy_addr += bp_len;
+
+	/* 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;
       }
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1075,7 +1075,7 @@ ppc_linux_displaced_step_location (struct gdbarch *gdbarch)
       /* Inferior calls also use the entry point as a breakpoint location.
 	 We don't want displaced stepping to interfere with those
 	 breakpoints, so leave space.  */
-      ppc_linux_entry_point_addr = addr + 2 * PPC_INSN_SIZE;
+      ppc_linux_entry_point_addr = addr + 3 * PPC_INSN_SIZE;
     }
 
   return ppc_linux_entry_point_addr;

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

[Mark Kettenis]

> Date: Wed, 28 Dec 2011 19:01:48 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -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.  */

Hmm, did you actually try using ON_STACK?  We specifically treat
SIGSEGV as a potential breakpoint to deal with non-executable stacks.
And the diff below works just fine on OpenBSD/amd64 where stacks are
non-executable.


Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.340
diff -u -p -r1.340 i386-tdep.c
--- i386-tdep.c	14 Nov 2011 20:07:20 -0000	1.340
+++ i386-tdep.c	28 Dec 2011 20:06:23 -0000
@@ -2321,6 +2321,19 @@ i386_16_byte_align_p (struct type *type)
 }
 
 static CORE_ADDR
+i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
+		      CORE_ADDR funcaddr,
+		      struct value **args, int nargs,
+		      struct type *value_type,
+		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
+		      struct regcache *regcache)
+{
+  *bp_addr = sp - 1;
+  *real_pc = funcaddr;
+  return sp - 1;
+}
+
+static CORE_ADDR
 i386_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		      struct regcache *regcache, CORE_ADDR bp_addr, int nargs,
 		      struct value **args, CORE_ADDR sp, int struct_return,
@@ -7366,6 +7379,8 @@ i386_gdbarch_init (struct gdbarch_info i
   set_gdbarch_get_longjmp_target (gdbarch, i386_get_longjmp_target);
 
   /* Call dummy code.  */
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code(gdbarch, i386_push_dummy_code);
   set_gdbarch_push_dummy_call (gdbarch, i386_push_dummy_call);
   set_gdbarch_frame_align (gdbarch, i386_frame_align);
 

[patch] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #3

[Jan Kratochvil]

On Wed, 28 Dec 2011 21:09:21 +0100, Mark Kettenis wrote:
> Hmm, did you actually try using ON_STACK?

No.

> We specifically treat
> SIGSEGV as a potential breakpoint to deal with non-executable stacks.

I can confirm at least on RHEL-6.2 x86_64 a breakpoint on non-executable stack
gets properly recognized by GDB.

I have dropped those comments change.  I understand it was incorrect from me
to include it into that mostly unrelated patch.

As ON_STACK is a valid option sure one may prefer to convert all the archs to
ON_STACK instead of fixing AT_ENTRY_POINT; not preferred by me TBH.


Thanks,
Jan


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

	Fix regression for gdb.cp/gdb2495.exp with gcc-4.7.
	* arch-utils.c (displaced_step_at_entry_point): Incrase BP_LEN skip to
	3 times.
	* infcall.c (call_function_by_hand) <AT_SYMBOL>: Move it upwards and
	fall through into AT_ENTRY_POINT.
	(call_function_by_hand) <AT_ENTRY_POINT>: New variable bp_len.  Adjust
	DUMMY_ADDR with it.
	* ppc-linux-tdep.c (ppc_linux_displaced_step_location): Increase
	PPC_INSN_SIZE skip to 3 times.

--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -86,7 +86,7 @@ displaced_step_at_entry_point (struct gdbarch *gdbarch)
      We don't want displaced stepping to interfere with those
      breakpoints, so leave space.  */
   gdbarch_breakpoint_from_pc (gdbarch, &addr, &bp_len);
-  addr += bp_len * 2;
+  addr += bp_len * 3;
 
   return addr;
 }
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -627,17 +628,6 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 				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
@@ -661,11 +652,41 @@ 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;
 	  }
-	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.  */
+	break;
+      }
+      /* FALLTHROUGH */
+    case AT_ENTRY_POINT:
+      {
+	CORE_ADDR dummy_addr;
+	int bp_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 use the second byte (approximately,
+	   alignments depending on GDBARCH).  It does not matter if it
+	   is placed inside the very first instruction, nothing tries
+	   to execute it.  */
+
+	gdbarch_breakpoint_from_pc (gdbarch, &dummy_addr, &bp_len);
+	dummy_addr += bp_len;
+
+	/* 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;
       }
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1075,7 +1075,7 @@ ppc_linux_displaced_step_location (struct gdbarch *gdbarch)
       /* Inferior calls also use the entry point as a breakpoint location.
 	 We don't want displaced stepping to interfere with those
 	 breakpoints, so leave space.  */
-      ppc_linux_entry_point_addr = addr + 2 * PPC_INSN_SIZE;
+      ppc_linux_entry_point_addr = addr + 3 * PPC_INSN_SIZE;
     }
 
   return ppc_linux_entry_point_addr;

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

[Joel Brobecker]

> As ON_STACK is a valid option sure one may prefer to convert all the
> archs to ON_STACK instead of fixing AT_ENTRY_POINT; not preferred by
> me TBH.

I don't understand why, though. ON_STACK seems to be perfect, as
we control exactly what's there, and we know we're not going to
interfere with the rest of the code.

Are there any situation where ON_STACK wouldn't be possible? I know
that on some architectures such as VxWorks, where objfiles are
already loaded in memory, and where memory is shared by all
processes[1], there is no concept of "entry point".

I have put in my TODO list to see if I can get rid of the last
use of AT_SYMBOL (in mips-tdep.c) and convert it to ON_STACK.

> 2011-12-30  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix regression for gdb.cp/gdb2495.exp with gcc-4.7.
> 	* arch-utils.c (displaced_step_at_entry_point): Incrase BP_LEN skip to
> 	3 times.
> 	* infcall.c (call_function_by_hand) <AT_SYMBOL>: Move it upwards and
> 	fall through into AT_ENTRY_POINT.
> 	(call_function_by_hand) <AT_ENTRY_POINT>: New variable bp_len.  Adjust
> 	DUMMY_ADDR with it.
> 	* ppc-linux-tdep.c (ppc_linux_displaced_step_location): Increase
> 	PPC_INSN_SIZE skip to 3 times.

I keep staring at the diff, and I can't figure out how the AT_SYMBOL
case is falling through, or why it would be necessary. The lack of
understading why is probably related to the fact that I may still
have an incorrect understanding of the situation.

I think that either way, it's better to have the dummy calling
address at a location where there is no unwinding information.
ON_STACK seems to be a better place to guaranty that.  But that
being said, making sure that, for AT_ENTRY_POINT, the dummy
calling address is indeed our entry point cannot make things
worse.

>  	    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.  */
                  ^^^
                  its

> +      }
> +      /* FALLTHROUGH */
> +    case AT_ENTRY_POINT:

Is this really a FALLTHROUGH?

> +	   Therefore, we use the second byte (approximately,
> +	   alignments depending on GDBARCH).  It does not matter if it
> +	   is placed inside the very first instruction, nothing tries
> +	   to execute it.  */

I can't remember if you explicitly decided to use the second byte
and then changed your mind, or whether this is a typo from the fact
that the breakpoint instruction on x86 is 1 byte long? Suggest
replacing with:

        Therefore, we adjust the return address by the length
        of a breakpoint, guaranteeing that the unwinder finds
        the correct function as the caller.

-- 
Joel

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

[Mark Kettenis]

> Date: Fri, 30 Dec 2011 07:30:20 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > As ON_STACK is a valid option sure one may prefer to convert all the
> > archs to ON_STACK instead of fixing AT_ENTRY_POINT; not preferred by
> > me TBH.
> 
> I don't understand why, though. ON_STACK seems to be perfect, as
> we control exactly what's there, and we know we're not going to
> interfere with the rest of the code.

I believe it has always been the intention to make ON_STACK the
default.  See the comment by Andrew Cagney in mips-tdep.c.  For some
architectures it is the only viable option.

I'll probably switch i386/amd64 to use ON_STACK, since it allows for
some cleanups in the DICOS support.

> Are there any situation where ON_STACK wouldn't be possible?

I don't think so.

> I have put in my TODO list to see if I can get rid of the last
> use of AT_SYMBOL (in mips-tdep.c) and convert it to ON_STACK.

That'd be good since mips-tdep.c seems to be the only one left that
still uses AT_SYMBOL.  Would be nice if we can remove that code.

> > 2011-12-30  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Fix regression for gdb.cp/gdb2495.exp with gcc-4.7.
> > 	* arch-utils.c (displaced_step_at_entry_point): Incrase BP_LEN skip to
> > 	3 times.
> > 	* infcall.c (call_function_by_hand) <AT_SYMBOL>: Move it upwards and
> > 	fall through into AT_ENTRY_POINT.
> > 	(call_function_by_hand) <AT_ENTRY_POINT>: New variable bp_len.  Adjust
> > 	DUMMY_ADDR with it.
> > 	* ppc-linux-tdep.c (ppc_linux_displaced_step_location): Increase
> > 	PPC_INSN_SIZE skip to 3 times.
> 
> I keep staring at the diff, and I can't figure out how the AT_SYMBOL
> case is falling through, or why it would be necessary. The lack of
> understading why is probably related to the fact that I may still
> have an incorrect understanding of the situation.

The idea is that AT_SYMBOL will fall back on putting the call dummy
breakpoint at the entry point if the magic symbol name isn't found.
Currently this is achieved by having duplicated code.  Jan's diff
changes it in a FALLTHROUGH to make it more explicit.

> I think that either way, it's better to have the dummy calling
> address at a location where there is no unwinding information.
> ON_STACK seems to be a better place to guaranty that.  But that
> being said, making sure that, for AT_ENTRY_POINT, the dummy
> calling address is indeed our entry point cannot make things
> worse.

I'm still a little bit worried that Jan's approach is making
additional assumptions.  The chance that the 2nd instruction of the
program will be executed again as part of the normal flow is probably
not much higher than for the 1st instructions, but the 1st instruction
could be a jump.  And I know Jan has checked his diff on ia64, but
there might be other architectures that have the concept of
instruction bundles where jumping into the middle of a bundle doesn't
work.

I think I'd be in favour of switching as many targets as possible to
ON_STACK, remove AT_SYMBOL and leave AT_ENTRY_POINT alone.  But I
don't feel too strongly about this.  Targets that switch to ON_STACK
won't be affected by assumptions in AT_ENTRY_POINT that turn out not
to be true.  And if we manage to switch all targets to ON_STACK, we
can simply delete AT_ENTRY_POINT.

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

[Jan Kratochvil]

On Fri, 30 Dec 2011 04:30:20 +0100, Joel Brobecker wrote:
> >  not preferred by me TBH.
> 
> I don't understand why, though. ON_STACK seems to be perfect, as
> we control exactly what's there,

There exist already so many security frameworks in the toolchain and Linux
kernel I expect it will have some compatibility problems with them.
But I was unable to see any problems at least with RHEl-6.2 SELinux targeted
policy and sandbox_t containment and I do not know any other real countercase.


> I keep staring at the diff,

In this case the diff is not much readable, one should read the patched code
IMO.


> and I can't figure out how the AT_SYMBOL case is falling through,

There was one leftover "break;", thanks.


> > +      /* FALLTHROUGH */
> > +    case AT_ENTRY_POINT:
> 
> Is this really a FALLTHROUGH?

It was not, it is now.


> I can't remember if you explicitly decided to use the second byte
> and then changed your mind, or whether this is a typo from the fact
> that the breakpoint instruction on x86 is 1 byte long? Suggest
> replacing with:
> 
>         Therefore, we adjust the return address by the length
>         of a breakpoint, guaranteeing that the unwinder finds
>         the correct function as the caller.

I agree my text was wrong.  It was instruction length placed there before.  It
is now breakpoint length in this patch but it was incorrectly described as
"byte" (thinking about x86* myself while writing it).


Thanks,
Jan


2011-12-30  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Joel Brobecker  <brobecker@adacore.com>

	Fix regression for gdb.cp/gdb2495.exp with gcc-4.7.
	* arch-utils.c (displaced_step_at_entry_point): Incrase BP_LEN skip to
	3 times.
	* infcall.c (call_function_by_hand) <AT_SYMBOL>: Move it upwards and
	fall through into AT_ENTRY_POINT.
	(call_function_by_hand) <AT_ENTRY_POINT>: New variable bp_len.  Adjust
	DUMMY_ADDR with it.
	* ppc-linux-tdep.c (ppc_linux_displaced_step_location): Increase
	PPC_INSN_SIZE skip to 3 times.

--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -86,7 +86,7 @@ displaced_step_at_entry_point (struct gdbarch *gdbarch)
      We don't want displaced stepping to interfere with those
      breakpoints, so leave space.  */
   gdbarch_breakpoint_from_pc (gdbarch, &addr, &bp_len);
-  addr += bp_len * 2;
+  addr += bp_len * 3;
 
   return addr;
 }
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -631,17 +631,6 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 				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
@@ -661,11 +650,39 @@ 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 its address is the same as the address of the dummy.  */
+	    bp_addr = dummy_addr;
+	    break;
 	  }
-	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.  */
+      }
+      /* FALLTHROUGH */
+    case AT_ENTRY_POINT:
+      {
+	CORE_ADDR dummy_addr;
+	int bp_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 adjust the return address by the length of
+	   a breakpoint, guaranteeing that the unwinder finds the
+	   correct function as the caller.  */
+
+	gdbarch_breakpoint_from_pc (gdbarch, &dummy_addr, &bp_len);
+	dummy_addr += bp_len;
+
+	/* 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;
       }
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1075,7 +1075,7 @@ ppc_linux_displaced_step_location (struct gdbarch *gdbarch)
       /* Inferior calls also use the entry point as a breakpoint location.
 	 We don't want displaced stepping to interfere with those
 	 breakpoints, so leave space.  */
-      ppc_linux_entry_point_addr = addr + 2 * PPC_INSN_SIZE;
+      ppc_linux_entry_point_addr = addr + 3 * PPC_INSN_SIZE;
     }
 
   return ppc_linux_entry_point_addr;

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

[Jan Kratochvil]

On Fri, 30 Dec 2011 11:38:59 +0100, Mark Kettenis wrote:
> I'm still a little bit worried that Jan's approach is making
> additional assumptions.  The chance that the 2nd instruction of the
> program will be executed again as part of the normal flow is probably
> not much higher than for the 1st instructions,

The patched code does not use the original entry_point_address in any way,
it passes to gdbarch_push_dummy_call already the adjusted address as bp_addr.

This is also the reason why it no longer uses gdb_buffered_insn_length but
just gdbarch_breakpoint_from_pc's bp_len is enough.  We may place the
breakpoint inside the first instruction ("corrupting" it) but as the first
instruction is not used anywhere it does not matter.  The inferior call will
return right to our placed breakpoint address.


> And I know Jan has checked his diff on ia64, but there might be other
> architectures that have the concept of instruction bundles where jumping
> into the middle of a bundle doesn't work.

It is only important entry_point_address + bp_len is still a valid placement
for a breakpoint.  ia64 will place it to the next bundle (+16 bytes) which
will work.

Still I can imagine for example big endian architecture with 32-bit fixed
lenght instructions where any opcode 0xFF?????? is a breakpoint and therefore
its gdbarch_breakpoint_from_pc returns BP_LEN as 1.  But we cannot place
our breakpoint on entry_point_address + 1.  Unaware if such arch exists.
My patch would break such arch.  It also means the current
displaced_step_at_entry_point implementation for displaced stepping would not
work on that arch, if one would try to implemented displaced stepping there.


Thanks,
Jan

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

[Peter Schauer]

> > Date: Fri, 30 Dec 2011 07:30:20 +0400
> > From: Joel Brobecker <brobecker@adacore.com>
> > 
> > > As ON_STACK is a valid option sure one may prefer to convert all the
> > > archs to ON_STACK instead of fixing AT_ENTRY_POINT; not preferred by
> > > me TBH.
> > 
> > I don't understand why, though. ON_STACK seems to be perfect, as
> > we control exactly what's there, and we know we're not going to
> > interfere with the rest of the code.
> 
> I believe it has always been the intention to make ON_STACK the
> default.  See the comment by Andrew Cagney in mips-tdep.c.  For some
> architectures it is the only viable option.
> 
> I'll probably switch i386/amd64 to use ON_STACK, since it allows for
> some cleanups in the DICOS support.
> 
> > Are there any situation where ON_STACK wouldn't be possible?
> 
> I don't think so.
> 
> > I have put in my TODO list to see if I can get rid of the last
> > use of AT_SYMBOL (in mips-tdep.c) and convert it to ON_STACK.
> 
> That'd be good since mips-tdep.c seems to be the only one left that
> still uses AT_SYMBOL.  Would be nice if we can remove that code.
> 
> > > 2011-12-30  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > > 
> > > 	Fix regression for gdb.cp/gdb2495.exp with gcc-4.7.
> > > 	* arch-utils.c (displaced_step_at_entry_point): Incrase BP_LEN skip to
> > > 	3 times.
> > > 	* infcall.c (call_function_by_hand) <AT_SYMBOL>: Move it upwards and
> > > 	fall through into AT_ENTRY_POINT.
> > > 	(call_function_by_hand) <AT_ENTRY_POINT>: New variable bp_len.  Adjust
> > > 	DUMMY_ADDR with it.
> > > 	* ppc-linux-tdep.c (ppc_linux_displaced_step_location): Increase
> > > 	PPC_INSN_SIZE skip to 3 times.
> > 
> > I keep staring at the diff, and I can't figure out how the AT_SYMBOL
> > case is falling through, or why it would be necessary. The lack of
> > understading why is probably related to the fact that I may still
> > have an incorrect understanding of the situation.
> 
> The idea is that AT_SYMBOL will fall back on putting the call dummy
> breakpoint at the entry point if the magic symbol name isn't found.
> Currently this is achieved by having duplicated code.  Jan's diff
> changes it in a FALLTHROUGH to make it more explicit.
> 
> > I think that either way, it's better to have the dummy calling
> > address at a location where there is no unwinding information.
> > ON_STACK seems to be a better place to guaranty that.  But that
> > being said, making sure that, for AT_ENTRY_POINT, the dummy
> > calling address is indeed our entry point cannot make things
> > worse.
> 
> I'm still a little bit worried that Jan's approach is making
> additional assumptions.  The chance that the 2nd instruction of the
> program will be executed again as part of the normal flow is probably
> not much higher than for the 1st instructions, but the 1st instruction
> could be a jump.  And I know Jan has checked his diff on ia64, but
> there might be other architectures that have the concept of
> instruction bundles where jumping into the middle of a bundle doesn't
> work.
> 
> I think I'd be in favour of switching as many targets as possible to
> ON_STACK, remove AT_SYMBOL and leave AT_ENTRY_POINT alone.  But I
> don't feel too strongly about this.  Targets that switch to ON_STACK
> won't be affected by assumptions in AT_ENTRY_POINT that turn out not
> to be true.  And if we manage to switch all targets to ON_STACK, we
> can simply delete AT_ENTRY_POINT.

It is funny that we tried to convert all targets to use AT_ENTRY in
the past and now you are heading towards the opposite direction.

Provided that ON_STACK will handle non-executable stacks properly in
all cases, AT_ENTRY_POINT has some small additional advantage:
If the called function clobbers the stack and overwrites the breakpoint
instruction, the inferior will no longer stop after the inferior function
call when using ON_STACK.

Apart from that, I found no reasons in my age old notes why ON_STACK
could cause problems.
Let me know if you want some more historical notes on why AT_ENTRY_POINT
was introduced at all.

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

[Jan Kratochvil]

Hi Joel,

On Fri, 30 Dec 2011 12:11:04 +0100, Jan Kratochvil wrote:
> 2011-12-30  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Joel Brobecker  <brobecker@adacore.com>
> 
> 	Fix regression for gdb.cp/gdb2495.exp with gcc-4.7.
> 	* arch-utils.c (displaced_step_at_entry_point): Incrase BP_LEN skip to
> 	3 times.
> 	* infcall.c (call_function_by_hand) <AT_SYMBOL>: Move it upwards and
> 	fall through into AT_ENTRY_POINT.
> 	(call_function_by_hand) <AT_ENTRY_POINT>: New variable bp_len.  Adjust
> 	DUMMY_ADDR with it.
> 	* ppc-linux-tdep.c (ppc_linux_displaced_step_location): Increase
> 	PPC_INSN_SIZE skip to 3 times.

FYI do you still plan the 7.4 release tomorrow?  For example this should go in
IMO, gcc-4.7 will be GA (and for example gcc-4.7 built Fedora 17 GA is
2012-05-08), both before gdb-7.5 will be GA, just the HEAD files are not
rotated yet for 2012.


Thanks,
Jan

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

[Joel Brobecker]

> FYI do you still plan the 7.4 release tomorrow?  For example this should go in
> IMO, gcc-4.7 will be GA (and for example gcc-4.7 built Fedora 17 GA is
> 2012-05-08), both before gdb-7.5 will be GA, just the HEAD files are not
> rotated yet for 2012.

I am not planning on releaing GDB on the 2nd anymore. There are a couple
of patches from me, and maybe this one. For yours, there's been enough
scrutiny I think that it can go in I think that it can go in already.

PS: For the ChangeLog on the branch, yeah, no point in rotating it.
    It'd be a very small ChangeLog in the end. However, we do need
    to be careful with copyright dates, I believe. Applying the E-o-Y
    procedure takes a lot of time, and so doing on the branch as well
    would be a bit of a waste.

-- 
Joel

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

[Jan Kratochvil]

On Mon, 02 Jan 2012 03:45:32 +0100, Joel Brobecker wrote:
> and maybe this one. For yours, there's been enough
> scrutiny I think that it can go in I think that it can go in already.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2012-01/msg00013.html
Checked in 7.4:
	http://sourceware.org/ml/gdb-cvs/2012-01/msg00014.html


> PS: For the ChangeLog on the branch, yeah, no point in rotating it.
>     It'd be a very small ChangeLog in the end. However, we do need
>     to be careful with copyright dates,

I see I should have checked in for 7.4 also the patch below, right?


Thanks,
Jan


gdb/
2012-01-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* arch-utils.c: Update the copyright for 2012.
	* infcall.c: Update the copyright for 2012.
	* ppc-linux-tdep.c: Update the copyright for 2012.

--- gdb/arch-utils.c	2 Jan 2012 02:53:34 -0000	1.193.2.1
+++ gdb/arch-utils.c	2 Jan 2012 02:56:21 -0000
@@ -1,7 +1,7 @@
 /* Dynamic architecture support for GDB, the GNU debugger.
 
    Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007,
-   2008, 2009, 2010, 2011 Free Software Foundation, Inc.
+   2008, 2009, 2010, 2011, 2012 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
--- gdb/infcall.c	2 Jan 2012 02:53:34 -0000	1.147.2.1
+++ gdb/infcall.c	2 Jan 2012 02:56:22 -0000
@@ -2,7 +2,7 @@
 
    Copyright (C) 1986, 1987, 1988, 1989, 1990, 1991, 1992, 1993, 1994, 1995,
    1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007,
-   2008, 2009, 2010, 2011 Free Software Foundation, Inc.
+   2008, 2009, 2010, 2011, 2012 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
--- gdb/ppc-linux-tdep.c	2 Jan 2012 02:53:34 -0000	1.129.2.2
+++ gdb/ppc-linux-tdep.c	2 Jan 2012 02:56:22 -0000
@@ -1,7 +1,7 @@
 /* Target-dependent code for GDB, the GNU debugger.
 
    Copyright (C) 1986, 1987, 1989, 1991, 1992, 1993, 1994, 1995, 1996, 1997,
-   2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
+   2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012
    Free Software Foundation, Inc.
 
    This file is part of GDB.

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

[Pedro Alves]

On 12/22/2011 08:20 PM, Jan Kratochvil wrote:
> @@ -634,9 +635,32 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
>       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 will try to unwind all the frames incl. the dummy frame.
> +	   Using the entry_point_address directly will try to find FDE at the
> +	   function right before the entry_point_address address as the
> +	   unwinder subtracts 1 to get at the call instruction.  FDE of the
> +	   preceding function, if found, would be invalid for the dummy frame
> +	   and it would crash the inferior's unwinder.  Therefore attempt to
> +	   skip the very first instruction of entry_point_address.  */
> +

I'm confused.  Shouldn't this instead be handled in the unwind 
machinery?  Is this subtraction you refer to the 
get_frame_address_in_block one?  That already has special
handling for something like this.  Why doesn't it work?

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

[Jan Kratochvil]

On Mon, 02 Jan 2012 15:09:42 +0100, Pedro Alves wrote:
> I'm confused.  Shouldn't this instead be handled in the unwind
> machinery?  Is this subtraction you refer to the
> get_frame_address_in_block one?  That already has special
> handling for something like this.  Why doesn't it work?

This `- 1' is in inferior's:

gcc/libgcc/unwind-dw2.c:
uw_frame_state_for:
  fde = _Unwind_Find_FDE (context->ra + _Unwind_IsSignalFrame (context) - 1,
                          &context->bases);

GDB can only change the address put on inferior stack and later picked up by
the inferior's exceptions unwinder.


Thanks,
Jan

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

[Pedro Alves]

On 01/02/2012 02:19 PM, Jan Kratochvil wrote:
> On Mon, 02 Jan 2012 15:09:42 +0100, Pedro Alves wrote:
>> I'm confused.  Shouldn't this instead be handled in the unwind
>> machinery?  Is this subtraction you refer to the
>> get_frame_address_in_block one?  That already has special
>> handling for something like this.  Why doesn't it work?
>
> This `- 1' is in inferior's:
>
> gcc/libgcc/unwind-dw2.c:
> uw_frame_state_for:
>    fde = _Unwind_Find_FDE (context->ra + _Unwind_IsSignalFrame (context) - 1,
>                            &context->bases);
>
> GDB can only change the address put on inferior stack and later picked up by
> the inferior's exceptions unwinder.

I see.  That '_Unwind_IsSignalFrame(context)' is there to cancel out the 
'- 1' for signal frames.  Ideally, the same treatment would be
applied for gdb's dummy frame, perhaps by having gdb itself change 
`context->flags' making libunwind treat it as a signal frame.  I have no 
idea whether that's feasible.  Oh well.

-- 
Pedro Alves

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

[Jan Kratochvil]

On Mon, 02 Jan 2012 15:44:02 +0100, Pedro Alves wrote:
> I see.  That '_Unwind_IsSignalFrame(context)' is there to cancel out
> the '- 1' for signal frames.  Ideally, the same treatment would be
> applied for gdb's dummy frame, perhaps by having gdb itself change
> `context->flags' making libunwind treat it as a signal frame.  I
> have no idea whether that's feasible.  Oh well.

It needs to be universal, at least for both libgcc and libunwind, via the
_Unwind_* API.

And this signal frame indication (such as 'S' in .eh_frame CIE augmentation)
would need to be present in the frame for the called code.  The caller frame
<function called from gdb> is already too late, that one gets already wrongly
unwound.

I do not find it feasible myself.


Thanks,
Jan

Regression on PowerPC (Re: [patch] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #3)

[Ulrich Weigand]

Jan Kratochvil wrote:
> On Mon, 02 Jan 2012 03:45:32 +0100, Joel Brobecker wrote:
> > and maybe this one. For yours, there's been enough
> > scrutiny I think that it can go in I think that it can go in already.
> 
> Checked in:
> 	http://sourceware.org/ml/gdb-cvs/2012-01/msg00013.html
> Checked in 7.4:
> 	http://sourceware.org/ml/gdb-cvs/2012-01/msg00014.html

This seems to have caused

FAIL: gdb.cp/gdb2495.exp: Call a function that raises an exception without a handler. (timeout)
FAIL: gdb.cp/gdb2495.exp: bt after returning from a popped frame (timeout)
FAIL: gdb.cp/gdb2495.exp: info breakpoints (timeout)
FAIL: gdb.cp/gdb2495.exp: (timeout) set unwind-on-terminating-exception off
FAIL: gdb.cp/gdb2495.exp: Turn off unwind on terminating exception flag (timeout)
FAIL: gdb.cp/gdb2495.exp: Call a function that raises an exception with unwinding off.. (timeout)
ERROR: Delete all breakpoints in delete_breakpoints (timeout)
UNRESOLVED: gdb.cp/gdb2495.exp: setting breakpoint at main (timeout)

on powerpc-linux and powerpc64-linux.

I think the problem is that your statement as of here:

"_start at least on GNU/Linux does not have .eh_frame FDE for itself."
(http://sourceware.org/ml/gdb-patches/2011-12/msg00873.html)

is actually not true on PowerPC (at least on my RHEL5 system):

[uweigand@cellntc3 ~]$ readelf -wf /usr/lib64/crt1.o 
The section .eh_frame contains:

00000000 00000010 00000000 CIE
  Version:               1
  Augmentation:          "zR"
  Code alignment factor: 4
  Data alignment factor: -8
  Return address column: 65
  Augmentation data:     1b

  DW_CFA_def_cfa: r1 ofs 0

00000014 00000010 00000018 FDE cie=00000000 pc=00000000..00000024
  DW_CFA_nop
  DW_CFA_nop
  DW_CFA_nop


When the unwinder runs into this FDE and assumes it is valid for the
dummy frame, it goes into an endless loop ...


Switching to the ON_STACK method fixes this for me.  But I'm not sure
if there are other platforms beside PowerPC that have the same problem ...
Maybe we ought to default to ON_STACK (at least on Linux?)?

Note that it's a bit annoying that we need to provide a push_dummy_code
routine in order to be able to use ON_STACK.  Maybe there should be a
default implementation based on gdbarch_inner_than/gdbarch_frame_align_p
and the breakpoint length?


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: Regression on PowerPC (Re: [patch] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #3)

[Joel Brobecker]

> > Checked in:
> > 	http://sourceware.org/ml/gdb-cvs/2012-01/msg00013.html
> > Checked in 7.4:
> > 	http://sourceware.org/ml/gdb-cvs/2012-01/msg00014.html

ARGH! Thanks for letting us know.

> Switching to the ON_STACK method fixes this for me.  But I'm not sure
> if there are other platforms beside PowerPC that have the same problem ...
> Maybe we ought to default to ON_STACK (at least on Linux?)?

But this wouldn't prevent the same sort of problem on other
platforms. I think we don't know enough to keep this change in
as is.

It seems to me, at this point, that indeed the best way to solve
the original problem is to transition the GNU/Linux platforms to
ON_STACK. We then no longer need the patch that causes the regression.

> Note that it's a bit annoying that we need to provide a push_dummy_code
> routine in order to be able to use ON_STACK.  Maybe there should be a
> default implementation based on gdbarch_inner_than/gdbarch_frame_align_p
> and the breakpoint length?

Agreed. We actually used to have such as routine, and deleted it
a while ago, after the last caller got removed. I reposted it
when I sent the patch series to add support for VxWorks.

The removal:
http://www.sourceware.org/ml/gdb-patches/2008-09/msg00320.html

The code:

> static CORE_ADDR
> rs6000_vxworks_push_dummy_code (struct gdbarch *gdbarch,
>                                 CORE_ADDR sp, CORE_ADDR funaddr,
>                                 struct value **args, int nargs,
>                                 struct type *value_type,
>                                 CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
>                                 struct regcache *regcache)
> {
>   /* Something here to findout the size of a breakpoint and then
>      allocate space for it on the stack.  */
>   int bplen;
>   /* This code assumes frame align.  */
>   gdb_assert (gdbarch_frame_align_p (gdbarch));
>   /* Force the stack's alignment.  The intent is to ensure that the SP
>      is aligned to at least a breakpoint instruction's boundary.  */
>   sp = gdbarch_frame_align (gdbarch, sp);
>   /* Allocate space for, and then position the breakpoint on the stack.  */
>   if (gdbarch_inner_than (gdbarch, 1, 2))
>     {
>       CORE_ADDR bppc = sp;
>       gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
>       sp = gdbarch_frame_align (gdbarch, sp - bplen);
>       (*bp_addr) = sp;
>       /* Should the breakpoint size/location be re-computed here?  */
>     }
>   else
>     {
>       (*bp_addr) = sp;
>       gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bplen);
>       sp = gdbarch_frame_align (gdbarch, sp + bplen);
>     }
>   /* Inferior resumes at the function entry point.  */
>   (*real_pc) = funaddr;
>   return sp;
> }

-- 
Joel

[revert] Regression on PowerPC (Re: [patch] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #3)

[Jan Kratochvil]

On Tue, 03 Jan 2012 15:44:53 +0100, Ulrich Weigand wrote:
> This seems to have caused
> 
> FAIL: gdb.cp/gdb2495.exp: Call a function that raises an exception without a handler. (timeout)

Thanks for catching it.


> Switching to the ON_STACK method fixes this for me.  But I'm not sure
> if there are other platforms beside PowerPC that have the same problem ...
> Maybe we ought to default to ON_STACK (at least on Linux?)?

Both AT_ENTRY_POINT solutions have some problems so I agree it is better to
keep it as is before some complete fix is implemented.  Reverted my patch:
	http://sourceware.org/ml/gdb-cvs/2012-01/msg00042.html
and reverted it also from 7.4:
	http://sourceware.org/ml/gdb-cvs/2012-01/msg00043.html

In fact the whole issue isn't so serious (exception thrown out of infcall).


> Maybe there should be a default implementation based on
> gdbarch_inner_than/gdbarch_frame_align_p and the breakpoint length?

I will return to it in some time.


On Tue, 03 Jan 2012 16:52:06 +0100, Joel Brobecker wrote:
> It seems to me, at this point, that indeed the best way to solve
> the original problem is to transition the GNU/Linux platforms to
> ON_STACK.

I agree, I am no longer aware how to solve it without depending on the stack
space.


Sorry,
Jan

Re: [revert] Regression on PowerPC (Re: [patch] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #3)

[Joel Brobecker]

> In fact the whole issue isn't so serious (exception thrown out of infcall).

Ah, ok! I was going to add this to the Wiki as a blocking problem
for the release...

Thanks for letting us know.

-- 
Joel

[patch] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #4 [Re: [revert] Regression on PowerPC]

[Jan Kratochvil]

On Wed, 04 Jan 2012 15:01:04 +0100, Jan Kratochvil wrote:
> On Tue, 03 Jan 2012 15:44:53 +0100, Ulrich Weigand wrote:
> > Maybe there should be a default implementation based on
> > gdbarch_inner_than/gdbarch_frame_align_p and the breakpoint length?
> 
> I will return to it in some time.

I made a copy from dicos-tdep.

No regressions on {x86_64,x86_64-m32,i686} many Fedora distros tried.


Thanks,
Jan


2012-03-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* amd64-linux-tdep.c: Include inferior.h.
	(amd64_linux_init_abi): Set ON_STACK and i386_linux_push_dummy_code.
	* i386-linux-tdep.c (i386_linux_push_dummy_code): New function.
	(i386_linux_init_abi): Set ON_STACK and i386_linux_push_dummy_code.
	* i386-tdep.h (i386_linux_push_dummy_code): New declaration.

--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -32,6 +32,7 @@
 #include "i386-linux-tdep.h"
 #include "linux-tdep.h"
 #include "i386-xstate.h"
+#include "inferior.h"
 
 #include "gdb_string.h"
 
@@ -1530,6 +1531,9 @@ amd64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   amd64_linux_record_tdep.arg6 = AMD64_R9_REGNUM;
 
   tdep->i386_syscall_record = amd64_linux_syscall_record;
+
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code (gdbarch, i386_linux_push_dummy_code);
 }
 \f
 
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -704,6 +704,26 @@ i386_linux_displaced_step_copy_insn (struct gdbarch *gdbarch,
   return closure;
 }
 
+/* Implementation for set_gdbarch_push_dummy_code.  */
+
+CORE_ADDR
+i386_linux_push_dummy_code (struct gdbarch *gdbarch,
+			    CORE_ADDR sp, CORE_ADDR funaddr,
+			    struct value **args, int nargs,
+			    struct type *value_type,
+			    CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
+			    struct regcache *regcache)
+{
+  int bplen;
+  CORE_ADDR bppc = sp;
+
+  gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
+  *bp_addr = sp - bplen;
+  *real_pc = funaddr;
+
+  return *bp_addr;
+}
+
 static void
 i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
@@ -965,6 +985,9 @@ i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
                                   i386_linux_get_syscall_number);
 
   set_gdbarch_get_siginfo_type (gdbarch, linux_get_siginfo_type);
+
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code (gdbarch, i386_linux_push_dummy_code);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
--- a/gdb/i386-tdep.h
+++ b/gdb/i386-tdep.h
@@ -379,6 +379,14 @@ extern void i386_svr4_init_abi (struct gdbarch_info, struct gdbarch *);
 
 extern int i386_process_record (struct gdbarch *gdbarch,
                                 struct regcache *regcache, CORE_ADDR addr);
+
+extern CORE_ADDR i386_linux_push_dummy_code (struct gdbarch *gdbarch,
+					     CORE_ADDR sp, CORE_ADDR funaddr,
+					     struct value **args, int nargs,
+					     struct type *value_type,
+					     CORE_ADDR *real_pc,
+					     CORE_ADDR *bp_addr,
+					     struct regcache *regcache);
 \f
 
 /* Functions and variables exported from i386bsd-tdep.c.  */

cancel: [patch] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #4 [Re: [revert] Regression on PowerPC]

[Jan Kratochvil]

On Fri, 09 Mar 2012 00:23:45 +0100, Jan Kratochvil wrote:
> 2012-03-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* amd64-linux-tdep.c: Include inferior.h.
> 	(amd64_linux_init_abi): Set ON_STACK and i386_linux_push_dummy_code.
> 	* i386-linux-tdep.c (i386_linux_push_dummy_code): New function.
> 	(i386_linux_init_abi): Set ON_STACK and i386_linux_push_dummy_code.
> 	* i386-tdep.h (i386_linux_push_dummy_code): New declaration.

FYI cancelling this patch, it has some issues:

(1) As i386-tdep.c already has OS-independent i386_push_dummy_call I believe
    push_dummy_code can be also put into i386-tdep.c.

(2) At least on RHEL-5 i386 there is a regression:
    FAIL: gdb.base/call-signal-resume.exp: continue to program exit

#0  null_hand_call () at ./gdb.base/call-signals.c:56
#1  <function called from gdb>
#2  0xf7fe0430 in __kernel_vsyscall ()
#3  0xf7e7d236 in kill () from /lib/libc.so.6
#4  0x080484f4 in gen_signal () at ./gdb.base/call-signals.c:35
#5  0x08048574 in main () at ./gdb.base/call-signals.c:81
->
#0  null_hand_call () at ./gdb.base/call-signals.c:56
#1  <function called from gdb>
#2  0xf7fe0430 in __kernel_vsyscall ()
#3  0xf7e7d236 in kill () from /lib/libc.so.6
#4  0xf70484f4 in ?? ()
#5  0x00004b03 in ?? ()
#6  0x00000006 in ?? ()
#7  0xffffd068 in ?? ()
#8  0x08048574 in main () at ./gdb.base/call-signals.c:81

This is because (a) the dummy frame has no associated DWARF/EH frame unwind
info and i386_push_dummy_call does not setup proper frame pointer in the new
frame.  Moreover I believe it should be setup with 0 frame pointer / 0 return
address to indicate end of unwinding for possible inferior unwinders, IMO
inferior should not unwind through the dummy frame.  And then therefore GDB
needs new frame_unwind to properly unwind that 0 return address / frame
pointer.


Regards,
Jan