unnecessary aarch64_write_pc ?

unnecessary aarch64_write_pc ?

[Joel Brobecker]

Hello,

I noticed the following code:

> /* Implement the "write_pc" gdbarch method.  */
> 
> static void
> aarch64_write_pc (struct regcache *regcache, CORE_ADDR pc)
> {
>   regcache_cooked_write_unsigned (regcache, AARCH64_PC_REGNUM, pc);
> }

But looking at the only use of the gdbarch_write_pc function, I see:

> void
> regcache_write_pc (struct regcache *regcache, CORE_ADDR pc)
> {
>   struct gdbarch *gdbarch = get_regcache_arch (regcache);
> 
>   if (gdbarch_write_pc_p (gdbarch))
>     gdbarch_write_pc (gdbarch, regcache, pc);
>   else if (gdbarch_pc_regnum (gdbarch) >= 0)
>     regcache_cooked_write_unsigned (regcache,
>                                     gdbarch_pc_regnum (gdbarch), pc);
>   else
>     internal_error (__FILE__, __LINE__,
>                     _("regcache_write_pc: Unable to update PC"));
> 
>   /* Writing the PC (for instance, from "load") invalidates the
>      current frame.  */
>   reinit_frame_cache ();
> }

And gdbarch_pc_regnum is AARCH64_PC_REGNUM.

So it looks like we shouldn't need aarch64_write_pc?

I am reviewing the other definitions, and so far, they do seem to be
necessary.

-- 
Joel

Re: unnecessary aarch64_write_pc ?

[Yufeng Zhang]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

Hello Joel,

On 04/15/13 13:50, Joel Brobecker wrote:
> I noticed the following code:
>
>> /* Implement the "write_pc" gdbarch method.  */
>>
>> static void
>> aarch64_write_pc (struct regcache *regcache, CORE_ADDR pc)
>> {
>>    regcache_cooked_write_unsigned (regcache, AARCH64_PC_REGNUM, pc);
>> }
>
> But looking at the only use of the gdbarch_write_pc function, I see:
>
>> void
>> regcache_write_pc (struct regcache *regcache, CORE_ADDR pc)
>> {
>>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>>
>>    if (gdbarch_write_pc_p (gdbarch))
>>      gdbarch_write_pc (gdbarch, regcache, pc);
>>    else if (gdbarch_pc_regnum (gdbarch)>= 0)
>>      regcache_cooked_write_unsigned (regcache,
>>                                      gdbarch_pc_regnum (gdbarch), pc);
>>    else
>>      internal_error (__FILE__, __LINE__,
>>                      _("regcache_write_pc: Unable to update PC"));
>>
>>    /* Writing the PC (for instance, from "load") invalidates the
>>       current frame.  */
>>    reinit_frame_cache ();
>> }
>
> And gdbarch_pc_regnum is AARCH64_PC_REGNUM.
>
> So it looks like we shouldn't need aarch64_write_pc?

Thanks for the heads up; yes, aarch64_write_pc doesn't appear necessary. 
  I've prepared the attached patch to remove the function.

Thanks,
Yufeng

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 998 bytes --]

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index ebc78d6..2b03106 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2499,14 +2499,6 @@ aarch64_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
   gdb_assert_not_reached ("regnum out of bound");
 }
 
-/* Implement the "write_pc" gdbarch method.  */
-
-static void
-aarch64_write_pc (struct regcache *regcache, CORE_ADDR pc)
-{
-  regcache_cooked_write_unsigned (regcache, AARCH64_PC_REGNUM, pc);
-}
-
 /* Callback function for user_reg_add.  */
 
 static struct value *
@@ -2618,8 +2610,6 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_push_dummy_call (gdbarch, aarch64_push_dummy_call);
   set_gdbarch_frame_align (gdbarch, aarch64_frame_align);
 
-  set_gdbarch_write_pc (gdbarch, aarch64_write_pc);
-
   /* Frame handling.  */
   set_gdbarch_dummy_id (gdbarch, aarch64_dummy_id);
   set_gdbarch_unwind_pc (gdbarch, aarch64_unwind_pc);

Re: unnecessary aarch64_write_pc ?

[Joel Brobecker]

> Thanks for the heads up; yes, aarch64_write_pc doesn't appear
> necessary.  I've prepared the attached patch to remove the function.

Awesome, thanks for confirming. The patch is approved if you throw in
a ChangeLog entry :).

> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index ebc78d6..2b03106 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -2499,14 +2499,6 @@ aarch64_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
>    gdb_assert_not_reached ("regnum out of bound");
>  }
>  
> -/* Implement the "write_pc" gdbarch method.  */
> -
> -static void
> -aarch64_write_pc (struct regcache *regcache, CORE_ADDR pc)
> -{
> -  regcache_cooked_write_unsigned (regcache, AARCH64_PC_REGNUM, pc);
> -}
> -
>  /* Callback function for user_reg_add.  */
>  
>  static struct value *
> @@ -2618,8 +2610,6 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    set_gdbarch_push_dummy_call (gdbarch, aarch64_push_dummy_call);
>    set_gdbarch_frame_align (gdbarch, aarch64_frame_align);
>  
> -  set_gdbarch_write_pc (gdbarch, aarch64_write_pc);
> -
>    /* Frame handling.  */
>    set_gdbarch_dummy_id (gdbarch, aarch64_dummy_id);
>    set_gdbarch_unwind_pc (gdbarch, aarch64_unwind_pc);


-- 
Joel

Re: unnecessary aarch64_write_pc ?

[Yufeng Zhang]

On 04/17/13 15:37, Joel Brobecker wrote:
>> Thanks for the heads up; yes, aarch64_write_pc doesn't appear
>> necessary.  I've prepared the attached patch to remove the function.
>
> Awesome, thanks for confirming. The patch is approved if you throw in
> a ChangeLog entry :).

Thanks for the approval.  The patch has been committed with the 
following changelog entry:

	* aarch64-tdep.c (aarch64_write_pc): Removed.
	(aarch64_gdbarch_init): Remove set_gdbarch_write_pc of the above function.

Thanks,
Yufeng

Re: unnecessary aarch64_write_pc ?

[Joel Brobecker]

> Thanks for the approval.  The patch has been committed with the
> following changelog entry:
> 
> 	* aarch64-tdep.c (aarch64_write_pc): Removed.
> 	(aarch64_gdbarch_init): Remove set_gdbarch_write_pc of the above function.

FYI: The last line was too long. I went to the actual ChangeLog to verify,
and by then, it was just simpler for me to fix...

-- 
Joel