[PATCH] Remove MAX_REGISTER_SIZE from sol-thread.c

[PATCH] Remove MAX_REGISTER_SIZE from sol-thread.c

[Alan Hayward]

Regcache calls in sol-thread.c are bogus and do nothing.
The code in between will not change or update regcache.
Removed.

Ok to commit?

Alan.

2017-02-24  Alan Hayward  <alan.hayward@arm.com>

	* sol-thread.c (sol_thread_store_registers): Remove regcache calls.


diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
index a09a3ab9a8bc56f367e3ba1537f5674f0a7f491f..06b146c314a10be0e360a7242bab229ced1c00b1 100644
--- a/gdb/sol-thread.c
+++ b/gdb/sol-thread.c
@@ -534,12 +534,6 @@ sol_thread_store_registers (struct target_ops *ops,

   if (regnum != -1)
     {
-      /* Not writing all the registers.  */
-      char old_value[MAX_REGISTER_SIZE];
-
-      /* Save new register value.  */
-      regcache_raw_collect (regcache, regnum, old_value);
-
       val = p_td_thr_getgregs (&thandle, gregset);
       if (val != TD_OK)
 	error (_("sol_thread_store_registers: td_thr_getgregs %s"),
@@ -548,9 +542,6 @@ sol_thread_store_registers (struct target_ops *ops,
       if (val != TD_OK)
 	error (_("sol_thread_store_registers: td_thr_getfpregs %s"),
 	       td_err_string (val));
-
-      /* Restore new register value.  */
-      regcache_raw_supply (regcache, regnum, old_value);
     }

   fill_gregset (regcache, (gdb_gregset_t *) &gregset, regnum);

Re: [PATCH] Remove MAX_REGISTER_SIZE from sol-thread.c

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> Regcache calls in sol-thread.c are bogus and do nothing.
> The code in between will not change or update regcache.
> Removed.

If we remove code, we need to figure out why the code was there.
The code is about to remove was added by

commit 7cdd6cac82faad2083029b2ac014d44d869f76c0
Author: Frank Ch. Eigler <fche@redhat.com>
Date:   Thu Apr 2 18:54:04 1998 +0000

    * Fixes for PR 14571.
    
    Thu Apr  2 12:47:41 1998  Frank Ch. Eigler  <fche@cygnus.com>
    
        * sol-thread.c (sol_thread_store_registers): Save & restore new
        value of single updated register to prevent accidental clobbering.

and there was a global buffer "registers".  However, the global buffer
no longer exists in current GDB, so the code is no longer needed, IMO.

Hi, Frank, do you remember how does your patch (above) fix the problem
you described in the ChangeLog?  I don't see how p_td_thr_getgregs and
p_td_thr_getfpregs clobber the global buffer 'registers'.

> diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
> index a09a3ab9a8bc56f367e3ba1537f5674f0a7f491f..06b146c314a10be0e360a7242bab229ced1c00b1 100644
> --- a/gdb/sol-thread.c
> +++ b/gdb/sol-thread.c
> @@ -534,12 +534,6 @@ sol_thread_store_registers (struct target_ops *ops,
>
>    if (regnum != -1)
>      {
> -      /* Not writing all the registers.  */
> -      char old_value[MAX_REGISTER_SIZE];
> -
> -      /* Save new register value.  */
> -      regcache_raw_collect (regcache, regnum, old_value);
> -
>        val = p_td_thr_getgregs (&thandle, gregset);
>        if (val != TD_OK)
>  	error (_("sol_thread_store_registers: td_thr_getgregs %s"),
> @@ -548,9 +542,6 @@ sol_thread_store_registers (struct target_ops *ops,
>        if (val != TD_OK)
>  	error (_("sol_thread_store_registers: td_thr_getfpregs %s"),
>  	       td_err_string (val));
> -
> -      /* Restore new register value.  */
> -      regcache_raw_supply (regcache, regnum, old_value);

-- 
Yao (齐尧)

Re: [PATCH] Remove MAX_REGISTER_SIZE from sol-thread.c

[Frank Ch. Eigler]

Hi -

> 
> > Regcache calls in sol-thread.c are bogus and do nothing.
> > The code in between will not change or update regcache.
> > Removed.
> 
> If we remove code, we need to figure out why the code was there.
> The code is about to remove was added by
> 
> commit 7cdd6cac82faad2083029b2ac014d44d869f76c0
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date:   Thu Apr 2 18:54:04 1998 +0000
> 
>     * Fixes for PR 14571.
> [...]

Heh, sorry, my recollection of this after 19 years is totally blank.

Interestingly, PR14571 must refer to the pre-bugzilla bug tracker
system.  There was one on sourceware.org, but its web interface is no
longer operating, and scraps of the database files that are still
archived seem not to go up to 14571.  Likewise, no google hits.

So I'm at a loss, can't find orginal supporting data either, and
that's a bummer.

- FChE

Re: [PATCH] Remove MAX_REGISTER_SIZE from sol-thread.c

[Alan Hayward]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1908 bytes --]


> On 1 Mar 2017, at 17:24, Frank Ch. Eigler <fche@redhat.com> wrote:
> 
> Hi -
> 
>> 
>>> Regcache calls in sol-thread.c are bogus and do nothing.
>>> The code in between will not change or update regcache.
>>> Removed.
>> 
>> If we remove code, we need to figure out why the code was there.
>> The code is about to remove was added by
>> 
>> commit 7cdd6cac82faad2083029b2ac014d44d869f76c0
>> Author: Frank Ch. Eigler <fche@redhat.com>
>> Date:   Thu Apr 2 18:54:04 1998 +0000
>> 
>>    * Fixes for PR 14571.
>> [...]
> 
> Heh, sorry, my recollection of this after 19 years is totally blank.
> 
> Interestingly, PR14571 must refer to the pre-bugzilla bug tracker
> system.  There was one on sourceware.org, but its web interface is no
> longer operating, and scraps of the database files that are still
> archived seem not to go up to 14571.  Likewise, no google hits.
> 
> So I'm at a loss, can't find orginal supporting data either, and
> that's a bummer.
> 
> - FChE

Reading both the 1998 patch and the code today, it looks to me like the
patch never adding anything.

Unless I’m missing something, the code is doing:


If only backing if 1 register to kernel
then
    *Backup the register in regcache into local var old_value.

    Read all general registers from kernel into local var gregset.
    Read all fp registers from kernel into local var fpregset.

    *Restore the register from local var old_value back to regcache.
EndIf

Copy 1 or all general registers from regcache into local var gregset.
Copy 1 or all fp registers from regcache into local var fpregset.

Store gregset to kernel.
Store fpregset to kernel.


Where the lines with the * are the lines no required.



If people are unsure, I’m happy to keep the code and replace the array
with std::vector (or anything else).


Alan.

\x16º&Öéj×!zÊÞ¶êç×~÷ëÙb²Ö«r\x18\x1dn–­r\x17¬

Re: [PATCH] Remove MAX_REGISTER_SIZE from sol-thread.c

[Yao Qi]

On 17-03-01 18:35:36, Alan Hayward wrote:
> 
> Reading both the 1998 patch and the code today, it looks to me like the
> patch never adding anything.
>

Agreed.

> 
> 
> If people are unsure, I???m happy to keep the code and replace the array
> with std::vector (or anything else).
> 

I believe your patch is correct.  Leave this patch here for several
days, if there is no other comments, you can push it in.

-- 
Yao

Re: [PATCH] Remove MAX_REGISTER_SIZE from sol-thread.c

[Frank Ch. Eigler]

Hi -

> > [...]  There was one on sourceware.org, but its web interface is no
> > longer operating, and scraps of the database files that are still
> > archived seem not to go up to 14571.  Likewise, no google hits.
> > 
> > So I'm at a loss, can't find orginal supporting data either, and
> > that's a bummer.
> 
> Thank you all the same.

Hold on!  Personal email archives to the rescue.  gdb/14571 was an
internal Cygnus PRMS bug number.  This was solaris 2.5.1 era.  

Here are some excerpts from emails on the topic.  Please excuse the
goofy "autoethnography", I'm just so weirdly gratified to have found
some traces of this old work!

------------------------------------------------------------------------

The problem was caused by sol-thread.c's inability to write to
individual registers in the target.  The updated value got lost in
sol_thread_store_registers before being written out.  The following
patch corrects the problem.

------------------------------------------------------------------------

>Can you send me a pointer to your toy test case?

Argh - I think I zapped it accidentally.  Basically it looked
like this:


void* thread_me(void* arg) { sleep(1); return arg; }

main()
 { 
   thread_t id;
   register int i;
   thr_create(NULL, NULL, thread_me, NULL, <flags>, &id);
   /* breakpoint here */
   i = 0;
   i = 1;
   i = 2;
 }


I fidgeted with the <flags> to get various THR_* combinations.

My desired test was to be unable to change the register variable i by
commands like "p (i=4)" from the debugger, after the breakpoint is
hit.  (Before my patch, I would expect the new value to be clobbered.)
I wanted gdb to go through the sol-thread.c's register-setting function,
but the bugger would not go near it.

------------------------------------------------------------------------

>[...]
>Can you tell me a bit how you figured gdb/14571 out. I am
>seeing more multithread releted bugs and I was hoping you
>could share some hints and methods.

Sure, though I must admin it was just plain ordinary
perseverance rather than any special technique.  As I recall,
I went along steps like these:

- traced backward from the "target halted while in function"
  call to the expression parser
- figured out the mechanism used to make inferior calls
  (that ugly stack frame stuff, and the pre-assembled
  block of instructions)
- convinced myself that the code should work, mostly by
  very slow single-stepping in the region
- found the bunch of routines that get/set inferior
  registers
- figured out weird register caching mechanism
- traced it work through the thread_db & procfs calls; it
  worked for threads fine
- found, by breakpoints, that the PC set in the cache at
  an earlier point was reset to an old value, before control
  was handed to the target - an anomaly
- let a very slow watchpoint run go overnight to find who
  was overwriting the PC slot in the cache
- eureka one morning: watchpoint triggered at around 8 AM. :)
- a bug in the threads/procfs code caused the register cache
  to be invalidated (re-read), if not all registers were
  desired to be updated

It was cool, but as you see, nothing very special in reasoning.
I just eliminated a bunch of possible problems, and learned how
registers, threads, and inferior calls were supposed to work,
and then ultimately found one anomaly.  The watchpoint mechanism
could then (very slowly) find the anomaly's trigger.

------------------------------------------------------------------------


- FChE

Re: [PATCH] Remove MAX_REGISTER_SIZE from sol-thread.c

[Yao Qi]

On Wed, Mar 1, 2017 at 9:59 PM, Frank Ch. Eigler <fche@redhat.com> wrote:
> Hold on!  Personal email archives to the rescue.  gdb/14571 was an
> internal Cygnus PRMS bug number.  This was solaris 2.5.1 era.
>
> Here are some excerpts from emails on the topic.  Please excuse the
> goofy "autoethnography", I'm just so weirdly gratified to have found
> some traces of this old work!
>

They look great.

> ------------------------------------------------------------------------
>
> The problem was caused by sol-thread.c's inability to write to
> individual registers in the target.  The updated value got lost in
> sol_thread_store_registers before being written out.  The following
> patch corrects the problem.

> - a bug in the threads/procfs code caused the register cache
>   to be invalidated (re-read), if not all registers were
>   desired to be updated

Now, I understand the problem you fixed 19 years ago.  That is,
before we write out the new value in global buffer "registers", we have
p_td_thr_getgregs call, like this,

      val = p_td_thr_getgregs (&thandle, regset);
      if (val != TD_OK)
        error ("sol_thread_store_registers: td_thr_getgregs %s",
               td_err_string (val));
      val = p_td_thr_getfpregs (&thandle, &fpregset);
      if (val != TD_OK)
        error ("sol_thread_store_registers: td_thr_getfpregs %s",
               td_err_string (val));

td_thr_getgregs in thread_db calls ps_lgetregs (in sol-threads.c) which
fetches registers again and save it in global buffer "registers" (in
findvar.c:supply_register), so that the new value to be written out in
"registers" is lost.  The save/restore across td_thr_getgregs/td_thr_getfpregs
fixes this problem.

Nowadays, we don't have the global buffer any more, so the save/restore
is no longer needed.

-- 
Yao (齐尧)