Failures in store.exp caused by regcache

Failures in store.exp caused by regcache

[Daniel Jacobowitz]

(Or rather, by the value code's interaction with the regcache)

Andrew, this is more your area; I'd like your advice before I dig any
further.  Here's what's going wrong.  Consider the command sequence:
"up; print u; set u = s_1; print u".
  - u has class LOC_REGISTER
  - The register's home is memory
  - read_var_value therefore returns an lval_memory
  - the value of the register is in the register unwind cache at this point
  - we modify the memory backing the store
  - we have no way to tell that we've just modified the value of a saved
    register on the stack
  - the second print returns the cached value

So, what do we do?  I really don't want to flush cached register values in
value_assign.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: Failures in store.exp caused by regcache

[Andrew Cagney]

> (Or rather, by the value code's interaction with the regcache)
> 
> Andrew, this is more your area; I'd like your advice before I dig any
> further.  Here's what's going wrong.  Consider the command sequence:
> "up; print u; set u = s_1; print u".
>   - u has class LOC_REGISTER
>   - The register's home is memory
>   - read_var_value therefore returns an lval_memory
>   - the value of the register is in the register unwind cache at this point
>   - we modify the memory backing the store
>   - we have no way to tell that we've just modified the value of a saved
>     register on the stack
>   - the second print returns the cached value
> 
> So, what do we do?

Flush the frame cache.

Andrew

Re: Failures in store.exp caused by regcache

[Daniel Jacobowitz]

On Wed, Jan 08, 2003 at 02:10:44PM -0500, Andrew Cagney wrote:
> >(Or rather, by the value code's interaction with the regcache)
> >
> >Andrew, this is more your area; I'd like your advice before I dig any
> >further.  Here's what's going wrong.  Consider the command sequence:
> >"up; print u; set u = s_1; print u".
> >  - u has class LOC_REGISTER
> >  - The register's home is memory
> >  - read_var_value therefore returns an lval_memory
> >  - the value of the register is in the register unwind cache at this point
> >  - we modify the memory backing the store
> >  - we have no way to tell that we've just modified the value of a saved
> >    register on the stack
> >  - the second print returns the cached value
> >
> >So, what do we do?
> 
> Flush the frame cache.

Ugg.  Well, if we have to, then we have to.  I suppose we do.

We obviously want to preserve things like the selected frame, however. 
Andrew, should I do this the way I do for "set backtrace-below-main",
and should there be a general function for that?  I.E.:

void
do_flush_frames_sfunc (char *args, int from_tty, struct cmd_list_element *c)
{
  int saved_level;
  struct frame_info *cur_frame;

  if (! target_has_stack)
    return;

  saved_level = frame_relative_level (get_selected_frame ());

  flush_cached_frames ();

  cur_frame = find_relative_frame (get_current_frame (), &saved_level);
  select_frame (cur_frame);

  /* If we were below main and backtrace-below-main was turned off,
     SAVED_LEVEL will be non-zero.  CUR_FRAME will point to main.
     Accept this but print the new frame.  */
  if (saved_level != 0)
    print_stack_frame (get_selected_frame (), -1, 0);
}


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: Failures in store.exp caused by regcache

[Andrew Cagney]

> On Wed, Jan 08, 2003 at 02:10:44PM -0500, Andrew Cagney wrote:
> 
>> >(Or rather, by the value code's interaction with the regcache)
>> >
>> >Andrew, this is more your area; I'd like your advice before I dig any
>> >further.  Here's what's going wrong.  Consider the command sequence:
>> >"up; print u; set u = s_1; print u".
>> >  - u has class LOC_REGISTER
>> >  - The register's home is memory
>> >  - read_var_value therefore returns an lval_memory
>> >  - the value of the register is in the register unwind cache at this point
>> >  - we modify the memory backing the store
>> >  - we have no way to tell that we've just modified the value of a saved
>> >    register on the stack
>> >  - the second print returns the cached value
>> >
>> >So, what do we do?
> 
>> 
>> Flush the frame cache.
> 
> 
> Ugg.  Well, if we have to, then we have to.  I suppose we do.

It isn't that bad.  In fact (per previous discussion), the code needed 
to avoid flushing the caches would be far worse than what we have now.

The only time the frame cache gets (well, ok, should get ...) flushed is:

- when the target resumes
The recovery time here is critical.
- when the target is modified
The recovery time here is bounded by the recovery time from a target resume.

People `never' modify the target.  That leaves the time taken to recover 
from a resume and then:
- we must flush the cache
- since it is an upper bound on target modify recovery time, making it 
faster is a win win.

> We obviously want to preserve things like the selected frame, however. 

I'm actually a bit puzzled.  I recently fixed a case and added a 
testcase (store.exp) that handles stores.  Look for frame_find_by_id() 
in valops.c.

 > Andrew, should I do this the way I do for "set backtrace-below-main",
 > and should there be a general function for that?  I.E.:

Have a look at the comments in "frame.h" around reinit_frame_cache() and 
get_selected_frame().  In particular:

    FIXME: cagney/2002-11-28: The only difference between
    flush_cached_frames() and reinit_frame_cache() is that the latter
    explicitly sets the selected frame back to the current frame there
    isn't any real difference (except that one delays the selection of
    a new frame).  Code can instead simply rely on get_selected_frame()
    to reinit's the selected frame as needed.  As for invalidating the
    cache, there should be two methods one that reverts the thread's
    selected frame back to current frame (for when the inferior
    resumes) and one that does not (for when the user modifies the
    target invalidating the frame cache).  */

/* FIXME: cagney/2002-11-28: At present, when there is no selected
    frame, this function always returns the current (inner most) frame.
    It should instead, when a thread has previously had its frame
    selected (but not resumed) and the frame cache invalidated, find
    and then return that thread's previously selected frame.  */

This only works if we're storing the selected frame in the selected 
thread.  Unfortunatly (arrrrrrg), GDB first needs to be convinced that 
`there is always a thread' so that there is always a `struct 
thread_info' into which the selected frame's id can be stored (or, I 
guess, as a temp, extend the existing `there might be a thread' hack to 
handle this case, double arrrrg).

The below does raise an interesting question:

> void
> do_flush_frames_sfunc (char *args, int from_tty, struct cmd_list_element *c)
> {
>   int saved_level;
>   struct frame_info *cur_frame;
> 
>   if (! target_has_stack)
>     return;
> 
>   saved_level = frame_relative_level (get_selected_frame ());
> 
>   flush_cached_frames ();
> 
>   cur_frame = find_relative_frame (get_current_frame (), &saved_level);
>   select_frame (cur_frame);
> 
>   /* If we were below main and backtrace-below-main was turned off,
>      SAVED_LEVEL will be non-zero.  CUR_FRAME will point to main.
>      Accept this but print the new frame.  */
>   if (saved_level != 0)
>     print_stack_frame (get_selected_frame (), -1, 0);
> }

what should:

(gdb) set backtrace-below-main on
(gdb) up ; up ; up
(gdb) set backtrace-below-main off
(gdb) set variable x = 1

do?  get_next_frame() is going to refuse to go beyond main, no matter 
how many times you try.

(btw, a frame_id is safer than a level).

Andrew

Re: Failures in store.exp caused by regcache

[Daniel Jacobowitz]

On Thu, Jan 09, 2003 at 11:49:56AM -0500, Andrew Cagney wrote:
> >On Wed, Jan 08, 2003 at 02:10:44PM -0500, Andrew Cagney wrote:
> >
> >>>(Or rather, by the value code's interaction with the regcache)
> >>>
> >>>Andrew, this is more your area; I'd like your advice before I dig any
> >>>further.  Here's what's going wrong.  Consider the command sequence:
> >>>"up; print u; set u = s_1; print u".
> >>>  - u has class LOC_REGISTER
> >>>  - The register's home is memory
> >>>  - read_var_value therefore returns an lval_memory
> >>>  - the value of the register is in the register unwind cache at this 
> >>point
> >>>  - we modify the memory backing the store
> >>>  - we have no way to tell that we've just modified the value of a saved
> >>>    register on the stack
> >>>  - the second print returns the cached value
> >>>
> >>>So, what do we do?
> >
> >>
> >>Flush the frame cache.
> >
> >
> >Ugg.  Well, if we have to, then we have to.  I suppose we do.
> 
> It isn't that bad.  In fact (per previous discussion), the code needed 
> to avoid flushing the caches would be far worse than what we have now.
> 
> The only time the frame cache gets (well, ok, should get ...) flushed is:
> 
> - when the target resumes
> The recovery time here is critical.
> - when the target is modified
> The recovery time here is bounded by the recovery time from a target resume.
> 
> People `never' modify the target.  That leaves the time taken to recover 
> from a resume and then:
> - we must flush the cache
> - since it is an upper bound on target modify recovery time, making it 
> faster is a win win.

Makes sense.  If this ever gets to be a problem (I'm not sure people
won't find creative ways to make a mockery of that "never" :) we can be
cleverer then.

> >We obviously want to preserve things like the selected frame, however. 
> 
> I'm actually a bit puzzled.  I recently fixed a case and added a 
> testcase (store.exp) that handles stores.  Look for frame_find_by_id() 
> in valops.c.

Yes.  On GCC 2.95 that test shows failures still - different
optimization.  Thanks for pointing me at your change; I see the problem
now.  You only added the flush for the lval_register case.  This is an
lval_memory, for all that it's in the frame cache.  Slightly different
problem, same general solution.

Something to think about: theoretically, any call to write_memory can
cause the frame cache to become out of date.  For now I'll just move
the flushing within value_assign; but long-term I think it might be
generally worthwhile to have better information on when the cache needs
to be flushed, so that we can do it from within functions that modify
the target.  It would mean something like a list of memory locations
read in the process of building the frame cache.  Even per frame so
that we don't need to flush the whole thing.

Yeah, the complexity's gross; but there's a whole class of bugs here...

> > Andrew, should I do this the way I do for "set backtrace-below-main",
> > and should there be a general function for that?  I.E.:
> 
> Have a look at the comments in "frame.h" around reinit_frame_cache() and 
> get_selected_frame().  In particular:
> 
>    FIXME: cagney/2002-11-28: The only difference between
>    flush_cached_frames() and reinit_frame_cache() is that the latter
>    explicitly sets the selected frame back to the current frame there
>    isn't any real difference (except that one delays the selection of
>    a new frame).  Code can instead simply rely on get_selected_frame()
>    to reinit's the selected frame as needed.  As for invalidating the
>    cache, there should be two methods one that reverts the thread's
>    selected frame back to current frame (for when the inferior
>    resumes) and one that does not (for when the user modifies the
>    target invalidating the frame cache).  */
> 
> /* FIXME: cagney/2002-11-28: At present, when there is no selected
>    frame, this function always returns the current (inner most) frame.
>    It should instead, when a thread has previously had its frame
>    selected (but not resumed) and the frame cache invalidated, find
>    and then return that thread's previously selected frame.  */
> 
> This only works if we're storing the selected frame in the selected 
> thread.  Unfortunatly (arrrrrrg), GDB first needs to be convinced that 
> `there is always a thread' so that there is always a `struct 
> thread_info' into which the selected frame's id can be stored (or, I 
> guess, as a temp, extend the existing `there might be a thread' hack to 
> handle this case, double arrrrg).
> 
> The below does raise an interesting question:

Right.  These comments are why I wrote the below.

> >void
> >do_flush_frames_sfunc (char *args, int from_tty, struct cmd_list_element 
> >*c)
> >{
> >  int saved_level;
> >  struct frame_info *cur_frame;
> >
> >  if (! target_has_stack)
> >    return;
> >
> >  saved_level = frame_relative_level (get_selected_frame ());
> >
> >  flush_cached_frames ();
> >
> >  cur_frame = find_relative_frame (get_current_frame (), &saved_level);
> >  select_frame (cur_frame);
> >
> >  /* If we were below main and backtrace-below-main was turned off,
> >     SAVED_LEVEL will be non-zero.  CUR_FRAME will point to main.
> >     Accept this but print the new frame.  */
> >  if (saved_level != 0)
> >    print_stack_frame (get_selected_frame (), -1, 0);
> >}
> 
> what should:
> 
> (gdb) set backtrace-below-main on
> (gdb) up ; up ; up
> (gdb) set backtrace-below-main off
> (gdb) set variable x = 1
> 
> do?  get_next_frame() is going to refuse to go beyond main, no matter 
> how many times you try.

I decided "it should put you in main".  That's what the comment above
is for.  It's a kind of arbitrary decision but it was the most
consistent thing I could come up with.

> (btw, a frame_id is safer than a level).

Sure; but I think a level is more appropriate here.  I don't expect
anything above main to change when I rebuild the frame cache.  And this
way select_level () has the exact behavior I wanted: if the frame we
wanted isn't there any more, stop at the bottom.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: Failures in store.exp caused by regcache

[Andrew Cagney]

> Makes sense.  If this ever gets to be a problem (I'm not sure people
> won't find creative ways to make a mockery of that "never" :) we can be
> cleverer then.

Like Apple?

They, I believe, currently hold the award for the worst case senario. 
Due to objective C, the code does repeated inferior function calls from 
more outer frames.  This causes GDB to constantly rebuild the frame 
cache.  Can't see anyone doing worse than that, however, yes, never say 
never :-)

>> >We obviously want to preserve things like the selected frame, however. 
> 
>> 
>> I'm actually a bit puzzled.  I recently fixed a case and added a 
>> testcase (store.exp) that handles stores.  Look for frame_find_by_id() 
>> in valops.c.
> 
> 
> Yes.  On GCC 2.95 that test shows failures still - different
> optimization.  Thanks for pointing me at your change; I see the problem
> now.  You only added the flush for the lval_register case.  This is an
> lval_memory, for all that it's in the frame cache.  Slightly different
> problem, same general solution.

Instead of flush frames, flush dcache, flush registers.  I think there 
should only be a generic target_changed().

> Something to think about: theoretically, any call to write_memory can
> cause the frame cache to become out of date.  For now I'll just move
> the flushing within value_assign; but long-term I think it might be
> generally worthwhile to have better information on when the cache needs
> to be flushed, so that we can do it from within functions that modify
> the target.  It would mean something like a list of memory locations
> read in the process of building the frame cache.  Even per frame so
> that we don't need to flush the whole thing.

A single write to a single register/memory can completly change the 
entire target - consider a store to the page table base register.

> Yeah, the complexity's gross; but there's a whole class of bugs here...

This is a real case of `show us the money'.  Compared to single step, 
there is no money in trying to make target writes faster.  There is, 
however, plenty of return on being dumb - coherency problem - flush all 
caches - bug fixed.

Compare the single step performance of Insight with Eclipse and Project 
Builder.  Eclipe is a slow, not because of GDB or MI but because Eclipse 
hasn't yet focused on the important case - single step and handling that 
efficiently.


> I decided "it should put you in main".  That's what the comment above
> is for.  It's a kind of arbitrary decision but it was the most
> consistent thing I could come up with.

M'kay.

Andrew