[PATCH] Fix for gdb.base/pc-fp.exp.

[PATCH] Fix for gdb.base/pc-fp.exp.

[Carl E. Love]

Fix for gdb.base/pc-fp.exp.

gdb/Changelog

2016-08-22  Carl Love  <cel@us.ibm.com>

	* rs6000-tdep.c (rs6000_gdbarch_init): Remove deprecated call set_gdbarch_deprecated_fp_regnum()
	from Power architecture initialization function.
---
 gdb/rs6000-tdep.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index eb12c5d..e180641 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -5957,7 +5957,6 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   set_gdbarch_pc_regnum (gdbarch, PPC_PC_REGNUM);
   set_gdbarch_sp_regnum (gdbarch, PPC_R0_REGNUM + 1);
-  set_gdbarch_deprecated_fp_regnum (gdbarch, PPC_R0_REGNUM + 1);
   set_gdbarch_fp0_regnum (gdbarch, tdep->ppc_fp0_regnum);
   set_gdbarch_register_sim_regno (gdbarch, rs6000_register_sim_regno);
 
-- 
1.8.3.1

Re: [PATCH] Fix for gdb.base/pc-fp.exp.

[Pedro Alves]

Can you provide more details?

E.g.:

What's was wrong?  What failed?  Why is removing this line the
right fix?

I'm not suggesting that the fix is wrong (or right, I have no
idea).  Just pointing out that context is missing.

Thanks,
Pedro Alves

Re: [PATCH] Fix for gdb.base/pc-fp.exp.

[Carl E. Love]

On Tue, 2016-08-23 at 00:17 +0100, Pedro Alves wrote:
> Can you provide more details?
> 
> E.g.:
> 
> What's was wrong?  What failed?  Why is removing this line the
> right fix?
> 
> I'm not suggesting that the fix is wrong (or right, I have no
> idea).  Just pointing out that context is missing.
> 
> Thanks,
> Pedro Alves
> 



Here is an updated patch with the missing detail.
--------------------------------------------------------------------------
Fix for gdb.base/pc-fp.exp.

It is my understanding that GDB used to require each architecture to
define a Frame Pointer (fp).  However, this functionality was deprecated
some time ago so the call to setup the fp_reg was changed to deprecated
(set_gdbarch_deprecated_fp_regnum).  It should have been removed from the
Power code.

That said, the code "set_gdbarch_deprecated_fp_regnum
(gdbarch, PPC_R0_REGNUM + 1);" sets up register r1 as the frame pointer.
Register r1 is no longer used to hold the frame pointer on Power.  By
removing the fp definition for Power in GDB, it causes GDB to fall back
to the call get_frame_base_address (frame) which returns the correct value
depending on the specific senario but most of the time is the DWARF
canonical frame address.

gdb/ChangeLog

2016-08-22  Carl Love  <cel@us.ibm.com>

	* rs6000-tdep.c (rs6000_gdbarch_init): Remove deprecated call set_gdbarch_deprecated_fp_regnum()
	from Power architecture initialization function.
---
 gdb/rs6000-tdep.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index a616cbe..db63be0 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -5957,7 +5957,6 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   set_gdbarch_pc_regnum (gdbarch, PPC_PC_REGNUM);
   set_gdbarch_sp_regnum (gdbarch, PPC_R0_REGNUM + 1);
-  set_gdbarch_deprecated_fp_regnum (gdbarch, PPC_R0_REGNUM + 1);
   set_gdbarch_fp0_regnum (gdbarch, tdep->ppc_fp0_regnum);
   set_gdbarch_register_sim_regno (gdbarch, rs6000_register_sim_regno);
 
-- 
1.8.3.1

Re: [PATCH] Fix for gdb.base/pc-fp.exp.

[Luis Machado]

On 08/23/2016 11:17 AM, Carl E. Love wrote:
> On Tue, 2016-08-23 at 00:17 +0100, Pedro Alves wrote:
>> Can you provide more details?
>>
>> E.g.:
>>
>> What's was wrong?  What failed?  Why is removing this line the
>> right fix?
>>
>> I'm not suggesting that the fix is wrong (or right, I have no
>> idea).  Just pointing out that context is missing.
>>
>> Thanks,
>> Pedro Alves
>>
>
>
>
> Here is an updated patch with the missing detail.
> --------------------------------------------------------------------------
> Fix for gdb.base/pc-fp.exp.
>
> It is my understanding that GDB used to require each architecture to
> define a Frame Pointer (fp).  However, this functionality was deprecated
> some time ago so the call to setup the fp_reg was changed to deprecated
> (set_gdbarch_deprecated_fp_regnum).  It should have been removed from the
> Power code.
>
> That said, the code "set_gdbarch_deprecated_fp_regnum
> (gdbarch, PPC_R0_REGNUM + 1);" sets up register r1 as the frame pointer.
> Register r1 is no longer used to hold the frame pointer on Power.  By
> removing the fp definition for Power in GDB, it causes GDB to fall back
> to the call get_frame_base_address (frame) which returns the correct value
> depending on the specific senario but most of the time is the DWARF
> canonical frame address.

Is this the case for all Power ABI's or only server? I wonder what the 
impact would be on Power embedded.

Re: [PATCH] Fix for gdb.base/pc-fp.exp.

[Ulrich Weigand]

Carl Love wrote:

> That said, the code "set_gdbarch_deprecated_fp_regnum
> (gdbarch, PPC_R0_REGNUM + 1);" sets up register r1 as the frame pointer.
> Register r1 is no longer used to hold the frame pointer on Power.  By
> removing the fp definition for Power in GDB, it causes GDB to fall back
> to the call get_frame_base_address (frame) which returns the correct value
> depending on the specific senario but most of the time is the DWARF
> canonical frame address.

That's true.  In fact, the one place that really cares about the value
returned via $fp is varobj.c:find_frame_addr_in_frame_chain, since you're
supposed to use the value of $fp to identify a frame for MI routines.
And there we explicitly assume that this value matches the value returned
by get_frame_base_address.

> 2016-08-22  Carl Love  <cel@us.ibm.com>
> 
> 	* rs6000-tdep.c (rs6000_gdbarch_init): Remove deprecated call set_gdbarch_deprecated_fp_regnum()
> 	from Power architecture initialization function.

Please watch the 80-char line length limit, which holds for the ChangeLog
as well.  In fact, the text can be shortened anyway: "deprecated" is
redundant (included in the function name), and likewise is "from Power
architecture initialization function" (you already specified which
function --rs6000_gdbarch_init-- you're modifying).  I'd suggest:

 	* rs6000-tdep.c (rs6000_gdbarch_init): Remove call to
	set_gdbarch_deprecated_fp_regnum.

Patch is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix for gdb.base/pc-fp.exp.

[Ulrich Weigand]

Luis Machado wrote:
> On 08/23/2016 11:17 AM, Carl E. Love wrote:
> > It is my understanding that GDB used to require each architecture to
> > define a Frame Pointer (fp).  However, this functionality was deprecated
> > some time ago so the call to setup the fp_reg was changed to deprecated
> > (set_gdbarch_deprecated_fp_regnum).  It should have been removed from the
> > Power code.
> >
> > That said, the code "set_gdbarch_deprecated_fp_regnum
> > (gdbarch, PPC_R0_REGNUM + 1);" sets up register r1 as the frame pointer.
> > Register r1 is no longer used to hold the frame pointer on Power.  By
> > removing the fp definition for Power in GDB, it causes GDB to fall back
> > to the call get_frame_base_address (frame) which returns the correct value
> > depending on the specific senario but most of the time is the DWARF
> > canonical frame address.
> 
> Is this the case for all Power ABI's or only server? I wonder what the 
> impact would be on Power embedded.

This doesn't really have anything to do with the ABI.  As I said in the other
email, the only effect of set_gdbarch_deprecated_fp_regnum these days is to
affect what value GDB prints for $fp.  This has really no meaning for anything
except that MI front ends use it to identify stack frames: you examine a
frame's $fp value, and use it as argument to the -var-create MI command in
order to create a variable bound to this frame.  (And even that usage is
really questionably, and only remains in there to avoid incompatible changes
in the interface.  The "natural" way these days to identify a frame would
be via its frame ID.)

For this to work, the value of $fp must be the value of get_frame_base_address,
which means set_gdbarch_deprecated_fp_regnum must not be used.  And in fact
basically no targets do use it, except for rs6000 and frv, both of which
seem to be just incorrect.

(Note that in any case, the rs6000 back end sets deprecated_fp_regnum to 1,
which has never been the *frame pointer* register in any ABI, even those
that -sometimes- use one.  In fact, it is the *stack pointer* register ...)

(Also note that there is a second remaining use of deprecated_fp_regnum,
in legacy_virtual_frame_pointer.  This whole routine is really a hack and
probably doesn't work in any except the most trivial circumstances.  Even
so, Carl's change is a no-op for legacy_virtual_frame_pointer, since if
deprecated_fp_regnum isn't set, it will fall back to sp_regnum, which is
in fact also register 1 on rs6000.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix for gdb.base/pc-fp.exp.

[Luis Machado]

On 08/23/2016 12:06 PM, Ulrich Weigand wrote:
> Luis Machado wrote:
>> On 08/23/2016 11:17 AM, Carl E. Love wrote:
>>> It is my understanding that GDB used to require each architecture to
>>> define a Frame Pointer (fp).  However, this functionality was deprecated
>>> some time ago so the call to setup the fp_reg was changed to deprecated
>>> (set_gdbarch_deprecated_fp_regnum).  It should have been removed from the
>>> Power code.
>>>
>>> That said, the code "set_gdbarch_deprecated_fp_regnum
>>> (gdbarch, PPC_R0_REGNUM + 1);" sets up register r1 as the frame pointer.
>>> Register r1 is no longer used to hold the frame pointer on Power.  By
>>> removing the fp definition for Power in GDB, it causes GDB to fall back
>>> to the call get_frame_base_address (frame) which returns the correct value
>>> depending on the specific senario but most of the time is the DWARF
>>> canonical frame address.
>>
>> Is this the case for all Power ABI's or only server? I wonder what the
>> impact would be on Power embedded.
>
> This doesn't really have anything to do with the ABI.  As I said in the other
> email, the only effect of set_gdbarch_deprecated_fp_regnum these days is to
> affect what value GDB prints for $fp.  This has really no meaning for anything
> except that MI front ends use it to identify stack frames: you examine a
> frame's $fp value, and use it as argument to the -var-create MI command in
> order to create a variable bound to this frame.  (And even that usage is
> really questionably, and only remains in there to avoid incompatible changes
> in the interface.  The "natural" way these days to identify a frame would
> be via its frame ID.)
>
> For this to work, the value of $fp must be the value of get_frame_base_address,
> which means set_gdbarch_deprecated_fp_regnum must not be used.  And in fact
> basically no targets do use it, except for rs6000 and frv, both of which
> seem to be just incorrect.
>
> (Note that in any case, the rs6000 back end sets deprecated_fp_regnum to 1,
> which has never been the *frame pointer* register in any ABI, even those
> that -sometimes- use one.  In fact, it is the *stack pointer* register ...)
>
> (Also note that there is a second remaining use of deprecated_fp_regnum,
> in legacy_virtual_frame_pointer.  This whole routine is really a hack and
> probably doesn't work in any except the most trivial circumstances.  Even
> so, Carl's change is a no-op for legacy_virtual_frame_pointer, since if
> deprecated_fp_regnum isn't set, it will fall back to sp_regnum, which is
> in fact also register 1 on rs6000.)
>
> Bye,
> Ulrich
>

Fair enough. I was just checking what the outcome would be with Power 
Embedded and if it had been tested, since the original mail doesn't make 
it explicit what the failure mode is, nor what specific test is failing.

Re: [PATCH] Fix for gdb.base/pc-fp.exp.

[Carl E. Love]

On Tue, 2016-08-23 at 18:30 +0200, Ulrich Weigand wrote:
> Carl Love wrote:
> 
> > That said, the code "set_gdbarch_deprecated_fp_regnum
> > (gdbarch, PPC_R0_REGNUM + 1);" sets up register r1 as the frame pointer.
> > Register r1 is no longer used to hold the frame pointer on Power.  By
> > removing the fp definition for Power in GDB, it causes GDB to fall back
> > to the call get_frame_base_address (frame) which returns the correct value
> > depending on the specific senario but most of the time is the DWARF
> > canonical frame address.
> 
> That's true.  In fact, the one place that really cares about the value
> returned via $fp is varobj.c:find_frame_addr_in_frame_chain, since you're
> supposed to use the value of $fp to identify a frame for MI routines.
> And there we explicitly assume that this value matches the value returned
> by get_frame_base_address.
> 
> > 2016-08-22  Carl Love  <cel@us.ibm.com>
> > 
> > 	* rs6000-tdep.c (rs6000_gdbarch_init): Remove deprecated call set_gdbarch_deprecated_fp_regnum()
> > 	from Power architecture initialization function.
> 
> Please watch the 80-char line length limit, which holds for the ChangeLog
> as well.  In fact, the text can be shortened anyway: "deprecated" is
> redundant (included in the function name), and likewise is "from Power
> architecture initialization function" (you already specified which
> function --rs6000_gdbarch_init-- you're modifying).  I'd suggest:
> 
>  	* rs6000-tdep.c (rs6000_gdbarch_init): Remove call to
> 	set_gdbarch_deprecated_fp_regnum.
> 
> Patch is OK.
> 
> Thanks,
> Ulrich
> 

Ulrich, Luis, Pedro:

I haven't heard any additional discussion on this patch in the last 24
hours or so.  I believe at this point everyone's questions/concerns have
been addressed.  I have pushed the patch.  I did fix the line length
issue on the ChangeLog entry and updated the dates.  Thanks for the
input and help with this patch.

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=bdd78711b4c1ae26dbc8c2a64f28abec3486ae6c

                 Carl Love