read_register_bytes() bug; was my Regcache revamp

read_register_bytes() bug; was my Regcache revamp

[Andrew Cagney]

To quote my original e-mail:

http://sources.redhat.com/ml/gdb-patches/2001-03/msg00312.htm

 > Hello,
 >
 > This patch may change your life!
 >
 > Well, yes, ok, I'm probably being a little dramatic!  However, it
 > certainly does change the way targets implement pseudo/cooked registers.
 >
 > The attatched patch revamps the regcache interface along the lines
 > described in:


Well its taken 5 months but I've finaly found a ``bug'' this change
introduces.

The old read_register_bytes() had the logic (greatly simplified):

	o	go through all registers
		and read each valid (as defined
		by REGISTER_NAME()) register
		into the cache

	o	copy out the relevant raw bytes
		from the register buffer

the new code combined those two:

	o	go through all registers
		if valid (as defined by REGISTER_NAME())
			read the register into the cache
			write the register into the buffer

The problem is that the register cache can have ``holes'' in it.  An
invalid/ignore/dne REGNUM, as determined from REGISTER_NAME(), can still
occupy space in the register buffer.  The old code was blindly copying
the contents of the holes.  The new code, skips them, leaving them
undefined - typically picking up garbage from the heap.

This shouldn't hurt since the holes are invalid/ignored/dne/... right?
Nope, a target was getting that ``invalid data'' and then using it - the
corresponding write_register_bytes() doesn't skip invalid/ignore/dne
REGNUMs.

I think the ``real bug'' is that the updated read_register_bytes() can
leave part of the buffer undefined.  I'm thinking of either changing
things to:

	o	initializing the gaps from
		the regcache (restoring old
		behavour)

	o	initializing the gaps with
		0xdeadbeaf.

I am very tempted to implement the latter but suspect I'll be forced to
implement the former, Sigh!

	Andrew

[rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Andrew Cagney]

> I think the ``real bug'' is that the updated read_register_bytes() can
> leave part of the buffer undefined.  I'm thinking of either changing
> things to:
> 
>     o    initializing the gaps from
>         the regcache (restoring old
>         behavour)


The attached patch implements this.  Thoughts?

	Andrew

Re: What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Eli Zaretskii]

> Date: Mon, 20 Aug 2001 01:42:51 -0400
> From: Andrew Cagney <ac131313@cygnus.com>
> 
> As far as I know most of the patches going past at the moment are not 
> going onto the branch(1).  Hmm, quick check, I've attached a copy of the 
> 5.1 ChangeLog.

Thanks.

Why aren't the entries there in chronological order?

> > Perhaps in the future each patch could tell explicitly whether it is
> > meant ffor the branch or not, to prevent confusion.
> 
> I've been using the prefix [.../5.1] for anything 5.1 related.

Perhaps everyone could follow that convention.

Re: What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Daniel Jacobowitz]

On Mon, Aug 20, 2001 at 09:09:41AM +0300, Eli Zaretskii wrote:
> > Date: Mon, 20 Aug 2001 01:42:51 -0400
> > From: Andrew Cagney <ac131313@cygnus.com>
> > 
> > As far as I know most of the patches going past at the moment are not 
> > going onto the branch(1).  Hmm, quick check, I've attached a copy of the 
> > 5.1 ChangeLog.
> 
> Thanks.
> 
> Why aren't the entries there in chronological order?

I tend to date ChangeLog entries with the day the patch was last
modified, not the day it was committed.  Do others prefer to keep the
changelog dates relative to the day the source changed?

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Eli Zaretskii]

> Date: Sun, 19 Aug 2001 23:17:47 -0700
> From: Daniel Jacobowitz <drow@mvista.com>
> > 
> > Why aren't the entries there in chronological order?
> 
> I tend to date ChangeLog entries with the day the patch was last
> modified, not the day it was committed.

I think this is wrong: the logs should reflect the commit time, and if
they aren't chronologically increasing, it's hard to find a specific
entry and even harder to figure out which change came after which,
without resorting to CVS.

Re: What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Mark Kettenis]

"Eli Zaretskii" <eliz@is.elta.co.il> writes:

> > Date: Sun, 19 Aug 2001 23:17:47 -0700
> > From: Daniel Jacobowitz <drow@mvista.com>
> > > 
> > > Why aren't the entries there in chronological order?
> > 
> > I tend to date ChangeLog entries with the day the patch was last
> > modified, not the day it was committed.
> 
> I think this is wrong: the logs should reflect the commit time, and if
> they aren't chronologically increasing, it's hard to find a specific
> entry and even harder to figure out which change came after which,
> without resorting to CVS.

Oh dear!  It's the ChangeLog dating issue again.  It is generally
accepted among the GNU projects to date the entries with the day the
patch was last modified.  Which patch came after which is implied by
the order in which the entries appear in the ChangeLog file.

Mark

Re: What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Eli Zaretskii]

> From: Mark Kettenis <kettenis@science.uva.nl>
> Date: 21 Aug 2001 00:06:14 +0200
> > 
> > I think this is wrong: the logs should reflect the commit time, and if
> > they aren't chronologically increasing, it's hard to find a specific
> > entry and even harder to figure out which change came after which,
> > without resorting to CVS.
> 
> Oh dear!  It's the ChangeLog dating issue again.

Sorry to raise that again, but I don't think I ever saw this being
discussed since the time I became involved with GDB.  If this has been
beaten to death, it should probably be spelled out in the coding
conventions docs.  Did I miss it?

> It is generally
> accepted among the GNU projects to date the entries with the day the
> patch was last modified.

Which GNU projects are those?  Emacs is not one of them: they take
great care there to have all the entries labeled by the commit date.

> Which patch came after which is implied by
> the order in which the entries appear in the ChangeLog file.

That order can be messed up by snafus such as CVS conflicts etc.

Why is it such a problem to label the entries with the date when the
change is committed?  I don't see any disadvantages to this, only
advantages.

Re: What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Kevin Buettner]

On Aug 19, 11:17pm, Daniel Jacobowitz wrote:

> I tend to date ChangeLog entries with the day the patch was last
> modified, not the day it was committed.  Do others prefer to keep the
> changelog dates relative to the day the source changed?

I prefer to see ChangeLog entries that reflect the date of the
commit for the reasons Eli mentioned.  The only exception to this
is that when merging changes from the mainline to the branch, I'd
rather see the dates stay the same as listed in the mainline
to make it easier to see which changes have been brought in...

Kevin

Re: What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Andrew Cagney]

> Date: Sun, 19 Aug 2001 23:17:47 -0700
>> From: Daniel Jacobowitz <drow@mvista.com>
> 
>> > 
>> > Why aren't the entries there in chronological order?
> 
>> 
>> I tend to date ChangeLog entries with the day the patch was last
>> modified, not the day it was committed.
> 
> 
> I think this is wrong: the logs should reflect the commit time, and if
> they aren't chronologically increasing, it's hard to find a specific
> entry and even harder to figure out which change came after which,
> without resorting to CVS.

Yes, Eli's correct. Allowing for timezones and approval latency a few 
days here or there is ok.

Anyway, this is why I personally adopted the convention:

2001-07-28  Andrew Cagney  <ac131313@redhat.com>

         Fix some PID/TPID fallout for HP/UX.
         From 2001-07-22 Rodney Brown <rbrown64@csc.com.au>:
         * infttrace.c (ptrace_wait): Match external declaration,
         and match target_post_wait declaration.

so that the original date and the time of commit are retained.  This 
isn't a GNU standard.

	Andrew

What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Andrew Cagney]

> Frankly, for quite some time I'm confused by the RFC's and RFA's
> posted here.  Are they all meant to go to the branch as well as the
> trunk?  If so, is it wise to make such changes after cutting the
> release branch?

As far as I know most of the patches going past at the moment are not 
going onto the branch(1).  Hmm, quick check, I've attached a copy of the 
5.1 ChangeLog.

This current thread/patch, however is interesting, I was not thinking of 
it as something for the 5.1 branch, but, yes, it probably is.

> Perhaps in the future each patch could tell explicitly whether it is
> meant ffor the branch or not, to prevent confusion.

I've been using the prefix [.../5.1] for anything 5.1 related.

enjoy,  (I hope this helps)

	Andrew

Now to try to get http://sources.redhat.com/gdb/ == 
http://www.gnu.org/software/gdb/ .

(1) For me it is stuff like overhalling the floating-point code; fixing 
some nasty true multi-arch initialization problems; more MI and LIBGDB 
cleanup; more multi-arch; ...

Re: What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Andrew Cagney]

> 
> Thanks.
> 
> Why aren't the entries there in chronological order?

You mean the first few entries?  I'd guess that H.J. committed that 
change to the trunk and then Daniel picked it up and committed it to the 
branch several days later.

	Andrew

Re: What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Andrew Cagney]

> Which patch came after which is implied by
>> the order in which the entries appear in the ChangeLog file.
> 
> 
> That order can be messed up by snafus such as CVS conflicts etc.
> 
> Why is it such a problem to label the entries with the date when the
> change is committed?  I don't see any disadvantages to this, only
> advantages

The disadvantage I'm aware of occures when you're trying to determine if 
a specific change has been committed (I tend to be doing this a lot). 
The date (especially the pre ISO date which included the time) makes a 
really good identifying stamp.  Per other e-mail, this is why I'll 
retain the date-stamp when committing a patch.  I'll even occasionally do:

> 2001-03-19  Andrew Cagney  <ac131313@redhat.com>
> 
>         From Mon Nov 20 13:59:29 2000 Andrew Cagney <cagney@b1.cygnus.com>:

:-)
	Andrew

Re: What is on the 5.1 branch; Was: [rfc] Re: read_register_bytes() bug; was my Regcache revamp

[Eli Zaretskii]

> Date: Tue, 21 Aug 2001 11:16:04 -0400
> From: Andrew Cagney <ac131313@cygnus.com>
> > 
> > Why is it such a problem to label the entries with the date when the
> > change is committed?  I don't see any disadvantages to this, only
> > advantages
> 
> The disadvantage I'm aware of occures when you're trying to determine if 
> a specific change has been committed (I tend to be doing this a lot). 
> The date (especially the pre ISO date which included the time) makes a 
> really good identifying stamp.

Alternatively, you could search for some string in the ChangeLog entry
posted here (the same place where I guess you find the date to look
for).