[PATCH]: Don't use deprecated regcache functions

[PATCH]: Don't use deprecated regcache functions

[David S. Miller]

I just noticed the following while investigating a threading
failure on Linux/Sparc.

Any reason we were still using deprecated_read_register_gen()
here?

Otherwise, ok to apply?

2006-04-11  David S. Miller  <davem@sunset.davemloft.net>

	* linux-thread-db.c (thread_db_store_registers): Use
	regcache_raw_collect.

--- linux-thread-db.c.~1~	2006-03-30 08:34:23.000000000 -0800
+++ linux-thread-db.c	2006-04-11 01:30:58.000000000 -0700
@@ -1047,7 +1047,7 @@ thread_db_store_registers (int regno)
     {
       gdb_byte raw[MAX_REGISTER_SIZE];
 
-      deprecated_read_register_gen (regno, raw);
+      regcache_raw_collect (current_regcache, regno, raw);
       thread_db_fetch_registers (-1);
       regcache_raw_supply (current_regcache, regno, raw);
     }

Re: [PATCH]: Don't use deprecated regcache functions

[Daniel Jacobowitz]

On Tue, Apr 11, 2006 at 01:39:09AM -0700, David S. Miller wrote:
> 
> I just noticed the following while investigating a threading
> failure on Linux/Sparc.
> 
> Any reason we were still using deprecated_read_register_gen()
> here?

Surely not.

> Otherwise, ok to apply?

> -      deprecated_read_register_gen (regno, raw);
> +      regcache_raw_collect (current_regcache, regno, raw);
>        thread_db_fetch_registers (-1);
>        regcache_raw_supply (current_regcache, regno, raw);

I might be mistaken, but what the heck does the call to
thread_db_fetch_registers accomplish?  I think nothing.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH]: Don't use deprecated regcache functions

[David S. Miller]

From: Daniel Jacobowitz <drow@false.org>
Date: Tue, 11 Apr 2006 09:00:57 -0400

> On Tue, Apr 11, 2006 at 01:39:09AM -0700, David S. Miller wrote:
> > Otherwise, ok to apply?
> 
> > -      deprecated_read_register_gen (regno, raw);
> > +      regcache_raw_collect (current_regcache, regno, raw);
> >        thread_db_fetch_registers (-1);
> >        regcache_raw_supply (current_regcache, regno, raw);
> 
> I might be mistaken, but what the heck does the call to
> thread_db_fetch_registers accomplish?  I think nothing.

The solaris thread support does the same thing.  And that code
uses the more correct regcache_raw_collect() which is partly how
I noticed this.

What's happening here is that if we're only writing one register
we make sure to read in all the other registers first, then we
write the changing register back into the regcache.

This is interrelated with another piece of gdb code history I
was researching last night when I ran across this.  Several
platforms used to define CHILD_PREPARE_TO_STORE in order to
deal with interfaces, such as some ptrace() variants, that can
only set the whole set of registers at once.

There are some really nasty cases on Sparc for example, say you
want to write just the stack pointer.  We obtain the local and
in registers from the on-stack save area, so if we were not careful
just changing the stack or the frame pointer would cause all of
those other registers to change even though that is not what we
intended.  So to do it right, you have to read all the registers
into the regcache, then modify the stack or frame pointer.

This CHILD_PREPARE_TO_STORE macro would get invoked by the
target_prepare_to_store().  If you look at the regcache code it always
does a sequence like this:

  target_prepare_to_store ();
  memcpy (register_buffer (regcache, regnum), buf,
	  regcache->descr->sizeof_register[regnum]);
  regcache->register_valid_p[regnum] = 1;
  target_store_registers (regnum);

So, if necessary, target_prepare_to_store() would make sure the
regcache was fully read in if not already, then it would be safe to
write into the register cache.

But it seems that Mark Kettenis has been trying to transition
away from this deprecated scheme, and instead handle this issue
inside of the store register methods of the individual targets
and native support modules.

In fact, the only platform defining CHILD_PREPARE_TO_STORE is
GNU Hurd on x86, and probably that will eventually be eliminated
as well, and along with it target_prepare_to_store() and all
assosciated hooks.

In any event, the code we are discussing here in the Linux thread_db
code really is needed and it's related to the issues I've discussed
above.

So I think it's ok to apply this :-)

Re: [PATCH]: Don't use deprecated regcache functions

[Michael Snyder]

David S. Miller wrote:
> From: Daniel Jacobowitz <drow@false.org>
> Date: Tue, 11 Apr 2006 09:00:57 -0400
> 
> 
>>On Tue, Apr 11, 2006 at 01:39:09AM -0700, David S. Miller wrote:
>>
>>>Otherwise, ok to apply?
>>
>>>-      deprecated_read_register_gen (regno, raw);
>>>+      regcache_raw_collect (current_regcache, regno, raw);
>>>       thread_db_fetch_registers (-1);
>>>       regcache_raw_supply (current_regcache, regno, raw);
>>
>>I might be mistaken, but what the heck does the call to
>>thread_db_fetch_registers accomplish?  I think nothing.

Daniel, David's got the right of it.


> The solaris thread support does the same thing.  And that code
> uses the more correct regcache_raw_collect() which is partly how
> I noticed this.
> 
> What's happening here is that if we're only writing one register
> we make sure to read in all the other registers first, then we
> write the changing register back into the regcache.
> 
> This is interrelated with another piece of gdb code history I
> was researching last night when I ran across this.  Several
> platforms used to define CHILD_PREPARE_TO_STORE in order to
> deal with interfaces, such as some ptrace() variants, that can
> only set the whole set of registers at once.
> 
> There are some really nasty cases on Sparc for example, say you
> want to write just the stack pointer.  We obtain the local and
> in registers from the on-stack save area, so if we were not careful
> just changing the stack or the frame pointer would cause all of
> those other registers to change even though that is not what we
> intended.  So to do it right, you have to read all the registers
> into the regcache, then modify the stack or frame pointer.
> 
> This CHILD_PREPARE_TO_STORE macro would get invoked by the
> target_prepare_to_store().  If you look at the regcache code it always
> does a sequence like this:
> 
>   target_prepare_to_store ();
>   memcpy (register_buffer (regcache, regnum), buf,
> 	  regcache->descr->sizeof_register[regnum]);
>   regcache->register_valid_p[regnum] = 1;
>   target_store_registers (regnum);
> 
> So, if necessary, target_prepare_to_store() would make sure the
> regcache was fully read in if not already, then it would be safe to
> write into the register cache.
> 
> But it seems that Mark Kettenis has been trying to transition
> away from this deprecated scheme, and instead handle this issue
> inside of the store register methods of the individual targets
> and native support modules.
> 
> In fact, the only platform defining CHILD_PREPARE_TO_STORE is
> GNU Hurd on x86, and probably that will eventually be eliminated
> as well, and along with it target_prepare_to_store() and all
> assosciated hooks.
> 
> In any event, the code we are discussing here in the Linux thread_db
> code really is needed and it's related to the issues I've discussed
> above.
> 
> So I think it's ok to apply this :-)
> 

Re: [PATCH]: Don't use deprecated regcache functions

[David S. Miller]

From: Michael Snyder <msnyder@redhat.com>
Date: Wed, 12 Apr 2006 12:25:01 -0700

> David S. Miller wrote:
> > From: Daniel Jacobowitz <drow@false.org>
> > Date: Tue, 11 Apr 2006 09:00:57 -0400
> > 
> > 
> >>On Tue, Apr 11, 2006 at 01:39:09AM -0700, David S. Miller wrote:
> >>
> >>>Otherwise, ok to apply?
> >>
> >>>-      deprecated_read_register_gen (regno, raw);
> >>>+      regcache_raw_collect (current_regcache, regno, raw);
> >>>       thread_db_fetch_registers (-1);
> >>>       regcache_raw_supply (current_regcache, regno, raw);
> >>
> >>I might be mistaken, but what the heck does the call to
> >>thread_db_fetch_registers accomplish?  I think nothing.
> 
> Daniel, David's got the right of it.

Ok to apply? :-)

Re: [PATCH]: Don't use deprecated regcache functions

[Michael Snyder]

David S. Miller wrote:
> From: Michael Snyder <msnyder@redhat.com>
> Date: Wed, 12 Apr 2006 12:25:01 -0700
> 
> 
>>David S. Miller wrote:
>>
>>>From: Daniel Jacobowitz <drow@false.org>
>>>Date: Tue, 11 Apr 2006 09:00:57 -0400
>>>
>>>
>>>
>>>>On Tue, Apr 11, 2006 at 01:39:09AM -0700, David S. Miller wrote:
>>>>
>>>>
>>>>>Otherwise, ok to apply?
>>>>
>>>>>-      deprecated_read_register_gen (regno, raw);
>>>>>+      regcache_raw_collect (current_regcache, regno, raw);
>>>>>      thread_db_fetch_registers (-1);
>>>>>      regcache_raw_supply (current_regcache, regno, raw);
>>>>
>>>>I might be mistaken, but what the heck does the call to
>>>>thread_db_fetch_registers accomplish?  I think nothing.
>>
>>Daniel, David's got the right of it.
> 
> Ok to apply? :-)

Well, I meant your explanation as to why thread_db_fetch_registers
was used.  Now that I look at the actual change...

Why did you pick register_raw_collect as the replacement?
It's not semantically equivalent.  I would have thought
that the least-intrusive, most semantically-neutral change
would have been to use regcache_cooked_read.

That doesn't mean I think it's more or less correct...
I get confused just thinking about all these variants.
If you have an argument for why one is more correct in
this context, I'm certainly open to it.  I can't really
convince myself that the current behavior is correct,
since deprecated_read_register_gen calls regcache_cooked_read,
which may call regcache_raw_read, which may call
target_fetch_registers *before* capturing the current value.
Which it seems like would defeat the purpose of the whole
exercise...

Re: [PATCH]: Don't use deprecated regcache functions

[David S. Miller]

From: Michael Snyder <msnyder@redhat.com>
Date: Wed, 12 Apr 2006 19:41:42 -0700

> I get confused just thinking about all these variants.
> If you have an argument for why one is more correct in
> this context, I'm certainly open to it.  I can't really
> convince myself that the current behavior is correct,
> since deprecated_read_register_gen calls regcache_cooked_read,
> which may call regcache_raw_read, which may call
> target_fetch_registers *before* capturing the current value.
> Which it seems like would defeat the purpose of the whole
> exercise...

Any of these arguments for or against apply to the Solaris thread code
which does use regcache_raw_read() in this same exact situation.

Re: [PATCH]: Don't use deprecated regcache functions

[Michael Snyder]

David S. Miller wrote:
> From: Michael Snyder <msnyder@redhat.com>
> Date: Wed, 12 Apr 2006 19:41:42 -0700
> 
> 
>>I get confused just thinking about all these variants.
>>If you have an argument for why one is more correct in
>>this context, I'm certainly open to it.  I can't really
>>convince myself that the current behavior is correct,
>>since deprecated_read_register_gen calls regcache_cooked_read,
>>which may call regcache_raw_read, which may call
>>target_fetch_registers *before* capturing the current value.
>>Which it seems like would defeat the purpose of the whole
>>exercise...
> 
> 
> Any of these arguments for or against apply to the Solaris thread code
> which does use regcache_raw_read() in this same exact situation.
> 

Oh, I see -- you did mention that you were emulating
that code, but I didn't read thoroughly -- I thought
you were only referring to the fetch_registers call.

Well, I'm gonna plead ignorance, then.  I don't know enough
to say whether the change is correct or not.

Daniel?  Mark?

Re: [PATCH]: Don't use deprecated regcache functions

[Daniel Jacobowitz]

On Tue, Apr 11, 2006 at 02:13:34PM -0700, David S. Miller wrote:
> From: Daniel Jacobowitz <drow@false.org>
> Date: Tue, 11 Apr 2006 09:00:57 -0400
> 
> > On Tue, Apr 11, 2006 at 01:39:09AM -0700, David S. Miller wrote:
> > > Otherwise, ok to apply?
> > 
> > > -      deprecated_read_register_gen (regno, raw);
> > > +      regcache_raw_collect (current_regcache, regno, raw);
> > >        thread_db_fetch_registers (-1);
> > >        regcache_raw_supply (current_regcache, regno, raw);
> > 
> > I might be mistaken, but what the heck does the call to
> > thread_db_fetch_registers accomplish?  I think nothing.
> 
> The solaris thread support does the same thing.  And that code
> uses the more correct regcache_raw_collect() which is partly how
> I noticed this.
> 
> What's happening here is that if we're only writing one register
> we make sure to read in all the other registers first, then we
> write the changing register back into the regcache.

Well, yes.  Sorry for sending you off in the wrong direction; that's
not actually what I meant.  The bit that doesn't follow is:

> In any event, the code we are discussing here in the Linux thread_db
> code really is needed and it's related to the issues I've discussed
> above.

No, it really shouldn't be, for those same reasons.  But...

[long, mostly incorrect analysis deleted here.  This is confusing.]

[There's a lot more prepare-to-store methods than you
thought; while only one target defines the macro, many set it in the
target vector to_prepare_to_store.]

-      deprecated_read_register_gen (regno, raw);
+      regcache_raw_collect (current_regcache, regno, raw);
       thread_db_fetch_registers (-1);
       regcache_raw_supply (current_regcache, regno, raw);

fill_gregset (at least the example I checked, SPARC) will not read
registers as necessary - it also uses regcache_raw_collect.  So some
members of the gregset won't necessarily be filled in as valid unless
all registers are fetched first.  So that suggests the regno == -1 case
is bogus too.  Fortunately the lion's share of calls to these functions
come from regcache.c, which only ever writes one register at a time.

I think:

  - The calls to fill_gregset and fill_fpregset necessitate using
    target_fetch_registers (-1).  That's more or less the same as
    thread_db_fetch_registers (-1).  Either works.

  - What was previously unclear was that thread_db_fetch_registers
    may clobber already fetched registers.  How naughty.

  - It's totally unclear how the -1 case could work, since we can't
    fetch the registers without clobbering what we wanted to write
    out, which may explain why we previously didn't.  We'd need to
    preserve all the cached registers.  Another reason not to use
    the gregset functions this way.

What a mess.

Anyway, I guess the mess is long-standing and currently not a problem,
so go ahead and check in the change to use regcache_raw_collect.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH]: Don't use deprecated regcache functions

[David S. Miller]

From: Daniel Jacobowitz <drow@false.org>
Date: Thu, 20 Apr 2006 13:02:39 -0400

> Anyway, I guess the mess is long-standing and currently not a problem,
> so go ahead and check in the change to use regcache_raw_collect.

Ok, committed.