[commit] infcall.c cleanup - explict bp_addr variable

[commit] infcall.c cleanup - explict bp_addr variable

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

Hello,

This patch cleans up just one tiny part of the new merged 
call_function_by_hand().

It teases out an explicit "bp_addr" variable that contains the address 
of the breakpoint that the called function is to return to.  By doing 
this certain logic (such as the computation of sal.pc used to create the 
breakpoint) is simplified; and other logic (such as the initial 
computation of the "bp_addr") is localized.

The only depressing bit in this cleanup is that it is now pretty clear 
how messed up the logic to compute the ON_STACK breakpoint addr has become.

committed,
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 5121 bytes --]

2003-04-22  Andrew Cagney  <cagney@redhat.com>

	* infcall.c (call_function_by_hand): Use new variable "bp_addr" to
	compute the breakpoint address.  Only call FIX_CALL_DUMMY when
	ON_STACK.  Eliminate the variable "addr".  Do not pass "real_pc"
	to DEPRECATED_PUSH_RETURN_ADDRESS.

Index: infcall.c
===================================================================
RCS file: /cvs/src/src/gdb/infcall.c,v
retrieving revision 1.1
diff -u -r1.1 infcall.c
--- infcall.c	21 Apr 2003 16:48:39 -0000	1.1
+++ infcall.c	23 Apr 2003 14:58:16 -0000
@@ -272,6 +272,7 @@
   struct type *param_type = NULL;
   struct type *ftype = check_typedef (SYMBOL_TYPE (function));
   int n_method_args = 0;
+  CORE_ADDR bp_addr;
 
   dummy = alloca (SIZEOF_CALL_DUMMY_WORDS);
   sizeof_dummy1 = REGISTER_SIZE * SIZEOF_CALL_DUMMY_WORDS / sizeof (ULONGEST);
@@ -413,23 +414,33 @@
 			    REGISTER_SIZE,
 			    (ULONGEST) dummy[i]);
 
-#ifdef GDB_TARGET_IS_HPPA
-  real_pc = FIX_CALL_DUMMY (dummy1, start_sp, funaddr, nargs, args,
-			    value_type, using_gcc);
-#else
-  if (FIX_CALL_DUMMY_P ())
-    {
-      /* gdb_assert (CALL_DUMMY_LOCATION == ON_STACK) true?  */
-      FIX_CALL_DUMMY (dummy1, start_sp, funaddr, nargs, args, value_type,
-		      using_gcc);
-    }
-  real_pc = start_sp;
-#endif
-
   switch (CALL_DUMMY_LOCATION)
     {
     case ON_STACK:
+      /* NOTE: cagney/2003-04-22: This computation of REAL_PC, BP_ADDR
+         and DUMMY_ADDR is pretty messed up.  It comes from constant
+         tinkering with the values.  Instead a FIX_CALL_DUMMY
+         replacement (PUSH_DUMMY_BREAKPOINT?) should just do
+         everything.  */
+#ifdef GDB_TARGET_IS_HPPA
+      real_pc = FIX_CALL_DUMMY (dummy1, start_sp, funaddr, nargs, args,
+				value_type, using_gcc);
+#else
+      if (FIX_CALL_DUMMY_P ())
+	{
+	  /* gdb_assert (CALL_DUMMY_LOCATION == ON_STACK) true?  */
+	  FIX_CALL_DUMMY (dummy1, start_sp, funaddr, nargs, args, value_type,
+			  using_gcc);
+	}
+      real_pc = start_sp;
+#endif
       dummy_addr = start_sp;
+      /* Yes, the offset is applied to the real_pc and not the dummy
+         addr.  Ulgh!  Blame the HP/UX target.  */
+      bp_addr = real_pc + CALL_DUMMY_BREAKPOINT_OFFSET;
+      /* Yes, the offset is applied to the real_pc and not the
+         dummy_addr.  Ulgh!  Blame the HP/UX target.  */
+      real_pc += CALL_DUMMY_START_OFFSET;
       write_memory (start_sp, (char *) dummy1, sizeof_dummy1);
       if (DEPRECATED_USE_GENERIC_DUMMY_FRAMES)
 	generic_save_call_dummy_addr (start_sp, start_sp + sizeof_dummy1);
@@ -437,6 +448,9 @@
     case AT_ENTRY_POINT:
       real_pc = funaddr;
       dummy_addr = CALL_DUMMY_ADDRESS ();
+      /* 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;
       if (DEPRECATED_USE_GENERIC_DUMMY_FRAMES)
 	/* NOTE: cagney/2002-04-13: The entry point is going to be
            modified with a single breakpoint.  */
@@ -649,7 +663,13 @@
        return-address register as appropriate.  Formerly this has been
        done in PUSH_ARGUMENTS, but that's overloading its
        functionality a bit, so I'm making it explicit to do it here.  */
-    sp = DEPRECATED_PUSH_RETURN_ADDRESS (real_pc, sp);
+    /* NOTE: cagney/2003-04-22: The first parameter ("real_pc") has
+       been replaced with zero, it turns out that no implementation
+       used that parameter.  This occured because the value being
+       supplied - the address of the called function's entry point
+       instead of the address of the breakpoint that the called
+       function should return to - wasn't useful.  */
+    sp = DEPRECATED_PUSH_RETURN_ADDRESS (0, sp);
 
   /* NOTE: cagney/2003-03-23: Diable this code when there is a
      push_dummy_call() method.  Since that method will have already
@@ -745,7 +765,6 @@
 	 eventually be popped when we do hit the dummy end
 	 breakpoint).  */
 
-      CORE_ADDR addr = real_pc + CALL_DUMMY_START_OFFSET;
       struct regcache *buffer = retbuf;
       struct cleanup *old_cleanups = make_cleanup (null_cleanup, 0);
       int saved_async = 0;
@@ -756,22 +775,7 @@
       clear_proceed_status ();
 
       init_sal (&sal);		/* initialize to zeroes */
-      if (CALL_DUMMY_LOCATION == AT_ENTRY_POINT)
-	{
-	  sal.pc = CALL_DUMMY_ADDRESS ();
-	}
-      else
-	{
-	  /* If defined, CALL_DUMMY_BREAKPOINT_OFFSET is where we need
-	     to put a breakpoint instruction.  If not, the call dummy
-	     already has the breakpoint instruction in it.
-
-	     ADDR IS THE ADDRESS of the call dummy plus the
-	     CALL_DUMMY_START_OFFSET, so we need to subtract the
-	     CALL_DUMMY_START_OFFSET.  */
-	  sal.pc = (addr - (CALL_DUMMY_START_OFFSET
-			    + CALL_DUMMY_BREAKPOINT_OFFSET));
-	}
+      sal.pc = bp_addr;
       sal.section = find_pc_overlay (sal.pc);
   
       {
@@ -797,7 +801,7 @@
       if (target_can_async_p ())
 	saved_async = target_async_mask (0);
 
-      proceed (addr, TARGET_SIGNAL_0, 0);
+      proceed (real_pc, TARGET_SIGNAL_0, 0);
 
       if (saved_async)
 	target_async_mask (saved_async);

Re: [commit] infcall.c cleanup - explict bp_addr variable

[Andreas Schwab]

Andrew Cagney <ac131313@redhat.com> writes:

|> @@ -649,7 +663,13 @@
|>         return-address register as appropriate.  Formerly this has been
|>         done in PUSH_ARGUMENTS, but that's overloading its
|>         functionality a bit, so I'm making it explicit to do it here.  */
|> -    sp = DEPRECATED_PUSH_RETURN_ADDRESS (real_pc, sp);
|> +    /* NOTE: cagney/2003-04-22: The first parameter ("real_pc") has
|> +       been replaced with zero, it turns out that no implementation
|> +       used that parameter.  This occured because the value being

This is obviously wrong.  See ia64_push_return_address.  Fixes PR1256 and
most testsuite failures (there are a few new failures of things that
weren't tested before, especially backtraces from call dummies are
broken).

Andreas.

2003-06-27  Andreas Schwab  <schwab@suse.de>

	* infcall.c (call_function_by_hand): Partially revert change of
	2003-04-22: do pass real_pc to DEPRECATED_PUSH_RETURN_ADDRESS.

--- gdb/infcall.c.~1.17.~	2003-06-20 11:44:27.000000000 +0200
+++ gdb/infcall.c	2003-06-27 21:58:41.000000000 +0200
@@ -768,13 +768,7 @@ You must use a pointer to function type 
        return-address register as appropriate.  Formerly this has been
        done in PUSH_ARGUMENTS, but that's overloading its
        functionality a bit, so I'm making it explicit to do it here.  */
-    /* NOTE: cagney/2003-04-22: The first parameter ("real_pc") has
-       been replaced with zero, it turns out that no implementation
-       used that parameter.  This occured because the value being
-       supplied - the address of the called function's entry point
-       instead of the address of the breakpoint that the called
-       function should return to - wasn't useful.  */
-    sp = DEPRECATED_PUSH_RETURN_ADDRESS (0, sp);
+    sp = DEPRECATED_PUSH_RETURN_ADDRESS (real_pc, sp);
 
   /* NOTE: cagney/2003-03-23: Diable this code when there is a
      push_dummy_call() method.  Since that method will have already

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [commit] infcall.c cleanup - explict bp_addr variable

[Andrew Cagney]

> Andrew Cagney <ac131313@redhat.com> writes:
> 
> |> @@ -649,7 +663,13 @@
> |>         return-address register as appropriate.  Formerly this has been
> |>         done in PUSH_ARGUMENTS, but that's overloading its
> |>         functionality a bit, so I'm making it explicit to do it here.  */
> |> -    sp = DEPRECATED_PUSH_RETURN_ADDRESS (real_pc, sp);
> |> +    /* NOTE: cagney/2003-04-22: The first parameter ("real_pc") has
> |> +       been replaced with zero, it turns out that no implementation
> |> +       used that parameter.  This occured because the value being
> 
> This is obviously wrong.  See ia64_push_return_address.  Fixes PR1256 and
> most testsuite failures (there are a few new failures of things that
> weren't tested before, especially backtraces from call dummies are
> broken).

In a word, outch!  (I missed that).

> static CORE_ADDR
> ia64_push_return_address (CORE_ADDR pc, CORE_ADDR sp)
> {
>   CORE_ADDR global_pointer = FIND_GLOBAL_POINTER (pc);
> 
>   if (global_pointer != 0)
>     write_register (IA64_GR1_REGNUM, global_pointer);
> 
>   write_register (IA64_BR0_REGNUM, CALL_DUMMY_ADDRESS ());
>   return sp;
> }

Does, instead, a s/push_arguments/push_dummy_call/ and move the above 
code to push_dummy_call (PC -> FUNC_ADDR, CALL_DUMMY_ADDRESS -> BP_ADDR) 
work?

Andrew


> Andreas.
> 
> 2003-06-27  Andreas Schwab  <schwab@suse.de>
> 
> 	* infcall.c (call_function_by_hand): Partially revert change of
> 	2003-04-22: do pass real_pc to DEPRECATED_PUSH_RETURN_ADDRESS.
> 
> --- gdb/infcall.c.~1.17.~	2003-06-20 11:44:27.000000000 +0200
> +++ gdb/infcall.c	2003-06-27 21:58:41.000000000 +0200
> @@ -768,13 +768,7 @@ You must use a pointer to function type 
>         return-address register as appropriate.  Formerly this has been
>         done in PUSH_ARGUMENTS, but that's overloading its
>         functionality a bit, so I'm making it explicit to do it here.  */
> -    /* NOTE: cagney/2003-04-22: The first parameter ("real_pc") has
> -       been replaced with zero, it turns out that no implementation
> -       used that parameter.  This occured because the value being
> -       supplied - the address of the called function's entry point
> -       instead of the address of the breakpoint that the called
> -       function should return to - wasn't useful.  */
> -    sp = DEPRECATED_PUSH_RETURN_ADDRESS (0, sp);
> +    sp = DEPRECATED_PUSH_RETURN_ADDRESS (real_pc, sp);
>  
>    /* NOTE: cagney/2003-03-23: Diable this code when there is a
>       push_dummy_call() method.  Since that method will have already
> 
> -- Andreas Schwab, SuSE Labs, schwab@suse.de SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." 

Re: [commit] infcall.c cleanup - explict bp_addr variable

[Andreas Schwab]

Andrew Cagney <ac131313@redhat.com> writes:

|> Does, instead, a s/push_arguments/push_dummy_call/ and move the above code
|> to push_dummy_call (PC -> FUNC_ADDR, CALL_DUMMY_ADDRESS -> BP_ADDR) work?

Yes, that seems to work.  I'll prepare a patch that also replaces the
deprecated read/write_register calls with the regcache functions.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [commit] infcall.c cleanup - explict bp_addr variable

[Andreas Schwab]

Andrew Cagney <ac131313@redhat.com> writes:

|> Does, instead, a s/push_arguments/push_dummy_call/ and move the above code
|> to push_dummy_call (PC -> FUNC_ADDR, CALL_DUMMY_ADDRESS -> BP_ADDR) work?

Here is a patch that does the renaming and modernizes it.  Fixes almost
all tests that do inferior calls.  One thing that's still broken is
backtraces from dummy frames (causing an infinite loop).  I'll try to fix
that too.

 		=== gdb Summary ===
 
-# of expected passes		8654
-# of unexpected failures	324
+# of expected passes		8781
+# of unexpected failures	195
 # of unexpected successes	3
 # of expected failures		45
 # of known failures		23
-# of unresolved testcases	38
+# of unresolved testcases	41
 # of untested testcases		6
 # of unsupported tests		2
 /cvs/test/branch/gdb/gdb/testsuite/../../gdb/gdb version  5.3.90_2003-07-01-cvs -nx

Andreas.

2003-07-01  Andreas Schwab  <schwab@suse.de>

	* ia64-tdep.c (ia64_push_dummy_call): Define as combination of
	former ia64_push_arguments and ia64_push_return_address, and use
	regcache functions instead of read/write_register.
	(ia64_gdbarch_init): Set push_dummy_call instead of
	deprecated_push_arguments and deprecated_push_return_address.

--- gdb/ia64-tdep.c.~1.89.~	2003-06-23 11:44:21.000000000 +0200
+++ gdb/ia64-tdep.c	2003-07-01 19:38:43.000000000 +0200
@@ -1520,7 +1520,7 @@ static void
 ia64_store_struct_return (CORE_ADDR addr, CORE_ADDR sp)
 {
   /* FIXME: See above. */
-  /* Note that most of the work was done in ia64_push_arguments() */
+  /* Note that most of the work was done in ia64_push_dummy_call() */
   struct_return_address = addr;
 }
 
@@ -1853,8 +1853,10 @@ find_func_descr (CORE_ADDR faddr, CORE_A
 }
 
 static CORE_ADDR
-ia64_push_arguments (int nargs, struct value **args, CORE_ADDR sp,
-		    int struct_return, CORE_ADDR struct_addr)
+ia64_push_dummy_call (struct gdbarch *gdbarch, CORE_ADDR func_addr,
+		      struct regcache *regcache, CORE_ADDR bp_addr,
+		      int nargs, struct value **args, CORE_ADDR sp,
+		      int struct_return, CORE_ADDR struct_addr)
 {
   int argno;
   struct value *arg;
@@ -1862,7 +1864,9 @@ ia64_push_arguments (int nargs, struct v
   int len, argoffset;
   int nslots, rseslots, memslots, slotnum, nfuncargs;
   int floatreg;
-  CORE_ADDR bsp, cfm, pfs, new_bsp, funcdescaddr;
+  ULONGEST bsp, cfm, pfs, new_bsp;
+  CORE_ADDR funcdescaddr;
+  ULONGEST global_pointer = FIND_GLOBAL_POINTER (func_addr);
 
   nslots = 0;
   nfuncargs = 0;
@@ -1887,21 +1891,21 @@ ia64_push_arguments (int nargs, struct v
   memslots = nslots - rseslots;
 
   /* Allocate a new RSE frame */
-  cfm = read_register (IA64_CFM_REGNUM);
+  regcache_cooked_read_unsigned (regcache, IA64_CFM_REGNUM, &cfm);
 
-  bsp = read_register (IA64_BSP_REGNUM);
+  regcache_cooked_read_unsigned (regcache, IA64_BSP_REGNUM, &bsp);
   bsp = rse_address_add (bsp, cfm & 0x7f);
   new_bsp = rse_address_add (bsp, rseslots);
-  write_register (IA64_BSP_REGNUM, new_bsp);
+  regcache_cooked_write_unsigned (regcache, IA64_BSP_REGNUM, new_bsp);
 
-  pfs = read_register (IA64_PFS_REGNUM);
+  regcache_cooked_read_unsigned (regcache, IA64_PFS_REGNUM, &pfs);
   pfs &= 0xc000000000000000LL;
   pfs |= (cfm & 0xffffffffffffLL);
-  write_register (IA64_PFS_REGNUM, pfs);
+  regcache_cooked_write_unsigned (regcache, IA64_PFS_REGNUM, pfs);
 
   cfm &= 0xc000000000000000LL;
   cfm |= rseslots;
-  write_register (IA64_CFM_REGNUM, cfm);
+  regcache_cooked_write_unsigned (regcache, IA64_CFM_REGNUM, cfm);
   
   /* We will attempt to find function descriptors in the .opd segment,
      but if we can't we'll construct them ourselves.  That being the
@@ -1979,11 +1983,12 @@ ia64_push_arguments (int nargs, struct v
 	  len = TYPE_LENGTH (type);
 	  while (len > 0 && floatreg < IA64_FR16_REGNUM)
 	    {
-	      ia64_register_convert_to_raw (
-		float_elt_type,
-		floatreg,
-	        VALUE_CONTENTS (arg) + argoffset,
-		&deprecated_registers[REGISTER_BYTE (floatreg)]);
+	      char buf[MAX_REGISTER_SIZE];
+	      ia64_register_convert_to_raw (float_elt_type,
+					    floatreg,
+					    VALUE_CONTENTS (arg) + argoffset,
+					    buf);
+	      regcache_raw_write (regcache, floatreg, buf);
 	      floatreg++;
 	      argoffset += TYPE_LENGTH (float_elt_type);
 	      len -= TYPE_LENGTH (float_elt_type);
@@ -1994,11 +1999,14 @@ ia64_push_arguments (int nargs, struct v
   /* Store the struct return value in r8 if necessary. */
   if (struct_return)
     {
-      store_unsigned_integer (&deprecated_registers[REGISTER_BYTE (IA64_GR8_REGNUM)],
-			      REGISTER_RAW_SIZE (IA64_GR8_REGNUM),
-			      struct_addr);
+      regcache_cooked_write_unsigned (regcache, IA64_GR8_REGNUM, struct_addr);
     }
 
+  if (global_pointer != 0)
+    regcache_cooked_write_unsigned (regcache, IA64_GR1_REGNUM, global_pointer);
+
+  regcache_cooked_write_unsigned (regcache, IA64_BR0_REGNUM, bp_addr);
+
   /* Sync gdb's idea of what the registers are with the target. */
   target_store_registers (-1);
 
@@ -2018,18 +2026,6 @@ ia64_push_arguments (int nargs, struct v
   return sp;
 }
 
-static CORE_ADDR
-ia64_push_return_address (CORE_ADDR pc, CORE_ADDR sp)
-{
-  CORE_ADDR global_pointer = FIND_GLOBAL_POINTER (pc);
-
-  if (global_pointer != 0)
-    write_register (IA64_GR1_REGNUM, global_pointer);
-
-  write_register (IA64_BR0_REGNUM, CALL_DUMMY_ADDRESS ());
-  return sp;
-}
-
 static void
 ia64_store_return_value (struct type *type, char *valbuf)
 {
@@ -2289,8 +2285,7 @@ ia64_gdbarch_init (struct gdbarch_info i
   set_gdbarch_write_pc (gdbarch, ia64_write_pc);
 
   /* Settings for calling functions in the inferior.  */
-  set_gdbarch_deprecated_push_arguments (gdbarch, ia64_push_arguments);
-  set_gdbarch_deprecated_push_return_address (gdbarch, ia64_push_return_address);
+  set_gdbarch_push_dummy_call (gdbarch, ia64_push_dummy_call);
   set_gdbarch_deprecated_pop_frame (gdbarch, ia64_pop_frame);
 
   set_gdbarch_deprecated_call_dummy_words (gdbarch, ia64_call_dummy_words);

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [commit] infcall.c cleanup - explict bp_addr variable

[Kevin Buettner]

On Jul 1,  7:44pm, Andreas Schwab wrote:

> 2003-07-01  Andreas Schwab  <schwab@suse.de>
> 
> 	* ia64-tdep.c (ia64_push_dummy_call): Define as combination of
> 	former ia64_push_arguments and ia64_push_return_address, and use
> 	regcache functions instead of read/write_register.
> 	(ia64_gdbarch_init): Set push_dummy_call instead of
> 	deprecated_push_arguments and deprecated_push_return_address.

Okay.  Thanks!

Kevin