[RFC] hppa/multiarch - PUSH_DUMMY_FRAME depends on inf_status

[RFC] hppa/multiarch - PUSH_DUMMY_FRAME depends on inf_status

[Joel Brobecker]

tm-hppa.h contains the following macro definition for PUSH_DUMMY_FRAME:

    /* FIXME: brobecker 2002-12-26.  This macro definition takes advantage
       of the fact that PUSH_DUMMY_FRAME is called within a function where
       a variable inf_status of type struct inferior_status * is defined.
       Ugh!  Until this is fixed, we will not be able to move to multiarch
       partial.  */
    #define PUSH_DUMMY_FRAME hppa_push_dummy_frame (inf_status)
    extern void hppa_push_dummy_frame (struct inferior_status *);

As said in the comment, there is this nasty dependency of the fact that
there is a variable called inf_status in the scope where this macro is
"invoked". This dependency is getting in the way of the multiarch conversion,
so I'd like to remove it.

I looked at the only place where PUSH_DUMMY_FRAME is used, and found
the following code (function hand_function_call in valops.c):

    /* A cleanup for the inferior status.  Create this AFTER the retbuf
       so that this can be discarded or applied without interfering with
       the regbuf.  */
    inf_status = save_inferior_status (1);
    inf_status_cleanup = make_cleanup_restore_inferior_status (inf_status);

    /* PUSH_DUMMY_FRAME is responsible for saving the inferior registers
       (and POP_FRAME for restoring them).  (At least on most machines)
       they are saved on the stack in the inferior.  */
    PUSH_DUMMY_FRAME;

So inf_status is obtained by a call to save_inferior_status. OK.

HP's push_dummy_frame uses it exclusively to hack in some of the
registers.  A comment explains why this is necessary:

  /* Oh, what a hack.  If we're trying to perform an inferior call
     while the inferior is asleep, we have to make sure to clear
     the "in system call" bit in the flag register (the call will
     start after the syscall returns, so we're no longer in the system
     call!)  This state is kept in "inf_status", change it there.
  
     We also need a number of horrid hacks to deal with lossage in the
     PC queue registers (apparently they're not valid when the in syscall
     bit is set).  */

They do it like this:

      write_inferior_status_register (inf_status, 0, int_buffer);
      write_inferior_status_register (inf_status, PCOQ_HEAD_REGNUM, pc + 0);
      write_inferior_status_register (inf_status, PCOQ_TAIL_REGNUM, pc + 4);

My first obvious idea was to try to find a way to get a "copy" of the
inf_status, for instance via save_inferior_status. But this seems to be
a deep copy, so I am afraid that creating a new copy of inf_status will
defeat a bit the purpose of the above (ie write the register in a
regcache that will not be restored after the function call).

Similarly, replacing the calls to write_inferior_status_register does
not seem to be possible either; it seems that we really need the
inf_status saved by hand_function_call.

But on the other hand, I must admit that I am still a bit confused as
to how registers should be accessed in GDB. So most probably there is
something that I don't get.

I was also wondering if this push_dummy_frame shouldn't be split in
2 functions. One that hacks the registers, and the other that does
the "pushing"... We could then introduce a new gdbarch method, this
time dependent on the inf_status. And we would have something like
this in hand_function_call:

        inf_status = ...;

        PREPARE_INF_STATUS_FOR_HAND_FUNCTION_CALL (inf_status);
        PUSH_DUMMY_FRAME;

Finally, another idea that crossed my mind was to update the PUSH_DUMMY_FRAME
method to accept one parameter which would be the inf_status. I must
say that I am not too keen on doing such a drastic change...

Any comments?

Thanks,
-- 
Joel

Re: [RFC] hppa/multiarch - PUSH_DUMMY_FRAME depends on inf_status

[Andrew Cagney]

> tm-hppa.h contains the following macro definition for PUSH_DUMMY_FRAME:
> 
>     /* FIXME: brobecker 2002-12-26.  This macro definition takes advantage
>        of the fact that PUSH_DUMMY_FRAME is called within a function where
>        a variable inf_status of type struct inferior_status * is defined.
>        Ugh!  Until this is fixed, we will not be able to move to multiarch
>        partial.  */
>     #define PUSH_DUMMY_FRAME hppa_push_dummy_frame (inf_status)
>     extern void hppa_push_dummy_frame (struct inferior_status *);
> 
> As said in the comment, there is this nasty dependency of the fact that
> there is a variable called inf_status in the scope where this macro is
> "invoked". This dependency is getting in the way of the multiarch conversion,
> so I'd like to remove it.
> 
> I looked at the only place where PUSH_DUMMY_FRAME is used, and found
> the following code (function hand_function_call in valops.c):
> 
>     /* A cleanup for the inferior status.  Create this AFTER the retbuf
>        so that this can be discarded or applied without interfering with
>        the regbuf.  */
>     inf_status = save_inferior_status (1);
>     inf_status_cleanup = make_cleanup_restore_inferior_status (inf_status);

[tangent]

This makes a copy of the inferior's registers.

Hmm, this means that there are potentially two copies of the system 
registers - inf_status and dummy-frame's (but HP doesn't yet use dummy 
frames).

In fact, this means that all targets, even the ones that don't use 
generic dummy frames, are restoring their registers from a regcache and 
_not_ from the the register values pushed onto the stack!

>     /* PUSH_DUMMY_FRAME is responsible for saving the inferior registers
>        (and POP_FRAME for restoring them).  (At least on most machines)
>        they are saved on the stack in the inferior.  */
>     PUSH_DUMMY_FRAME;
> 
> So inf_status is obtained by a call to save_inferior_status. OK.
> 
> HP's push_dummy_frame uses it exclusively to hack in some of the
> registers.  A comment explains why this is necessary:
> 
>   /* Oh, what a hack.  If we're trying to perform an inferior call
>      while the inferior is asleep, we have to make sure to clear
>      the "in system call" bit in the flag register (the call will
>      start after the syscall returns, so we're no longer in the system
>      call!)  This state is kept in "inf_status", change it there.
>   
>      We also need a number of horrid hacks to deal with lossage in the
>      PC queue registers (apparently they're not valid when the in syscall
>      bit is set).  */


> They do it like this:
> 
>       write_inferior_status_register (inf_status, 0, int_buffer);
>       write_inferior_status_register (inf_status, PCOQ_HEAD_REGNUM, pc + 0);
>       write_inferior_status_register (inf_status, PCOQ_TAIL_REGNUM, pc + 4);

Joel,

I think I'm missing something pretty fundamental here.  This shouldn't 
work.  I can't figure out why it doesn't end up trying to do a write to 
a read-only copy of current_regcache?

- inf_status->registers is created using regcache_dup (current_regcache)
gdb_assert (inf_status->registers != current_registers) true?
gdb_assert (inf_status->registers->readonly_p) true?

- inf_status->registers->regcache_raw_write() is calling (you've got 
legacy code :-) legacy_write_register_gen(), but first it checks:
gdb_assert (!regcache->readonly_p);
gdb_assert (regcache == current_regcache);

- legacy_write_register_gen() is then both writing the value into the 
current_regcache and direct to the target.

i.e., those writes are hitting the target directly.

Any idea what I'm missing?

confused,
Andrew