RFA: assert that target_fetch_registers did its job

RFA: assert that target_fetch_registers did its job

[Jim Blandy]

Does anyone see anything wrong with this?  Should it be an error, or a
warning, instead of an internal error?  It seems to me that the error
should be furnished by the target-specific code; if
target_fetch_registers returns silently, it should have done its job.

But thread_db_fetch_registers doesn't follow that assumption.  In the
threaded case, given any register number, it fetches the gprs, and the
fprs, supplies them, and assumes its job is done.  It seems to me it
sholud be calling register_valid_p (current_regcache, regno) to check
that the register's value has really been supplied, and complaining if
it hasn't.

Or should regcache.c check these conditions and report the warning /
error?  The behavior being hoped for here is target-independent.


2004-07-21  Jim Blandy  <jimb@redhat.com>

	* regcache.c (legacy_read_register_gen, regcache_raw_read): Assert
	that, after calling target_fetch_registers, the register we're
	reading is cached.

Index: gdb/regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/regcache.c,v
retrieving revision 1.116
diff -c -p -r1.116 regcache.c
*** gdb/regcache.c	10 Jul 2004 01:17:52 -0000	1.116
--- gdb/regcache.c	21 Jul 2004 23:04:46 -0000
*************** legacy_read_register_gen (int regnum, ch
*** 735,740 ****
--- 735,742 ----
    if (!register_cached (regnum))
      target_fetch_registers (regnum);
  
+   gdb_assert (register_cached (regnum));
+ 
    memcpy (myaddr, register_buffer (current_regcache, regnum),
  	  DEPRECATED_REGISTER_RAW_SIZE (regnum));
  }
*************** regcache_raw_read (struct regcache *regc
*** 768,773 ****
--- 770,776 ----
  	}
        if (!register_cached (regnum))
  	target_fetch_registers (regnum);
+       gdb_assert (register_cached (regnum));
      }
    /* Copy the value directly into the register cache.  */
    memcpy (buf, register_buffer (regcache, regnum),

Re: RFA: assert that target_fetch_registers did its job

[Daniel Jacobowitz]

On Fri, Jul 23, 2004 at 05:59:11PM -0500, Jim Blandy wrote:
> 
> Does anyone see anything wrong with this?  Should it be an error, or a
> warning, instead of an internal error?  It seems to me that the error
> should be furnished by the target-specific code; if
> target_fetch_registers returns silently, it should have done its job.

It shouldn't be an error.  internal-error makes the most sense to me;
if we need to relax it while some broken target is worked on, then
it should be a warning or silent.

> But thread_db_fetch_registers doesn't follow that assumption.  In the
> threaded case, given any register number, it fetches the gprs, and the
> fprs, supplies them, and assumes its job is done.  It seems to me it
> sholud be calling register_valid_p (current_regcache, regno) to check
> that the register's value has really been supplied, and complaining if
> it hasn't.

I suggest we slay thread_db_fetch_registers.

Once upon a time, it served a purpose.  Now it is nothing but a source
of problems.  We could pass opaque cookies rather than register data
through the gregset structure - the interface doesn't really support
this but at least two of the five thread-db implementations I'm aware
of would.  Or we could just give up, use thread-db for nothing besides
finding new threads, and ask the LWP for its registers directly without
six or eight call frames of indirection.

-- 
Daniel Jacobowitz

Re: RFA: assert that target_fetch_registers did its job

[Andrew Cagney]

> On Fri, Jul 23, 2004 at 05:59:11PM -0500, Jim Blandy wrote:
> 
>>> 
>>> Does anyone see anything wrong with this?  Should it be an error, or a
>>> warning, instead of an internal error?  It seems to me that the error
>>> should be furnished by the target-specific code; if
>>> target_fetch_registers returns silently, it should have done its job.
> 
> 
> It shouldn't be an error.  internal-error makes the most sense to me;

Right, it's a contract between two internal parts of gdb.

>>> But thread_db_fetch_registers doesn't follow that assumption.  In the
>>> threaded case, given any register number, it fetches the gprs, and the
>>> fprs, supplies them, and assumes its job is done.  It seems to me it
>>> sholud be calling register_valid_p (current_regcache, regno) to check
>>> that the register's value has really been supplied, and complaining if
>>> it hasn't.
> 
> 
> I suggest we slay thread_db_fetch_registers.
> 
> Once upon a time, it served a purpose.  Now it is nothing but a source
> of problems.  We could pass opaque cookies rather than register data
> through the gregset structure - the interface doesn't really support
> this but at least two of the five thread-db implementations I'm aware
> of would.  Or we could just give up, use thread-db for nothing besides
> finding new threads, and ask the LWP for its registers directly without
> six or eight call frames of indirection.

You're saying have GNU/Linux thread_db_fetch_regsters bypass libthread-db?

The thread-db can certainly take the shortest path to the registers. 
However, I'm not so sure about core GDB doing the bypass - it would 
violate the separation of thread and lwp (what little there is).

--

Looking at the internals of libthread-db, this looks like a strong 
motivation for introducing more explicit greg, fpreg, ... target_objects 
and fetching the values using that.

Andrew

Re: RFA: assert that target_fetch_registers did its job

[Daniel Jacobowitz]

On Tue, Aug 03, 2004 at 08:36:04AM -0400, Andrew Cagney wrote:
> >On Fri, Jul 23, 2004 at 05:59:11PM -0500, Jim Blandy wrote:
> >>>But thread_db_fetch_registers doesn't follow that assumption.  In the
> >>>threaded case, given any register number, it fetches the gprs, and the
> >>>fprs, supplies them, and assumes its job is done.  It seems to me it
> >>>sholud be calling register_valid_p (current_regcache, regno) to check
> >>>that the register's value has really been supplied, and complaining if
> >>>it hasn't.
> >
> >
> >I suggest we slay thread_db_fetch_registers.
> >
> >Once upon a time, it served a purpose.  Now it is nothing but a source
> >of problems.  We could pass opaque cookies rather than register data
> >through the gregset structure - the interface doesn't really support
> >this but at least two of the five thread-db implementations I'm aware
> >of would.  Or we could just give up, use thread-db for nothing besides
> >finding new threads, and ask the LWP for its registers directly without
> >six or eight call frames of indirection.
> 
> You're saying have GNU/Linux thread_db_fetch_regsters bypass libthread-db?
> 
> The thread-db can certainly take the shortest path to the registers. 
> However, I'm not so sure about core GDB doing the bypass - it would 
> violate the separation of thread and lwp (what little there is).

Everything I suggested in that paragraph was intended to live in the
GNU/Linux thread-db support, not in GDB core.  For GNU/Linux,
maintaining the politely broken fiction that one thread and one LWP are
not equivalent is not worthwhile any more.

Is there even a thread/lwp separation in GDB core?  My understanding
was that the core should never see anything having to do with an LWP;
at least the trivial LWP == thread mapping should have taken place
somewhere, and GDB should just talk to the threads.

-- 
Daniel Jacobowitz

Re: RFA: assert that target_fetch_registers did its job

[Andrew Cagney]

> Is there even a thread/lwp separation in GDB core?  My understanding
> was that the core should never see anything having to do with an LWP;
> at least the trivial LWP == thread mapping should have taken place
> somewhere, and GDB should just talk to the threads.

In theory, yes.  In reality, look at regcache.c :-(

Andrew

Re: RFA: assert that target_fetch_registers did its job

[Jim Blandy]

I've committed the following:

2004-07-21  Jim Blandy  <jimb@redhat.com>

	* regcache.c (regcache_raw_read): Assert that, after calling
        target_fetch_registers, the register we're reading is cached.

Index: gdb/regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/regcache.c,v
retrieving revision 1.121
diff -c -p -r1.121 regcache.c
*** gdb/regcache.c	2 Aug 2004 21:58:44 -0000	1.121
--- gdb/regcache.c	2 Aug 2004 23:18:56 -0000
*************** regcache_raw_read (struct regcache *regc
*** 614,619 ****
--- 614,620 ----
  	}
        if (!register_cached (regnum))
  	target_fetch_registers (regnum);
+       gdb_assert (register_cached (regnum));
      }
    /* Copy the value directly into the register cache.  */
    memcpy (buf, register_buffer (regcache, regnum),

Re: RFA: assert that target_fetch_registers did its job

[Nathan J. Williams]

Jim Blandy <jimb@redhat.com> writes:

> Does anyone see anything wrong with this?  Should it be an error, or a
> warning, instead of an internal error?  It seems to me that the error
> should be furnished by the target-specific code; if
> target_fetch_registers returns silently, it should have done its job.

I noticed that "info reg" tripped over this with the BSD KVM target,
because the KVM backend only loads in the registers that are present
in the PCB. Should the KVM backend be zeroing out everything else
explicitly (is there a regcache call to do the wipe for us)?

        - Nathan

Re: RFA: assert that target_fetch_registers did its job

[Jim Blandy]

"Nathan J. Williams" <nathanw@wasabisystems.com> writes:

> Jim Blandy <jimb@redhat.com> writes:
> 
> > Does anyone see anything wrong with this?  Should it be an error, or a
> > warning, instead of an internal error?  It seems to me that the error
> > should be furnished by the target-specific code; if
> > target_fetch_registers returns silently, it should have done its job.
> 
> I noticed that "info reg" tripped over this with the BSD KVM target,
> because the KVM backend only loads in the registers that are present
> in the PCB. Should the KVM backend be zeroing out everything else
> explicitly (is there a regcache call to do the wipe for us)?

Silently supplying dummy register contents is confusing to developers
and users.  That just seems uncool.

There's no way to return an 'unavailable' indication from the regcache
cooked read functions.  And there are so many clients of that
interface it might be hard to introduce such an indication.  (Although
most of the uses are in -tdep.c files; the only real use in core code
is in sentinal_frame_prev_register, which does have a way to return
'unavailable'.  Hmm.)

Throwing an error would interrupt the register listing, even if later
registers were available.

So that leaves printing a warning message and supplying a dummy
value.  

Perhaps another option would be something like the below: it informs
users that things are not as they seem, and encourages developers to
fix up their targets.  Comments?


2004-08-06  Jim Blandy  <jimb@redhat.com>

	* regcache.c (regcache_raw_read): Replace assertion with a warning
	message.

Index: gdb/regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/regcache.c,v
retrieving revision 1.125
diff -c -p -r1.125 regcache.c
*** gdb/regcache.c	4 Aug 2004 17:50:55 -0000	1.125
--- gdb/regcache.c	6 Aug 2004 23:42:56 -0000
*************** regcache_raw_read (struct regcache *regc
*** 614,620 ****
  	}
        if (!register_cached (regnum))
  	target_fetch_registers (regnum);
!       gdb_assert (register_cached (regnum));
      }
    /* Copy the value directly into the register cache.  */
    memcpy (buf, register_buffer (regcache, regnum),
--- 614,649 ----
  	}
        if (!register_cached (regnum))
  	target_fetch_registers (regnum);
! 
!       /* FIXME: jimb/2004-08-06: Ideally, this would just be an
!          assert: target_fetch_registers should either throw an error,
!          or print a warning and furnish dummy register contents.  But
!          it should never say "okay" without actually supplying the
!          register.
! 
!          But in reality, there are a lot of arch / target combinations
!          where the arch's regset has grown beyond what the target
!          actually supplies.  Having GDB crash is too disruptive, but
!          silently using uninitialized bits is misleading as well.
! 
!          So this warning is here to prompt people to work on their
!          targets and get the mismatches sorted out, and to warn users
!          that they're not getting real data.  */
!       if (! register_cached (regnum))
!         {
!           struct gdbarch *arch = get_regcache_arch (regcache);
!           const char *name = gdbarch_register_name (arch, regnum);
! 
!           if (name)
!             warning ("unable to retrieve contents of raw register '%s' (#%d).",
!                      name, regnum);
!           else
!             warning ("unable to retrieve contents of raw register #%d.",
!                      regnum);
! 
!           memset (register_buffer (regcache, regnum), 0xff,
!                   regcache->descr->sizeof_register[regnum]);
!         }
      }
    /* Copy the value directly into the register cache.  */
    memcpy (buf, register_buffer (regcache, regnum),

Re: RFA: assert that target_fetch_registers did its job

[Ian Lance Taylor]

Jim Blandy <jimb@redhat.com> writes:

> Perhaps another option would be something like the below: it informs
> users that things are not as they seem, and encourages developers to
> fix up their targets.  Comments?

Aren't users going to wind up getting this error message approximately
one gazillion times when doing debugging on a target which doesn't
provide all the registers?

I don't know if this is the right approach in general, but if it is I
think it at least needs a static flag or something to only issue the
error once per execution.

I'm not sure this is the right approach because I don't see how to
avoid the error, at least not with something like BSD KVM.  The
registers are in the target, but the values just aren't available.
It's not like there is any way to fix that.

Ian

Re: RFA: assert that target_fetch_registers did its job

[Andrew Cagney]

> Jim Blandy <jimb@redhat.com> writes:
> 
> 
>>> Perhaps another option would be something like the below: it informs
>>> users that things are not as they seem, and encourages developers to
>>> fix up their targets.  Comments?

I'll disable this while we sort things out.  I've just hit another 
system (PPC/NetBSD) that panics.

While requiring that the inferior always `supply' a register appears 
reasonable (and I agree with the theory) it turns out we don't have the 
mechanisms for indicating all the possible supplied register states (see 
below) and the implementors of the supply code didn't realise that was 
part of the contract :-(

I think we can also just as effectively (and not as fatally :-) use the 
testsuite and something based on ``maint print cooked-registers'' (it 
will need to print state info) to detect mis-behaving inferior code.

> Aren't users going to wind up getting this error message approximately
> one gazillion times when doing debugging on a target which doesn't
> provide all the registers?
> 
> I don't know if this is the right approach in general, but if it is I
> think it at least needs a static flag or something to only issue the
> error once per execution.

Here's a brain dump of the current situtation:

A register, be it in the regcache or a frame, can have 4 states (I've 
listed the regcache states where applicable, the frame uses a flag 
`optimised out'):

- invalid (0)
Initial state, yet to probe for the register.

- valid (1)
The inferior was able to supply a full register value.

- unsupported
The register is not available on this system (due to limitations of the 
ptrace interface say).  Yes, there currently isn't a state for this :-(

- unknown (-1)
For what ever reason, the the register isn't known at this point in 
time.  (For CFI because it wasn't saved).

When it comes to using these values, GDB ducks the issue.  If an attempt 
to fetch a frame's register results in a memory problem, an error is 
thrown, if it results in an unknown/unsupported register, zero is 
silently returned.

I think:

- the register cache should just mark up the status of its registers and 
trust that the client will notice the problems

- the frame code should detect unknown / unsupported register accesses 
and throw an error

- the code displaying variables, using register values, should catch / 
incorporate such exceptions in their output vis:

(gdb) print foo
$1 = struct { string = <Memory error at 0x12345>, integer = <Register r3 
unknown>, ...}

however we've a ways to go before this happens.  We've not even killed 
of deprecated_registers[].

Andrew

Re: RFA: assert that target_fetch_registers did its job

[Jim Blandy]

Andrew Cagney <cagney@gnu.org> writes:
> I'll disable this while we sort things out.  I've just hit another
> system (PPC/NetBSD) that panics.
> 
> While requiring that the inferior always `supply' a register appears
> reasonable (and I agree with the theory) it turns out we don't have
> the mechanisms for indicating all the possible supplied register
> states (see below) and the implementors of the supply code didn't
> realise that was part of the contract :-(
>
> I think we can also just as effectively (and not as fatally :-) use
> the testsuite and something based on ``maint print cooked-registers''
> (it will need to print state info) to detect mis-behaving inferior
> code.

Yeah, I think this is probably best.  I hadn't expected this much
unravelling when I tugged on that little thread.  :)

It seems pretty clear that some of the consumers of register contents
need to become more sophisticated.  But that will necessarily be a
more gradual process.