[PATCH] Don't attach to 'target_changed' observer in regcache

[PATCH] Don't attach to 'target_changed' observer in regcache

[Yao Qi]

Hi,
When I am modifying 'target_changed' observer, I don't understand
why we attach regcache_observer_target_changed to 'target_changed'
observer, and invalidates all regcache.  'target_changed' observer
is notified in valops.c:value_assign, before that, register is
modified by calling either gdbarch_value_to_register or
put_frame_register_bytes.  Looks like regcache is in a good state,
why do we have to invalidate them?

The code that invalidates all regcache was added by this patch,

  Multiplexed registers and invalidating the register cache
  http://sourceware.org/ml/gdb-patches/2004-04/msg00282.html

The author (Orjan) tried to support "changing the bank select register
changes the contents (and meaning) for a whole set of other registers."
The requirement is quite specific to Orjan's own port, so the better
solution is to attach a function which invalidates all regcache in
Orjan's backend, instead of doing it in target-independent part.

With this patch, I can see that the number of regcache allocation/release
is reduced (16 -> 15) in test case (mi-cli.exp).  For a large
multi-threaded app, we'll get better gain.

Regression tested on x86_64-linux native and gdbserver.  OK for HEAD?

gdb:

2012-08-02  Yao Qi  <yao@codesourcery.com>

	* regcache.c (regcache_observer_target_changed): Remove.
	(_initialize_regcache): Don't call observer_attach_target_changed.
---
 gdb/regcache.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index c716280..3f83cfa 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -512,15 +512,6 @@ get_current_regcache (void)
   return get_thread_regcache (inferior_ptid);
 }
 
-
-/* Observer for the target_changed event.  */
-
-static void
-regcache_observer_target_changed (struct target_ops *target)
-{
-  registers_changed ();
-}
-
 /* Update global variables old ptids to hold NEW_PTID if they were
    holding OLD_PTID.  */
 static void
@@ -1405,7 +1396,6 @@ _initialize_regcache (void)
   regcache_descr_handle
     = gdbarch_data_register_post_init (init_regcache_descr);
 
-  observer_attach_target_changed (regcache_observer_target_changed);
   observer_attach_thread_ptid_changed (regcache_thread_ptid_changed);
 
   add_com ("flushregs", class_maintenance, reg_flush_command,
-- 
1.7.7.6

Re: [PATCH] Don't attach to 'target_changed' observer in regcache

[Pedro Alves]

On 08/02/2012 08:17 AM, Yao Qi wrote:
> Hi,
> When I am modifying 'target_changed' observer, I don't understand
> why we attach regcache_observer_target_changed to 'target_changed'
> observer, and invalidates all regcache.  'target_changed' observer
> is notified in valops.c:value_assign, before that, register is
> modified by calling either gdbarch_value_to_register or
> put_frame_register_bytes.  Looks like regcache is in a good state,
> why do we have to invalidate them?
> 
> The code that invalidates all regcache was added by this patch,
> 
>   Multiplexed registers and invalidating the register cache
>   http://sourceware.org/ml/gdb-patches/2004-04/msg00282.html
> 
> The author (Orjan) tried to support "changing the bank select register
> changes the contents (and meaning) for a whole set of other registers."
> The requirement is quite specific to Orjan's own port, so the better
> solution is to attach a function which invalidates all regcache in
> Orjan's backend, instead of doing it in target-independent part.

Really not sure about this.  There were also comments from Dan and Cagney
on that thread, that point at need for this being not that uncommon.
E.g. Dan wrote:

> I've been meaning to do this for a long time.  For instance, there is a
> writeable register on PowerPC targets which has some read-only bits.
> Right now, if you set it to an arbitrary value and then print it you'll
> get the value GDB wrote - not the value that was actually accepted into
> the register.
>
> Andrew convinced me that the performance cost associated with this
> would be small in practice.

I think you get to argue against the whole picture, not just Orjan's
port, although I think we'd also need a plan to address the original
issue in some other way.  E.g., I can picture that gdb might not even
have any knowledge of such registers (so no way to hardcode when-to-flush
in the backend), as they may have been included as part of a target
description, in addition to core registers.

-- 
Pedro Alves

Re: [PATCH] Don't attach to 'target_changed' observer in regcache

[Yao Qi]

On Thursday, August 02, 2012 03:46:13 PM Pedro Alves wrote:
> Really not sure about this.  There were also comments from Dan and Cagney
> on that thread, that point at need for this being not that uncommon.
> 
> E.g. Dan wrote:
> > I've been meaning to do this for a long time.  For instance, there is a
> > writeable register on PowerPC targets which has some read-only bits.
> > Right now, if you set it to an arbitrary value and then print it you'll
> > get the value GDB wrote - not the value that was actually accepted into
> > the register.
> > 
> > Andrew convinced me that the performance cost associated with this
> > would be small in practice.
> 

I red Dan's comment, but I thought it is uncommon :)

> I think you get to argue against the whole picture, not just Orjan's
> port, although I think we'd also need a plan to address the original
> issue in some other way.  E.g., I can picture that gdb might not even
> have any knowledge of such registers (so no way to hardcode when-to-flush
> in the backend), as they may have been included as part of a target
> description, in addition to core registers.

It is a good idea to extend target description for the attributes of each 
register, for example, a certain bit of a register is read-only.  However, 
target description can't handle Orjan's requirement (changing the bank select 
register changes the contents for a whole set of other registers.).

My original thought is to extend observer 'target_changed' to pass more 
parameters, such as 'register number', and backend registers its own hook to 
'target_changed' observer, and takes right actions for the "interesting" 
register changes.  Maybe we can do this via gdbarch hook.

THe problem is clear, but its description is not very precise.  I don't have a 
solid plan so far.

-- 
Yao (齐尧)

Re: [PATCH] Don't attach to 'target_changed' observer in regcache

[Mark Kettenis]

> From: Yao Qi <yao@codesourcery.com>
> Date: Thu, 2 Aug 2012 15:17:27 +0800
> 
> Hi,
> When I am modifying 'target_changed' observer, I don't understand
> why we attach regcache_observer_target_changed to 'target_changed'
> observer, and invalidates all regcache.  'target_changed' observer
> is notified in valops.c:value_assign, before that, register is
> modified by calling either gdbarch_value_to_register or
> put_frame_register_bytes.  Looks like regcache is in a good state,
> why do we have to invalidate them?
> 
> The code that invalidates all regcache was added by this patch,
> 
>   Multiplexed registers and invalidating the register cache
>   http://sourceware.org/ml/gdb-patches/2004-04/msg00282.html
> 
> The author (Orjan) tried to support "changing the bank select register
> changes the contents (and meaning) for a whole set of other registers."
> The requirement is quite specific to Orjan's own port, so the better
> solution is to attach a function which invalidates all regcache in
> Orjan's backend, instead of doing it in target-independent part.

Banked registers aren't really that exotic.  Especially if you realise
that register windows (SPARC, IA-64) are essentially banked registers.

I fear you're trading speed for correctness here.
> 
> 2012-08-02  Yao Qi  <yao@codesourcery.com>
> 
> 	* regcache.c (regcache_observer_target_changed): Remove.
> 	(_initialize_regcache): Don't call observer_attach_target_changed.

Re: [PATCH] Don't attach to 'target_changed' observer in regcache

[Yao Qi]

On Wednesday, August 08, 2012 07:53:40 PM Mark Kettenis wrote:
> > The author (Orjan) tried to support "changing the bank select register
> > changes the contents (and meaning) for a whole set of other registers."
> > The requirement is quite specific to Orjan's own port, so the better
> > solution is to attach a function which invalidates all regcache in
> > Orjan's backend, instead of doing it in target-independent part.
> 
> Banked registers aren't really that exotic.  Especially if you realise
> that register windows (SPARC, IA-64) are essentially banked registers.
> 

Banked register doesn't matter here.  "Changing one special register changes 
the whole set of registers", which is exotic, matters.  Are they (banked 
registers) switched by modifying a certain register?  Per my few knowledge on 
SPARC and IA-64, the answer is "No".

The issues of this patch are 1) modifying read-only bits in writable register 
(as Dan pointed out), 2) troubles to Orjan's port.  I can't see any other 
issues of this patch.

-- 
Yao (齐尧)

Re: [PATCH] Don't attach to 'target_changed' observer in regcache

[Mark Kettenis]

> From: Yao Qi <yao@codesourcery.com>
> Date: Thu, 9 Aug 2012 11:10:16 +0800
> 
> On Wednesday, August 08, 2012 07:53:40 PM Mark Kettenis wrote:
> > > The author (Orjan) tried to support "changing the bank select register
> > > changes the contents (and meaning) for a whole set of other registers."
> > > The requirement is quite specific to Orjan's own port, so the better
> > > solution is to attach a function which invalidates all regcache in
> > > Orjan's backend, instead of doing it in target-independent part.
> > 
> > Banked registers aren't really that exotic.  Especially if you realise
> > that register windows (SPARC, IA-64) are essentially banked registers.
> > 
> 
> Banked register doesn't matter here.  "Changing one special register
> changes the whole set of registers", which is exotic, matters.  Are
> they (banked registers) switched by modifying a certain register?
> Per my few knowledge on SPARC and IA-64, the answer is "No".

Banked registers are by defenition switched by modifying a certain
register I'd say.  And yes, for SPARC by changing %cwp will change the
visible register window and therefore the contents of the %i0-7, %l0-7
and %o0-7 registers.  And changing some bits in the %pstate register
will switch between sets of alternate global registers (%g0-7).  It's
been a while since I last looked at IA-64, but I'm pretty sure it has
some similar mechanisms.  Thinking about this a bit more, PA-RISC is
another architecture that has such a mechanism.  Some of its registers
are shadowed and you can switch between the shadowed and non-shadowed
by modifying a bit in the %psw register.

> The issues of this patch are 1) modifying read-only bits in writable
> register (as Dan pointed out), 2) troubles to Orjan's port.  I can't
> see any other issues of this patch.

And I'm telling you that the "troubles with Orjan's port" are more
general than you think they are.

Re: [PATCH] Don't attach to 'target_changed' observer in regcache

[Yao Qi]

On Thursday, August 09, 2012 10:11:37 AM Mark Kettenis wrote:
> > Banked register doesn't matter here.  "Changing one special register
> > changes the whole set of registers", which is exotic, matters.  Are
> > they (banked registers) switched by modifying a certain register?
> > Per my few knowledge on SPARC and IA-64, the answer is "No".
> 
> Banked registers are by defenition switched by modifying a certain
> register I'd say.  And yes, for SPARC by changing %cwp will change the
> visible register window and therefore the contents of the %i0-7, %l0-7
> and %o0-7 registers.  And changing some bits in the %pstate register
> will switch between sets of alternate global registers (%g0-7).  It's
> been a while since I last looked at IA-64, but I'm pretty sure it has
> some similar mechanisms.  Thinking about this a bit more, PA-RISC is
> another architecture that has such a mechanism.  Some of its registers
> are shadowed and you can switch between the shadowed and non-shadowed
> by modifying a bit in the %psw register.
> 

Mark,
thanks for your explanation.  I must use the wrong key words in google, and 
got less interesting search result than your answer here.

I am thinking that we may add a new gdbarch hook function 'register_changed 
(int regnum)'.  In default, it invalidates all regcaches, and backend can 
override it to do something optimally.  At least, we don't have to invalidate 
all regcache on some ports, such as x86, tic6x.

> > The issues of this patch are 1) modifying read-only bits in writable
> > register (as Dan pointed out), 2) troubles to Orjan's port.  I can't
> > see any other issues of this patch.
> 
> And I'm telling you that the "troubles with Orjan's port" are more
> general than you think they are.

Agreed.

-- 
Yao (齐尧)

Re: [PATCH] Don't attach to 'target_changed' observer in regcache

[Pedro Alves]

On 08/09/2012 09:36 AM, Yao Qi wrote:
> I am thinking that we may add a new gdbarch hook function 'register_changed 
> (int regnum)'.  In default, it invalidates all regcaches, and backend can 
> override it to do something optimally.  At least, we don't have to invalidate 
> all regcache on some ports, such as x86, tic6x.

gdbarch is only useful for the cases gdb can hardcode.  it is not
sufficient in the cases the target feeds gdb a target description with
more registers than gdb knows the core architecture of the running
target has.

-- 
Pedro Alves