[rfc] ``pc'' -> resume_addr?

[rfc] ``pc'' -> resume_addr?

[Andrew Cagney]

Hello,

GDB, in a number of places, refers to the ``pc'' - PC_REGNUM, frame->pc, 
  read_pc(), write_pc(), ...

I think this name choice was unfortunate.  It is too easy for a 
developer to confuse ``pc'' with the hardware ``pc''.

With this in mind, I'd like to propose a GDBspeak ``resume_addr''.  It 
is the address of the first instruction that will be executed when the 
target resumes.

thoughts?  better names?
Andrew

Re: [rfc] ``pc'' -> resume_addr?

[Kevin Buettner]

On Apr 11,  4:38pm, Andrew Cagney wrote:

> GDB, in a number of places, refers to the ``pc'' - PC_REGNUM, frame->pc, 
>   read_pc(), write_pc(), ...
> 
> I think this name choice was unfortunate.  It is too easy for a 
> developer to confuse ``pc'' with the hardware ``pc''.

Could you please explain further why you think the name choice was
unfortunate?

> With this in mind, I'd like to propose a GDBspeak ``resume_addr''.  It 
> is the address of the first instruction that will be executed when the 
> target resumes.

So, if I understand you correctly, you're suggesting the following
renaming:

    PC_REGNUM	==>	RESUME_ADDR_REGNUM
    frame->pc	==>	frame->resume_addr
    read_pc()	==>	read_resume_addr()
    write_pc()	==>	write_resume_addr()

Perhaps I've just gotten fond of ``pc'', but I don't really like any
of these.

Kevin

Re: [rfc] ``pc'' -> resume_addr?

[Andrew Cagney]

> On Apr 11,  4:38pm, Andrew Cagney wrote:
> 
> 
>> GDB, in a number of places, refers to the ``pc'' - PC_REGNUM, frame->pc, 
>> read_pc(), write_pc(), ...
>> 
>> I think this name choice was unfortunate.  It is too easy for a 
>> developer to confuse ``pc'' with the hardware ``pc''.
> 
> 
> Could you please explain further why you think the name choice was
> unfortunate?

I think the name ``pc'' brings with it a certain amount of baggage. 
When reading a piece of code, it isn't clear if the hardware ``pc'' 
(possibly needing adjustment) or the program's resume address is being used.

On an x86, and m68k, for instance, the hardware PC may or may not need 
to be adjusted (decr_pc_after_break()) before it becomes a frame->pc.

Within the frame, the ``pc'' designates ``resume'' address of the 
function.  Knowing this is important when understanding why some of the 
frame code does:

	if (frame->next != NULL)
	  return frame->pc - 1;
	else
	  return frame->pc;

>> With this in mind, I'd like to propose a GDBspeak ``resume_addr''.  It 
>> is the address of the first instruction that will be executed when the 
>> target resumes.
> 
> 
> So, if I understand you correctly, you're suggesting the following
> renaming:
> 
>     PC_REGNUM	==> RESUME_ADDR_REGNUM

This wouldn't change.  If the hardware has a ``PC'' like register then 
likely the maintainer will retain ``PC_REGNUM'' / $pc as an alias for it.

>     frame->pc	==> frame->resume_addr

This, I think, should change.  I'm 99% sure that this isn't the hardware 
PC but rather the continue address for the frame (but notice I'm not 
100% sure thanks to its poor definition).

>     read_pc()	==> read_resume_addr()

This one is harder.  Perhaphs it can be eliminated.

>     write_pc()	==> write_resume_addr()

Check the default implementation.  It not only modifies PC, but also NPC 
and even NNPC.  I think this function should be called something like - 
set_resume_address()?

Remember, when making an inferior function call, GDB does not set the 
PC.  Rather it sets the resume/continue address using the debug info. 
For instance, on the sparc, it sets:

	[PC] = resume_addr;
	[NPC] = resume_addr + 4;

This behavour is very different to what the user is trying to achieve if 
they enter:

	(gdb) jump *foo *bar

On a sparc, that would execute:

	*foo
	*bar
	*(bar + 4)
	*(bar + 8)

> Perhaps I've just gotten fond of ``pc'', but I don't really like any
> of these.

If someone uses PC in a typical e-mail, we'll know what they mean. 
However, if someone uses PC when refering to GDB's internals, I don't 
know that we'll be as sure.

enjoy,
Andrew

Re: [rfc] ``pc'' -> resume_addr?

[Andrew Cagney]

>     frame->pc    ==> frame->resume_addr
> 
> This, I think, should change.  I'm 99% sure that this isn't the hardware PC but rather the continue address for the frame (but notice I'm not 100% sure thanks to its poor definition).

Hmm, there is another approach here.  As with frame_base() from my 
previous post, a more dynamic:

	frame_resume_addr (frame)

that would let the ISA code compute it on demand using the frame's 
register information - in theory frame->pc could be removed.

Andrew

Re: [rfc] ``pc'' -> resume_addr?

[Kevin Buettner]

On Apr 11,  6:16pm, Andrew Cagney wrote:

> >> I think this name choice was unfortunate.  It is too easy for a 
> >> developer to confuse ``pc'' with the hardware ``pc''.
> > 
> > Could you please explain further why you think the name choice was
> > unfortunate?
> 
> I think the name ``pc'' brings with it a certain amount of baggage. 
> When reading a piece of code, it isn't clear if the hardware ``pc'' 
> (possibly needing adjustment) or the program's resume address is being used.

Is there really that much confusion about this though?  I think that
the length of time during which this confusion exists is mercifully
brief.  AFAICT, gdb's notion of the pc is adjusted shortly after
stopping via a call to bpstat_stop_status() and from then on, the pc
value is simply the continuation (or resume) address.  Even the "PC"
value in the register cache has been adjusted to be the program's
resume address at this point.

I think it'd be better to carefully document the places in gdb where
the pc value hasn't yet been adjusted to be the continuation address.

> On an x86, and m68k, for instance, the hardware PC may or may not need 
> to be adjusted (decr_pc_after_break()) before it becomes a frame->pc.
>
> Within the frame, the ``pc'' designates ``resume'' address of the 
> function.  Knowing this is important when understanding why some of the 
> frame code does:
> 
> 	if (frame->next != NULL)
> 	  return frame->pc - 1;
> 	else
> 	  return frame->pc;

Where does this code occur?  I'd like to take a closer look...

[...]
> >     frame->pc	==> frame->resume_addr
> 
> This, I think, should change.  I'm 99% sure that this isn't the hardware 
> PC but rather the continue address for the frame (but notice I'm not 
> 100% sure thanks to its poor definition).

I don't disagree with your characterization of it, but I also don't
have a problem with continuing to call it ``pc''.  Only the topmost
frame will have a value that corresponds to the actual hardware pc
(modulo some adjustment perhaps).  All of the other frames will have
pc values which corresond to return addresses.  But this is okay. 
When considering the execution context provided by a particular frame,
``pc'' does correspond to what the hardware pc would be if that frame
were executing.

> >     read_pc()	==> read_resume_addr()
> 
> This one is harder.  Perhaphs it can be eliminated.

I think it may prove somewhat difficult to eliminate read_pc().

> >     write_pc()	==> write_resume_addr()
> 
> Check the default implementation.  It not only modifies PC, but also NPC 
> and even NNPC.  I think this function should be called something like - 
> set_resume_address()?

I agree with this.

Kevin

Re: [rfc] ``pc'' -> resume_addr?

[Kevin Buettner]

On Apr 11,  7:35pm, Andrew Cagney wrote:
> Subject: Re: [rfc] ``pc'' -> resume_addr?
> >     frame->pc    ==> frame->resume_addr
> > 
> > This, I think, should change.  I'm 99% sure that this isn't the
> > hardware PC but rather the continue address for the frame (but
> > notice I'm not 100% sure thanks to its poor definition).
> 
> Hmm, there is another approach here.  As with frame_base() from my 
> previous post, a more dynamic:
> 
> 	frame_resume_addr (frame)
> 
> that would let the ISA code compute it on demand using the frame's 
> register information - in theory frame->pc could be removed.

Sure.

But what are the advantages of doing it this way?  Is there a reason
it needs to be this dynamic?

Kevin

Re: [rfc] ``pc'' -> resume_addr?

[Andrew Cagney]

> On Apr 11,  7:35pm, Andrew Cagney wrote:
> 
>> Subject: Re: [rfc] ``pc'' -> resume_addr?
> 
>> > frame->pc    ==> frame->resume_addr
>> > 
>> > This, I think, should change.  I'm 99% sure that this isn't the
>> > hardware PC but rather the continue address for the frame (but
>> > notice I'm not 100% sure thanks to its poor definition).
> 
>> 
>> Hmm, there is another approach here.  As with frame_base() from my 
>> previous post, a more dynamic:
>> 
>> frame_resume_addr (frame)
>> 
>> that would let the ISA code compute it on demand using the frame's 
>> register information - in theory frame->pc could be removed.
> 
> 
> Sure.
> 
> But what are the advantages of doing it this way?  Is there a reason
> it needs to be this dynamic?

If it's computation is significant (target overhead), and its value 
isn't needed immediatly, then yes.

Consider the situtation JimI described.  An inferior function call 
invalidates the frame cache.  To re-find the selected frame after that 
call, GDB needs to re-create each frame until it locates the selected 
one.  During the search, the only thing that absolutly needs to be 
computed for each frame, is frame->addr.

enjoy,
Andrew

Re: [rfc] ``pc'' -> resume_addr?

[Michael Snyder]

Andrew Cagney wrote:
> 
> Hello,
> 
> GDB, in a number of places, refers to the ``pc'' - PC_REGNUM, frame->pc,
>   read_pc(), write_pc(), ...
> 
> I think this name choice was unfortunate.  It is too easy for a
> developer to confuse ``pc'' with the hardware ``pc''.
> 
> With this in mind, I'd like to propose a GDBspeak ``resume_addr''.  It
> is the address of the first instruction that will be executed when the
> target resumes.

Under what circumstances is the address of the first instruction 
that will be executed different from the hardware pc?

Re: [rfc] ``pc'' -> resume_addr?

[Michael Snyder]

Andrew Cagney wrote:
> 
> > On Apr 11,  4:38pm, Andrew Cagney wrote:
> >
> >
> >> GDB, in a number of places, refers to the ``pc'' - PC_REGNUM, frame->pc,
> >> read_pc(), write_pc(), ...
> >>
> >> I think this name choice was unfortunate.  It is too easy for a
> >> developer to confuse ``pc'' with the hardware ``pc''.
> >
> >
> > Could you please explain further why you think the name choice was
> > unfortunate?
> 
> I think the name ``pc'' brings with it a certain amount of baggage.
> When reading a piece of code, it isn't clear if the hardware ``pc''
> (possibly needing adjustment) or the program's resume address is being used.

When are they not the same?

> On an x86, and m68k, for instance, the hardware PC may or may not need
> to be adjusted (decr_pc_after_break()) before it becomes a frame->pc.

Yeah -- but this is done almost immediately after the target stops.
Past that point, the hardware pc _is_ equal to the address at which
execution will resume.  Before that point, we haven't really built
or used very many of these objects called 'pc' or 'something->pc'.
Have we?


> Within the frame, the ``pc'' designates ``resume'' address of the
> function.  Knowing this is important when understanding why some of the
> frame code does:
> 
>         if (frame->next != NULL)
>           return frame->pc - 1;
>         else
>           return frame->pc;

Uggh.  Where does THAT code come from?   ;-)

> >> With this in mind, I'd like to propose a GDBspeak ``resume_addr''.  It
> >> is the address of the first instruction that will be executed when the
> >> target resumes.
> >
> >
> > So, if I understand you correctly, you're suggesting the following
> > renaming:
> >
> >     PC_REGNUM ==> RESUME_ADDR_REGNUM
> 
> This wouldn't change.  If the hardware has a ``PC'' like register then
> likely the maintainer will retain ``PC_REGNUM'' / $pc as an alias for it.
> 
> >     frame->pc ==> frame->resume_addr
> 
> This, I think, should change.  I'm 99% sure that this isn't the hardware
> PC but rather the continue address for the frame (but notice I'm not
> 100% sure thanks to its poor definition).

OK, but in that context, it's supposed to be understood that 
the data structure "frame" contains values for the registers
in that frame's context -- not the actual hardware registers.
They're all saved values -- not just the pc.


> >     read_pc() ==> read_resume_addr()
> 
> This one is harder.  Perhaphs it can be eliminated.
> 
> >     write_pc()        ==> write_resume_addr()
> 
> Check the default implementation.  It not only modifies PC, but also NPC
> and even NNPC.  I think this function should be called something like -
> set_resume_address()?

I can see that ...


> Remember, when making an inferior function call, GDB does not set the
> PC.  Rather it sets the resume/continue address using the debug info.
> For instance, on the sparc, it sets:
> 
>         [PC] = resume_addr;
>         [NPC] = resume_addr + 4;
> 
> This behavour is very different to what the user is trying to achieve if
> they enter:
> 
>         (gdb) jump *foo *bar
> 
> On a sparc, that would execute:
> 
>         *foo
>         *bar
>         *(bar + 4)
>         *(bar + 8)

Whoa, you lost me.  The "jump" command only accepts one argument.
What does "jump *foo *bar" mean?

> > Perhaps I've just gotten fond of ``pc'', but I don't really like any
> > of these.
> 
> If someone uses PC in a typical e-mail, we'll know what they mean.
> However, if someone uses PC when refering to GDB's internals, I don't
> know that we'll be as sure.
> 
> enjoy,
> Andrew

Re: [rfc] ``pc'' -> resume_addr?

[Michael Snyder]

Andrew Cagney wrote:
> 
> >     frame->pc    ==> frame->resume_addr
> >
> > This, I think, should change.  I'm 99% sure that this isn't the hardware PC but rather the continue address for the frame (but notice I'm not 100% sure thanks to its poor definition).
> 
> Hmm, there is another approach here.  As with frame_base() from my
> previous post, a more dynamic:
> 
>         frame_resume_addr (frame)
> 
> that would let the ISA code compute it on demand using the frame's
> register information 

I think that already exists, in the form of frame_saved_pc().

Re: [rfc] ``pc'' -> resume_addr?

[Michael Snyder]

Kevin Buettner wrote:
> 
> On Apr 11,  6:16pm, Andrew Cagney wrote:
> 
> > >> I think this name choice was unfortunate.  It is too easy for a
> > >> developer to confuse ``pc'' with the hardware ``pc''.
> > >
> > > Could you please explain further why you think the name choice was
> > > unfortunate?
> >
> > I think the name ``pc'' brings with it a certain amount of baggage.
> > When reading a piece of code, it isn't clear if the hardware ``pc''
> > (possibly needing adjustment) or the program's resume address is being used.
> 
> Is there really that much confusion about this though?  I think that
> the length of time during which this confusion exists is mercifully
> brief.  AFAICT, gdb's notion of the pc is adjusted shortly after
> stopping via a call to bpstat_stop_status() and from then on, the pc
> value is simply the continuation (or resume) address.  Even the "PC"
> value in the register cache has been adjusted to be the program's
> resume address at this point.

And since the register cache is write-thru, so has the child's PC
register.

Re: [rfc] ``pc'' -> resume_addr?

[Andrew Cagney]

>> I think the name ``pc'' brings with it a certain amount of baggage.
>> When reading a piece of code, it isn't clear if the hardware ``pc''
>> (possibly needing adjustment) or the program's resume address is being used.
> 
> 
> When are they not the same?

I'm not sure.  I'm also not the only one :-) cf:

/* Are we in a call dummy?  The code below which allows DECR_PC_AFTER_BREAK
    below is for infrun.c, which may give the macro a pc without that
    subtracted out.  */

int
pc_in_call_dummy_before_text_end (CORE_ADDR pc, CORE_ADDR sp,
				  CORE_ADDR frame_address)
{
   return ((pc) >= text_end - CALL_DUMMY_LENGTH
	  && (pc) <= text_end + DECR_PC_AFTER_BREAK);
}

In reading the code, I never know if I've got a ``pc'' or a ``pc''.  If 
I print the value and it isn't as expected, I'm left wondering if I 
should adjust it, or that something somewhere else forgot to adjust it.

>> On an x86, and m68k, for instance, the hardware PC may or may not need
>> to be adjusted (decr_pc_after_break()) before it becomes a frame->pc.
> 
> 
> Yeah -- but this is done almost immediately after the target stops.
> Past that point, the hardware pc _is_ equal to the address at which
> execution will resume.  Before that point, we haven't really built
> or used very many of these objects called 'pc' or 'something->pc'.
> Have we?

Ok, I think I've figured out how this ``works'':

- target stops

- infrun has an initial look round.  If it doesn't think the read_pc() 
value is interesting, it resumes the target.  Thing is, I can't detect 
any write_pc() for this case - as best as I can tell it resumes the 
target without adjusting the PC.

- Otherwise infrun calls bpstat_stop_status() and that will eventually call:
	write_pc(read_pc()-decr);
and patch up that PC value. Ulgh!

- finally the block/frame is created.   This calls read_pc() and gets 
the ``fixed'' pc.

I don't know if this can really be called ``immediatly after'' the 
target stops.  infrun.c, for instance, can call functions (such as 
pc_in_call_dummy) with either [PC] or [PC]-DECR_PC_AFTER_BREAK.

I think even more troubling (and something I didn't realize) is that the 
meaning of read_pc() its self changes as you step through the code: 
read_pc() != read_pc().

>> Within the frame, the ``pc'' designates ``resume'' address of the
>> function.  Knowing this is important when understanding why some of the
>> frame code does:
>> 
>> if (frame->next != NULL)
>> return frame->pc - 1;
>> else
>> return frame->pc;
> 
> 
> Uggh.  Where does THAT code come from?   ;-)

blockframe.c:get_frame_block() (it is lightly different).  The code is 
wierd but correct.

>> Remember, when making an inferior function call, GDB does not set the
>> PC.  Rather it sets the resume/continue address using the debug info.
>> For instance, on the sparc, it sets:
>> 
>> [PC] = resume_addr;
>> [NPC] = resume_addr + 4;
>> 
>> This behavour is very different to what the user is trying to achieve if
>> they enter:
>> 
>> (gdb) jump *foo *bar
>> 
>> On a sparc, that would execute:
>> 
>> *foo
>> *bar
>> *(bar + 4)
>> *(bar + 8)
> 
> 
> Whoa, you lost me.  The "jump" command only accepts one argument.
> What does "jump *foo *bar" mean?

Sorry, I'm wrong.  I thought that was implemented but (fortunatly) it 
isn't. The semantics would be:  [PC] = '*foo'.  [NPC] = '*bar'.

--

I guess there are several things here:

	frame->pc
		this is the resume address (always?)
	read_pc()
		thanks to decr_pc_after_break, this
		isn't defined.
	decr_pc_after_break
		the real villian


enjoy,
Andrew

Re: [rfc] ``pc'' -> resume_addr?

[Andrew Cagney]

It gets better ...

> Ok, I think I've figured out how this ``works'':
> 
> - target stops
> 
> - infrun has an initial look round.  If it doesn't think the read_pc() value is interesting, it resumes the target.  Thing is, I can't detect any write_pc() for this case - as best as I can tell it resumes the target without adjusting the PC.

Add:
- in looking around, it might create a frame (for PC_IN_CALL_DUMMY()) 
and that frame's PC won't have been adjusted.

> - Otherwise infrun calls bpstat_stop_status() and that will eventually call:
>     write_pc(read_pc()-decr);
> and patch up that PC value. Ulgh!

> - finally the block/frame is created.   This calls read_pc() and gets the ``fixed'' pc.

Add:
- since the frame (with a botched PC) may have previously been created, 
the top level frame->pc needs to be patched:

   /* Make sure that the current_frame's pc is correct.  This
      is a correction for setting up the frame info before doing
      DECR_PC_AFTER_BREAK */
   if (target_has_execution && get_current_frame ())
     (get_current_frame ())->pc = read_pc ();

:-)

Andrew

Re: [rfc] ``pc'' -> resume_addr?

[Andrew Cagney]

> Hello,
> 
> GDB, in a number of places, refers to the ``pc'' - PC_REGNUM, frame->pc,  read_pc(), write_pc(), ...
> 
> I think this name choice was unfortunate.  It is too easy for a developer to confuse ``pc'' with the hardware ``pc''.
> 
> With this in mind, I'd like to propose a GDBspeak ``resume_addr''.  It is the address of the first instruction that will be executed when the target resumes.
> 
> thoughts?  better names?
> Andrew 

To follow this up.  My take on this is:

The real culprit is DECR_PC_AFTER_BREAK.  That needs to be eliminated. 
If that goes, much of the PC confusion will go with it.  Me trying to 
rename ->pc is like applying window dressing :-)

Secondary to this, there is read_pc() vs PC_REGNUM. (At the risk of 
starting another debate :-) Perhaphs, I should rename PC_REGNUM to 
HW_PC_REGNUM (same for FP and SP and PSW). That way it is clear that 
these regnum's can only map onto hardware registers.

enjoy,
Andrew