[rfa] Update PC without side effect in displaced stepping

[rfa] Update PC without side effect in displaced stepping

[Yao Qi]

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

During preparation of displaced stepping (in displaced_step_prepare),
regcache_write_pc is called to update PC to the address of copy area,
and gdbarch_write_pc is called subsequently.  However, gdbarch_write_pc
has some side effects besides updating PC values.

As far as I know on updating PC in displaced_step_prepare, what we need
here is to force program to execute one or some instructions in copy
area, and get the *same* effect of single-step one instruction on
original place, so we should update PC without any side effect.

Current approach may have some drawbacks in some cases.  For example, on
ARM, system library is compiled in Thumb mode, and application is
compiled in ARM mode.  The copy area for displaced stepping is in thumb
mode.  During displaced stepping, GDB copies that ARM instruction to
copy area, and using regcache_write_pc to update PC to the new address
of this instruction.  Due to the side effect of arm_write_pc, the T bit
is set in status register, so one 32-bit ARM instruction is interpreted
as two 16-bit thumb instructions by mistake.

This patch is to fix this problem.  Regression tested on x86_64-linux.
OK for mainline?

-- 
Yao (齐尧)

[-- Attachment #2: displaced_step_prepare.1220.patch --]
[-- Type: text/x-patch, Size: 836 bytes --]

2010-12-20  Yao Qi  <yao@codesourcery.com>

	* infrun.c (displaced_step_prepare): Replace regcache_write_pc by
	regcache_cooked_write_unsigned to update PC without side effect.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 1bc00a4..2711e19 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1304,8 +1304,13 @@ displaced_step_prepare (ptid_t ptid)
 
   make_cleanup (displaced_step_clear_cleanup, displaced);
 
-  /* Resume execution at the copy.  */
-  regcache_write_pc (regcache, copy);
+  /* Resume execution at the copy.  Update PC without any side effects.  */
+  if (gdbarch_pc_regnum (gdbarch) >= 0)
+    regcache_cooked_write_unsigned (regcache,
+				    gdbarch_pc_regnum (gdbarch), copy);
+  else
+    internal_error (__FILE__, __LINE__,
+		    _("displaced: Unable to update PC"));
 
   discard_cleanups (ignore_cleanups);
 

Re: [rfa] Update PC without side effect in displaced stepping

[Mark Kettenis]

> Date: Mon, 20 Dec 2010 15:50:18 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> During preparation of displaced stepping (in displaced_step_prepare),
> regcache_write_pc is called to update PC to the address of copy area,
> and gdbarch_write_pc is called subsequently.  However, gdbarch_write_pc
> has some side effects besides updating PC values.
> 
> As far as I know on updating PC in displaced_step_prepare, what we need
> here is to force program to execute one or some instructions in copy
> area, and get the *same* effect of single-step one instruction on
> original place, so we should update PC without any side effect.
> 
> Current approach may have some drawbacks in some cases.  For example, on
> ARM, system library is compiled in Thumb mode, and application is
> compiled in ARM mode.  The copy area for displaced stepping is in thumb
> mode.  During displaced stepping, GDB copies that ARM instruction to
> copy area, and using regcache_write_pc to update PC to the new address
> of this instruction.  Due to the side effect of arm_write_pc, the T bit
> is set in status register, so one 32-bit ARM instruction is interpreted
> as two 16-bit thumb instructions by mistake.
> 
> This patch is to fix this problem.  Regression tested on x86_64-linux.
> OK for mainline?

Sorry, no this isn't right.  On sparc and hppa for example, the
effects of write_pc() are needed here, since both the pc and the "next
pc" registers need to be updated to make sure all instructions in the
copy area get executed.

I think you'll have to make sure that if the displaced instructions
are Thumb instructions, the copy area gets properly marked as Thumb
such that write_pc() can do the right thing on arm as well.

Re: [rfa] Update PC without side effect in displaced stepping

[Yao Qi]

On 12/20/2010 04:04 PM, Mark Kettenis wrote:
> 
> Sorry, no this isn't right.  On sparc and hppa for example, the
> effects of write_pc() are needed here, since both the pc and the "next
> pc" registers need to be updated to make sure all instructions in the
> copy area get executed.
> 

I know very little on sparc and hppa.  Thanks for your explanation.

> I think you'll have to make sure that if the displaced instructions
> are Thumb instructions, the copy area gets properly marked as Thumb
> such that write_pc() can do the right thing on arm as well.

That works.  If we do so, given an address of an instruction, we have to
return the mode of original instruction in displaced stepping, and
return real mode there otherwise.  However, current GDB dose not provide
interfaces to target-dependent parts to query the state of displaced
stepping.  Shall we refactor infrun.c a little bit to move some code to
displace_step.[c|h], and expose them to target-dependent part?

-- 
Yao (齐尧)

Re: [rfa] Update PC without side effect in displaced stepping

[Yao Qi]

On 12/20/2010 09:42 PM, Yao Qi wrote:
> On 12/20/2010 04:04 PM, Mark Kettenis wrote:
>>
>> Sorry, no this isn't right.  On sparc and hppa for example, the
>> effects of write_pc() are needed here, since both the pc and the "next
>> pc" registers need to be updated to make sure all instructions in the
>> copy area get executed.
>>
> 
> I know very little on sparc and hppa.  Thanks for your explanation.

When writing the new patch, I re-consider this problem again.  GDB
doesn't support displaced stepping on sparc and hppa, so it is not
harmful to sparc and hppa when regcache_write_pc is replaced by
regcache_cooked_write_unsigned.
Currently, GDB supports displaced stepping on s390, rs6000 (including
ppc-linux, aix), i386, amd64 and arm.  AFAICS, this replacement in my
original patch is not harmful to these targets.

>> I think you'll have to make sure that if the displaced instructions
>> are Thumb instructions, the copy area gets properly marked as Thumb
>> such that write_pc() can do the right thing on arm as well.
> 
> That works.  If we do so, given an address of an instruction, we have to
> return the mode of original instruction in displaced stepping, and
> return real mode there otherwise.  However, current GDB dose not provide
> interfaces to target-dependent parts to query the state of displaced
> stepping.  Shall we refactor infrun.c a little bit to move some code to
> displace_step.[c|h], and expose them to target-dependent part?

My new patch is drafted in this way, but it looks not better than my
original one, because in my new patch, some state of displaced stepping
has to be exposed to arm-tdep.c in order to return the correct mode (ARM
or Thumb) according to the address of copy area.  Unless we make some
refactor to extract displaced stepping from infrun, this approach makes
code looks ugly.

Given my original patch is clean, and not harmful to existing targets
support displaced stepping, please consider my original patch again.
Comments on promising directions/approaches are welcome.

-- 
Yao (齐尧)

Re: [rfa] Update PC without side effect in displaced stepping

[Joel Brobecker]

> When writing the new patch, I re-consider this problem again.  GDB
> doesn't support displaced stepping on sparc and hppa, so it is not
> harmful to sparc and hppa when regcache_write_pc is replaced by
> regcache_cooked_write_unsigned.
> Currently, GDB supports displaced stepping on s390, rs6000 (including
> ppc-linux, aix), i386, amd64 and arm.  AFAICS, this replacement in my
> original patch is not harmful to these targets.
[...]
> Given my original patch is clean, and not harmful to existing targets
> support displaced stepping, please consider my original patch again.
> Comments on promising directions/approaches are welcome.

I haven't seen the patch, so I cannot comment specifically, but I think
that you are using the wrong reasons to try to justify your initial
patch.  It does not matter whether sparc or hppa support displaced
stepping or not. They might - it's not far-fetched for sparc, for
instance.  Or other platforms where it matters might be contributed
in the future, and they could need displaced stepping too.  By letting
your patch in, we would be making it harder for other platforms to
implement it.  It would feel like sweeping the dust under the carpet...

-- 
Joel

Re: [rfa] Update PC without side effect in displaced stepping

[Yao Qi]

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

On 12/23/2010 12:22 PM, Joel Brobecker wrote:
> I haven't seen the patch, so I cannot comment specifically, but I think
> that you are using the wrong reasons to try to justify your initial
> patch.  It does not matter whether sparc or hppa support displaced
> stepping or not. They might - it's not far-fetched for sparc, for
> instance.  Or other platforms where it matters might be contributed
> in the future, and they could need displaced stepping too.  By letting
> your patch in, we would be making it harder for other platforms to
> implement it.  It would feel like sweeping the dust under the carpet...

OK.  I have to try the second approach, which is 1) exposing displaced
stepping state to tdep, and 2) take displaced stepping state into
account when determining the mode.

Regression tested on armv7-unknown-linux-gnueabi.  Is it OK?

-- 
Yao (齐尧)

[-- Attachment #2: arm_displace_step_in_thumb_1223.patch --]
[-- Type: text/x-patch, Size: 4754 bytes --]

2010-12-23  Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c: (arm_pc_is_thumb):  Adjust MEMADDR if it is within
	copy area of displaced stepping.
	* infrun.c (struct displaced_step_inferior_state): Move to ...
	Expose get_displaced_stepping_state.
	* inferior.h: ... here.
	Declare get_displaced_stepping_state.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 636c1de..3227619 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -367,6 +367,22 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
   struct obj_section *sec;
   struct minimal_symbol *sym;
   char type;
+  struct displaced_step_inferior_state *displaced
+    = get_displaced_stepping_state (ptid_get_pid (inferior_ptid));
+
+  /* If checking the mode of displaced instruction in copy area, the mode
+     should be determined by instruction on the original address.  */
+
+  if (displaced && !ptid_equal (displaced->step_ptid, null_ptid)
+      && (displaced->step_copy == memaddr))
+    {
+      if (debug_displaced)
+	fprintf_unfiltered (gdb_stdlog,
+			    "displaced: check mode of %.8lx instead of %.8lx\n",
+			    (unsigned long) displaced->step_original,
+			    (unsigned long) memaddr);
+      memaddr = displaced->step_original;
+    }
 
   /* If bit 0 of the address is set, assume this is a Thumb address.  */
   if (IS_THUMB_ADDR (memaddr))
diff --git a/gdb/inferior.h b/gdb/inferior.h
index f80ecb5..7e68d3b 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -362,10 +362,46 @@ extern struct regcache *stop_registers;
 /* True if we are debugging displaced stepping.  */
 extern int debug_displaced;
 
+/* Per-inferior displaced stepping state.  */
+struct displaced_step_inferior_state
+{
+  /* Pointer to next in linked list.  */
+  struct displaced_step_inferior_state *next;
+
+  /* The process this displaced step state refers to.  */
+  int pid;
+
+  /* A queue of pending displaced stepping requests.  One entry per
+     thread that needs to do a displaced step.  */
+  struct displaced_step_request *step_request_queue;
+
+  /* If this is not null_ptid, this is the thread carrying out a
+     displaced single-step in process PID.  This thread's state will
+     require fixing up once it has completed its step.  */
+  ptid_t step_ptid;
+
+  /* The architecture the thread had when we stepped it.  */
+  struct gdbarch *step_gdbarch;
+
+  /* The closure provided gdbarch_displaced_step_copy_insn, to be used
+     for post-step cleanup.  */
+  struct displaced_step_closure *step_closure;
+
+  /* The address of the original instruction, and the copy we
+     made.  */
+  CORE_ADDR step_original, step_copy;
+
+  /* Saved contents of copy area.  */
+  gdb_byte *step_saved_copy;
+};
+
 /* Dump LEN bytes at BUF in hex to FILE, followed by a newline.  */
 void displaced_step_dump_bytes (struct ui_file *file,
                                 const gdb_byte *buf, size_t len);
 
+
+struct displaced_step_inferior_state *get_displaced_stepping_state (int pid);
+
 \f
 /* Possible values for gdbarch_call_dummy_location.  */
 #define ON_STACK 1
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 1bc00a4..d943dd3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -998,46 +998,13 @@ struct displaced_step_request
   struct displaced_step_request *next;
 };
 
-/* Per-inferior displaced stepping state.  */
-struct displaced_step_inferior_state
-{
-  /* Pointer to next in linked list.  */
-  struct displaced_step_inferior_state *next;
-
-  /* The process this displaced step state refers to.  */
-  int pid;
-
-  /* A queue of pending displaced stepping requests.  One entry per
-     thread that needs to do a displaced step.  */
-  struct displaced_step_request *step_request_queue;
-
-  /* If this is not null_ptid, this is the thread carrying out a
-     displaced single-step in process PID.  This thread's state will
-     require fixing up once it has completed its step.  */
-  ptid_t step_ptid;
-
-  /* The architecture the thread had when we stepped it.  */
-  struct gdbarch *step_gdbarch;
-
-  /* The closure provided gdbarch_displaced_step_copy_insn, to be used
-     for post-step cleanup.  */
-  struct displaced_step_closure *step_closure;
-
-  /* The address of the original instruction, and the copy we
-     made.  */
-  CORE_ADDR step_original, step_copy;
-
-  /* Saved contents of copy area.  */
-  gdb_byte *step_saved_copy;
-};
-
 /* The list of states of processes involved in displaced stepping
    presently.  */
 static struct displaced_step_inferior_state *displaced_step_inferior_states;
 
 /* Get the displaced stepping state of process PID.  */
 
-static struct displaced_step_inferior_state *
+struct displaced_step_inferior_state *
 get_displaced_stepping_state (int pid)
 {
   struct displaced_step_inferior_state *state;

Re: [rfa] Update PC without side effect in displaced stepping

[Mark Kettenis]

> Date: Thu, 23 Dec 2010 08:22:36 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > When writing the new patch, I re-consider this problem again.  GDB
> > doesn't support displaced stepping on sparc and hppa, so it is not
> > harmful to sparc and hppa when regcache_write_pc is replaced by
> > regcache_cooked_write_unsigned.
> > Currently, GDB supports displaced stepping on s390, rs6000 (including
> > ppc-linux, aix), i386, amd64 and arm.  AFAICS, this replacement in my
> > original patch is not harmful to these targets.
> [...]
> > Given my original patch is clean, and not harmful to existing targets
> > support displaced stepping, please consider my original patch again.
> > Comments on promising directions/approaches are welcome.
> 
> I haven't seen the patch, so I cannot comment specifically, but I think
> that you are using the wrong reasons to try to justify your initial
> patch.  It does not matter whether sparc or hppa support displaced
> stepping or not. They might - it's not far-fetched for sparc, for
> instance.  Or other platforms where it matters might be contributed
> in the future, and they could need displaced stepping too.  By letting
> your patch in, we would be making it harder for other platforms to
> implement it.  It would feel like sweeping the dust under the carpet...

I have the same feeling.

[PING : rfa] Update PC without side effect in displaced stepping

[Yao Qi]

On 12/23/2010 03:00 PM, Yao Qi wrote:
> OK.  I have to try the second approach, which is 1) exposing displaced
> stepping state to tdep, and 2) take displaced stepping state into
> account when determining the mode.

Ping.
http://sourceware.org/ml/gdb-patches/2010-12/msg00427.html

2010-12-23  Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c: (arm_pc_is_thumb):  Adjust MEMADDR if it is within
	copy area of displaced stepping.
	* infrun.c (struct displaced_step_inferior_state): Move to ...
	Expose get_displaced_stepping_state.
	* inferior.h: ... here.
	Declare get_displaced_stepping_state.

P.S.  I am thinking that it may be a good idea to move code for
displaced stepping to a separate file, say displaced_step.[ch].  Comments?

-- 
Yao (齐尧)

[try 3rd] arm_pc_is_thumb takes displaced stepping into account

[Yao Qi]

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

On 12/23/2010 01:00 AM, Yao Qi wrote:
> OK.  I have to try the second approach, which is 1) exposing displaced
> stepping state to tdep, and 2) take displaced stepping state into
> account when determining the mode.
>

After talked with Ulrich, I realize that it is *not* a good idea to 
expose displaced stepping outside of infrun, and my patch is a little 
bit too intrusive.

> 2010-12-23  Yao Qi<yao@codesourcery.com>
>
> 	* arm-tdep.c: (arm_pc_is_thumb):  Adjust MEMADDR if it is within
> 	copy area of displaced stepping.
> 	* infrun.c (struct displaced_step_inferior_state): Move to ...
> 	Expose get_displaced_stepping_state.
> 	* inferior.h: ... here.
> 	Declare get_displaced_stepping_state.

This time, instead of exposing displaced_step_inferior_state to tdep, we 
return displaced_step_closure, which is defined by each tdep, instance 
to tdep appropriately.

OK to mainline?

-- 
Yao Qi

[-- Attachment #2: arm_displace_step_in_thumb_0111.patch --]
[-- Type: text/x-patch, Size: 2445 bytes --]

gdb/
        * infrun.c (get_displaced_step_closure_by_addr): New.
        * inferior.h: Declare it.
        * arm-tdep.c: (arm_pc_is_thumb): Call
	get_displaced_step_closure_by_addr.  Adjust MEMADDR if it
	returns non-NULL.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index ef4d9f3..fb080c1 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -368,6 +368,20 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
   struct obj_section *sec;
   struct minimal_symbol *sym;
   char type;
+  struct displaced_step_closure* dsc
+    = get_displaced_step_closure_by_addr(memaddr);
+
+  /* If checking the mode of displaced instruction in copy area, the mode
+     should be determined by instruction on the original address.  */
+  if (dsc)
+    {
+      if (debug_displaced)
+	fprintf_unfiltered (gdb_stdlog,
+			    "displaced: check mode of %.8lx instead of %.8lx\n",
+			    (unsigned long) dsc->insn_addr,
+			    (unsigned long) memaddr);
+      memaddr = dsc->insn_addr;
+    }
 
   /* If bit 0 of the address is set, assume this is a Thumb address.  */
   if (IS_THUMB_ADDR (memaddr))
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 7052d6f..a319847 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -366,6 +366,8 @@ extern int debug_displaced;
 void displaced_step_dump_bytes (struct ui_file *file,
                                 const gdb_byte *buf, size_t len);
 
+struct displaced_step_closure*
+get_displaced_step_closure_by_addr (CORE_ADDR addr);
 \f
 /* Possible values for gdbarch_call_dummy_location.  */
 #define ON_STACK 1
diff --git a/gdb/infrun.c b/gdb/infrun.c
index dd6fe6c..0714308 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1078,6 +1078,26 @@ add_displaced_stepping_state (int pid)
   return state;
 }
 
+/* If inferior is in displaced stepping, and ADDR equals to starting address
+   of copy area, return corresponding displaced_step_closure.  Otherwise,
+   return NULL.  */
+
+struct displaced_step_closure*
+get_displaced_step_closure_by_addr (CORE_ADDR addr)
+{
+  struct displaced_step_inferior_state *displaced
+    = get_displaced_stepping_state (ptid_get_pid (inferior_ptid));
+
+  /* If checking the mode of displaced instruction in copy area.  */
+  if (displaced && !ptid_equal (displaced->step_ptid, null_ptid)
+     && (displaced->step_copy == addr))
+    return displaced->step_closure;
+
+  return NULL;
+}
+
 /* Remove the displaced stepping state of process PID.  */
 
 static void

Re: [try 3rd] arm_pc_is_thumb takes displaced stepping into account

[Matthew Gretton-Dann]

On Tue, 2011-01-11 at 22:02 -0600, Yao Qi wrote:
> On 12/23/2010 01:00 AM, Yao Qi wrote:
> > OK.  I have to try the second approach, which is 1) exposing displaced
> > stepping state to tdep, and 2) take displaced stepping state into
> > account when determining the mode.
> >
> 
> After talked with Ulrich, I realize that it is *not* a good idea to 
> expose displaced stepping outside of infrun, and my patch is a little 
> bit too intrusive.
> 
> > 2010-12-23  Yao Qi<yao@codesourcery.com>
> >
> > 	* arm-tdep.c: (arm_pc_is_thumb):  Adjust MEMADDR if it is within
> > 	copy area of displaced stepping.
> > 	* infrun.c (struct displaced_step_inferior_state): Move to ...
> > 	Expose get_displaced_stepping_state.
> > 	* inferior.h: ... here.
> > 	Declare get_displaced_stepping_state.
> 
> This time, instead of exposing displaced_step_inferior_state to tdep, we 
> return displaced_step_closure, which is defined by each tdep, instance 
> to tdep appropriately.
> 
> OK to mainline?

I agree with Ulrich, and prefer this latest patch to your previous ones
- but I am still not 100% happy with it, as I think it would be better
not to need anything from infrun.c at all here.

What could be done instead is to have the displaced stepping routines
maintain a list of the areas of memory that are being used as scratch
space, and the instruction set state of the instructions in those areas.

Then arm_pc_is_thumb should check this list to see if the PC falls into
one of these areas, and return the appropriate value.

This keeps these changes wholly within the ARM backend for GDB, and
doesn't require changes to GDB globally.

However, this will impact performance and memory more than your patch -
so I am not completely opposed to your patch.

This is yet another instance of a problem Ulrich and I discussed last
year, which is: How do you tell what instruction-set state you are in
when you just have a CORE_ADDR, which is insufficient information on
ARM.  See the thread starting at:
http://sourceware.org/ml/gdb-patches/2010-08/msg00271.html

The long-term solution is probably either to implement multiple address
spaces for code, or to treat ARM & Thumb as different gdbarch's and use
multiarch support.  However, this is a much more difficult problem to
solve in general and shouldn't stop this patch going in.

[Note again that I am not a maintainer and so do not have approval
rights].

Thanks,

Matt

-- 
Matthew Gretton-Dann
Principal Engineer - PDSW Tools
ARM Ltd

Re: [try 3rd] arm_pc_is_thumb takes displaced stepping into account

[Yao Qi]

On 01/13/2011 09:15 AM, Matthew Gretton-Dann wrote:
> I agree with Ulrich, and prefer this latest patch to your previous ones
> - but I am still not 100% happy with it, as I think it would be better
> not to need anything from infrun.c at all here.

Agree.  The perfect situation is that we need nothing from infrun.c in tdep.

> What could be done instead is to have the displaced stepping routines
> maintain a list of the areas of memory that are being used as scratch
> space, and the instruction set state of the instructions in those areas.

> Then arm_pc_is_thumb should check this list to see if the PC falls into
> one of these areas, and return the appropriate value.

That sounds good.  Again, thanks for your inputs.

-- 
Yao Qi

[Ping 1: try 3rd] arm_pc_is_thumb takes displaced stepping into account

[Yao Qi]

On 01/11/2011 09:02 PM, Yao Qi wrote:

> This time, instead of exposing displaced_step_inferior_state to tdep, we
> return displaced_step_closure, which is defined by each tdep, instance
> to tdep appropriately.
>
> OK to mainline?
>
Ping.
http://sourceware.org/ml/gdb-patches/2011-01/msg00247.html

gdb/
         * infrun.c (get_displaced_step_closure_by_addr): New.
         * inferior.h: Declare it.
         * arm-tdep.c: (arm_pc_is_thumb): Call
	get_displaced_step_closure_by_addr.  Adjust MEMADDR if it
	returns non-NULL.

-- 
Yao Qi

[Ping 2: try 3rd] arm_pc_is_thumb takes displaced stepping into account

[Yao Qi]

On 01/20/2011 12:00 AM, Yao Qi wrote:
> On 01/11/2011 09:02 PM, Yao Qi wrote:
> 
>> This time, instead of exposing displaced_step_inferior_state to tdep, we
>> return displaced_step_closure, which is defined by each tdep, instance
>> to tdep appropriately.
>>
>> OK to mainline?
>>
> Ping.
> http://sourceware.org/ml/gdb-patches/2011-01/msg00247.html
> 
> gdb/
>         * infrun.c (get_displaced_step_closure_by_addr): New.
>         * inferior.h: Declare it.
>         * arm-tdep.c: (arm_pc_is_thumb): Call
>     get_displaced_step_closure_by_addr.  Adjust MEMADDR if it
>     returns non-NULL.
> 

Ping.

-- 
Yao (齐尧)

Re: [try 3rd] arm_pc_is_thumb takes displaced stepping into account

[Ulrich Weigand]

Yao Qi wrote:

> This time, instead of exposing displaced_step_inferior_state to tdep, we 
> return displaced_step_closure, which is defined by each tdep, instance 
> to tdep appropriately.

Hmmm.  I see one problem with this approach:

> +/* If inferior is in displaced stepping, and ADDR equals to starting address
> +   of copy area, return corresponding displaced_step_closure.  Otherwise,
> +   return NULL.  */

What if the copy area contains more than a single instruction, and we
need to find out the mode of the PC corresponding to the second of them?

Your get_displaced_step_closure_by_addr routine would not find the
closure in this case, because the PC doesn't match the start address.

I had originally suggested to create a function that would recognize
any PC within the copy area.  But it seems this cannot be implemented
in common code, since common code does not actually know the length
of the copy area.

Matthew suggested to keep another list of copy areas in target-dependent
code instead.  While I don't really like to keep duplicated data, it
would appear that this approach is able to handle the above situation.

Bye,
Ulrich

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

Re: [try 3rd] arm_pc_is_thumb takes displaced stepping into account

[Yao Qi]

On 01/31/2011 11:38 PM, Ulrich Weigand wrote:
> What if the copy area contains more than a single instruction, and we
> need to find out the mode of the PC corresponding to the second of them?
> 

This approach can handle multiple instructions in copy area, because
during preparation of displaced stepping (in displaced_step_prepare),
regcache_write_pc is called to update PC to the *address of copy area*,
no matter how many instructions in copy area.

Per my limited knowledge on GDB, I can't find a case that PC is set
somewhere of copy area other than the beginning of copy area during
displaced stepping.  That is to say, we don't have to know the mode of
the 2nd instruction.

Actually, most of the time, copy area contains more than one
instructions, and test suite works well.  This can also prove that this
approach works.

-- 
Yao (齐尧)

Re: [try 3rd] arm_pc_is_thumb takes displaced stepping into account

[Ulrich Weigand]

Yao Qi wrote:
> On 01/31/2011 11:38 PM, Ulrich Weigand wrote:
> > What if the copy area contains more than a single instruction, and we
> > need to find out the mode of the PC corresponding to the second of them?
> > 
> 
> This approach can handle multiple instructions in copy area, because
> during preparation of displaced stepping (in displaced_step_prepare),
> regcache_write_pc is called to update PC to the *address of copy area*,
> no matter how many instructions in copy area.
> 
> Per my limited knowledge on GDB, I can't find a case that PC is set
> somewhere of copy area other than the beginning of copy area during
> displaced stepping.  That is to say, we don't have to know the mode of
> the 2nd instruction.

Hmm, I guess you're right ... GDB does "hide" the fact that it has
replaced the original instruction, and you should never end up in
the middle of a displaced stepping sequence.

The patch is OK, except for a couple of formatting issues:

+  struct displaced_step_closure* dsc
+    = get_displaced_step_closure_by_addr(memaddr);

Please use
   struct displaced_step_closure *dsc
(i.e. the * next to the variable, not the type).

+struct displaced_step_closure*
+get_displaced_step_closure_by_addr (CORE_ADDR addr);

Here as well, there should be a space between the type and the *.

Also, please do not start the function name on the next line
in the header file; "grep ^get_displaced_step_closure_by_addr *"
should find only the definition, not the declaration.

Thanks,
Ulrich

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