diff mbox series

[v3,4/7] gdb/record: make record_full_history more c++-like

Message ID 20260515163706.3355686-5-guinevere@redhat.com
State New
Headers
Series refactor the internals of record-full |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply

Commit Message

Guinevere Larsen May 15, 2026, 4:37 p.m. UTC 
  Move some functions to being a method of record_full_history, instead of
floating functions.
---
 gdb/record-full.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Comments

Thiago Jung Bauermann May 16, 2026, 1:03 a.m. UTC | #1 
Hello Guinevere,

Regarding patch 3: Nice that you were able to turn the cleanup functions
into destructors!

Guinevere Larsen <guinevere@redhat.com> writes:

> Move some functions to being a method of record_full_history, instead of
> floating functions.
> ---
>  gdb/record-full.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)

Regarding this patch:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Schimpe, Christina May 26, 2026, 9:04 a.m. UTC | #2 
> -----Original Message-----
> From: Guinevere Larsen <guinevere@redhat.com>
> Sent: Freitag, 15. Mai 2026 18:37
> To: gdb-patches@sourceware.org
> Cc: Guinevere Larsen <guinevere@redhat.com>
> Subject: [PATCH v3 4/7] gdb/record: make record_full_history more c++-like
> 
> Move some functions to being a method of record_full_history, instead of
> floating functions.

Commit message:

- Shouldn't it be "a method of struct record_full_instruction" (also for the header)?
- it's only one function, so you could simply write the function name instead of "Move some functions" 

> ---
>  gdb/record-full.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/record-full.c b/gdb/record-full.c index
> 24e94eaf5da..8cabd6e9438 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -388,6 +388,10 @@ struct record_full_instruction
>    uint32_t insn_num;
>    std::optional<gdb_signal> sigval;
>    std::vector<record_full_entry> effects;
> +
> +  /* Execute the full instruction.  As a side effect, set
> +     record_full_stop_reason.  */
> +  void exec_insn (regcache *regcache);
>  };
> 
>  /* If true, query if PREC cannot record memory @@ -843,12 +847,9 @@
> record_full_gdb_operation_disable_set (void)  static enum target_stop_reason
> record_full_stop_reason
>    = TARGET_STOPPED_BY_NO_REASON;
> 
> -static inline void
> -record_full_exec_insn (regcache *regcache,
> -		       gdbarch *gdbarch,
> -		       record_full_instruction &insn)
> +void record_full_instruction::exec_insn (regcache *regcache)
>  {
> -  for (auto &entry : insn.effects)
> +  for (auto &entry : effects)
>      if (entry.execute (regcache))
>        record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT;  } @@ -
> 1308,9 +1309,7 @@ record_full_wait_1 (struct target_ops *ops,
>  		  break;
>  		}
> 
> -	      record_full_exec_insn
> -		(regcache, gdbarch,
> -		 record_full_list[record_full_next_insn]);
> +	      record_full_list[record_full_next_insn].exec_insn (regcache);
> 
>  	      /* step */
>  	      if (record_full_resume_step)
> @@ -2503,8 +2502,7 @@ record_full_base_target::save_record (const char
> *recfilename)
> 
>    /* Reverse execute to the begin of record list.  */
>    for (int i = record_full_next_insn - 1; i >= 0; i--)
> -    record_full_exec_insn (regcache, gdbarch,
> -			   record_full_list[i]);
> +    record_full_list[i].exec_insn (regcache);
> 
>    /* Compute the size needed for the extra bfd section.  */
>    save_size = 4;	/* magic cookie */
> @@ -2576,7 +2574,7 @@ record_full_base_target::save_record (const char
> *recfilename)
>  	}
>        /* Execute entry.  */
>        if (i < record_full_next_insn)
> -	record_full_exec_insn (regcache, gdbarch, record_full_list[i]);
> +	record_full_list[i].exec_insn (regcache);
>      }
> 
>    unlink_file.keep ();
> @@ -2597,17 +2595,16 @@ record_full_goto_insn (size_t target_insn,
>    scoped_restore restore_operation_disable
>      = record_full_gdb_operation_disable_set ();
>    regcache *regcache = get_thread_regcache (inferior_thread ());
> -  struct gdbarch *gdbarch = regcache->arch ();
> 
>    /* Assume everything is valid: we will hit the entry,
>       and we will not hit the end of the recording.  */
> 
>    if (dir == EXEC_REVERSE)
>      for (int i = record_full_next_insn; i > target_insn; i--)
> -      record_full_exec_insn (regcache, gdbarch, record_full_list[i - 1]);
> +      record_full_list[i-1].exec_insn (regcache);

Nit: spaces missing in i-1 => i - i

>    else
>      for (int i = record_full_next_insn; i < target_insn; i++)
> -      record_full_exec_insn (regcache, gdbarch, record_full_list[i]);
> +      record_full_list[i].exec_insn (regcache);
>  }
> 
>  /* Alias for "target record-full".  */
> --
> 2.54.0
> 

With the nits fixed, LGTM.

Reviewed-By: Christina Schimpe <christina.schimpe@intel.com>

Christina

Intel Deutschland GmbH
Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Guinevere Larsen May 26, 2026, 8:07 p.m. UTC | #3 
On 5/26/26 6:04 AM, Schimpe, Christina wrote:
>> -----Original Message-----
>> From: Guinevere Larsen <guinevere@redhat.com>
>> Sent: Freitag, 15. Mai 2026 18:37
>> To: gdb-patches@sourceware.org
>> Cc: Guinevere Larsen <guinevere@redhat.com>
>> Subject: [PATCH v3 4/7] gdb/record: make record_full_history more c++-like
>>
>> Move some functions to being a method of record_full_history, instead of
>> floating functions.
> Commit message:
>
> - Shouldn't it be "a method of struct record_full_instruction" (also for the header)?
> - it's only one function, so you could simply write the function name instead of "Move some functions"
>
>> ---
>>   gdb/record-full.c | 25 +++++++++++--------------
>>   1 file changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/gdb/record-full.c b/gdb/record-full.c index
>> 24e94eaf5da..8cabd6e9438 100644
>> --- a/gdb/record-full.c
>> +++ b/gdb/record-full.c
>> @@ -388,6 +388,10 @@ struct record_full_instruction
>>     uint32_t insn_num;
>>     std::optional<gdb_signal> sigval;
>>     std::vector<record_full_entry> effects;
>> +
>> +  /* Execute the full instruction.  As a side effect, set
>> +     record_full_stop_reason.  */
>> +  void exec_insn (regcache *regcache);
>>   };
>>
>>   /* If true, query if PREC cannot record memory @@ -843,12 +847,9 @@
>> record_full_gdb_operation_disable_set (void)  static enum target_stop_reason
>> record_full_stop_reason
>>     = TARGET_STOPPED_BY_NO_REASON;
>>
>> -static inline void
>> -record_full_exec_insn (regcache *regcache,
>> -		       gdbarch *gdbarch,
>> -		       record_full_instruction &insn)
>> +void record_full_instruction::exec_insn (regcache *regcache)
>>   {
>> -  for (auto &entry : insn.effects)
>> +  for (auto &entry : effects)
>>       if (entry.execute (regcache))
>>         record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT;  } @@ -
>> 1308,9 +1309,7 @@ record_full_wait_1 (struct target_ops *ops,
>>   		  break;
>>   		}
>>
>> -	      record_full_exec_insn
>> -		(regcache, gdbarch,
>> -		 record_full_list[record_full_next_insn]);
>> +	      record_full_list[record_full_next_insn].exec_insn (regcache);
>>
>>   	      /* step */
>>   	      if (record_full_resume_step)
>> @@ -2503,8 +2502,7 @@ record_full_base_target::save_record (const char
>> *recfilename)
>>
>>     /* Reverse execute to the begin of record list.  */
>>     for (int i = record_full_next_insn - 1; i >= 0; i--)
>> -    record_full_exec_insn (regcache, gdbarch,
>> -			   record_full_list[i]);
>> +    record_full_list[i].exec_insn (regcache);
>>
>>     /* Compute the size needed for the extra bfd section.  */
>>     save_size = 4;	/* magic cookie */
>> @@ -2576,7 +2574,7 @@ record_full_base_target::save_record (const char
>> *recfilename)
>>   	}
>>         /* Execute entry.  */
>>         if (i < record_full_next_insn)
>> -	record_full_exec_insn (regcache, gdbarch, record_full_list[i]);
>> +	record_full_list[i].exec_insn (regcache);
>>       }
>>
>>     unlink_file.keep ();
>> @@ -2597,17 +2595,16 @@ record_full_goto_insn (size_t target_insn,
>>     scoped_restore restore_operation_disable
>>       = record_full_gdb_operation_disable_set ();
>>     regcache *regcache = get_thread_regcache (inferior_thread ());
>> -  struct gdbarch *gdbarch = regcache->arch ();
>>
>>     /* Assume everything is valid: we will hit the entry,
>>        and we will not hit the end of the recording.  */
>>
>>     if (dir == EXEC_REVERSE)
>>       for (int i = record_full_next_insn; i > target_insn; i--)
>> -      record_full_exec_insn (regcache, gdbarch, record_full_list[i - 1]);
>> +      record_full_list[i-1].exec_insn (regcache);
> Nit: spaces missing in i-1 => i - i
>
>>     else
>>       for (int i = record_full_next_insn; i < target_insn; i++)
>> -      record_full_exec_insn (regcache, gdbarch, record_full_list[i]);
>> +      record_full_list[i].exec_insn (regcache);
>>   }
>>
>>   /* Alias for "target record-full".  */
>> --
>> 2.54.0
>>
> With the nits fixed, LGTM.
>
> Reviewed-By: Christina Schimpe <christina.schimpe@intel.com>
Fixed all the nits! thanks for the review!
>
> Christina
>
> Intel Deutschland GmbH
> Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
> Tel: +49 89 991 430, www.intel.de
> Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Seat: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>
diff mbox series

Patch

diff --git a/gdb/record-full.c b/gdb/record-full.c
index 24e94eaf5da..8cabd6e9438 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -388,6 +388,10 @@  struct record_full_instruction
   uint32_t insn_num;
   std::optional<gdb_signal> sigval;
   std::vector<record_full_entry> effects;
+
+  /* Execute the full instruction.  As a side effect, set
+     record_full_stop_reason.  */
+  void exec_insn (regcache *regcache);
 };
 
 /* If true, query if PREC cannot record memory
@@ -843,12 +847,9 @@  record_full_gdb_operation_disable_set (void)
 static enum target_stop_reason record_full_stop_reason
   = TARGET_STOPPED_BY_NO_REASON;
 
-static inline void
-record_full_exec_insn (regcache *regcache,
-		       gdbarch *gdbarch,
-		       record_full_instruction &insn)
+void record_full_instruction::exec_insn (regcache *regcache)
 {
-  for (auto &entry : insn.effects)
+  for (auto &entry : effects)
     if (entry.execute (regcache))
       record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT;
 }
@@ -1308,9 +1309,7 @@  record_full_wait_1 (struct target_ops *ops,
 		  break;
 		}
 
-	      record_full_exec_insn
-		(regcache, gdbarch,
-		 record_full_list[record_full_next_insn]);
+	      record_full_list[record_full_next_insn].exec_insn (regcache);
 
 	      /* step */
 	      if (record_full_resume_step)
@@ -2503,8 +2502,7 @@  record_full_base_target::save_record (const char *recfilename)
 
   /* Reverse execute to the begin of record list.  */
   for (int i = record_full_next_insn - 1; i >= 0; i--)
-    record_full_exec_insn (regcache, gdbarch,
-			   record_full_list[i]);
+    record_full_list[i].exec_insn (regcache);
 
   /* Compute the size needed for the extra bfd section.  */
   save_size = 4;	/* magic cookie */
@@ -2576,7 +2574,7 @@  record_full_base_target::save_record (const char *recfilename)
 	}
       /* Execute entry.  */
       if (i < record_full_next_insn)
-	record_full_exec_insn (regcache, gdbarch, record_full_list[i]);
+	record_full_list[i].exec_insn (regcache);
     }
 
   unlink_file.keep ();
@@ -2597,17 +2595,16 @@  record_full_goto_insn (size_t target_insn,
   scoped_restore restore_operation_disable
     = record_full_gdb_operation_disable_set ();
   regcache *regcache = get_thread_regcache (inferior_thread ());
-  struct gdbarch *gdbarch = regcache->arch ();
 
   /* Assume everything is valid: we will hit the entry,
      and we will not hit the end of the recording.  */
 
   if (dir == EXEC_REVERSE)
     for (int i = record_full_next_insn; i > target_insn; i--)
-      record_full_exec_insn (regcache, gdbarch, record_full_list[i - 1]);
+      record_full_list[i-1].exec_insn (regcache);
   else
     for (int i = record_full_next_insn; i < target_insn; i++)
-      record_full_exec_insn (regcache, gdbarch, record_full_list[i]);
+      record_full_list[i].exec_insn (regcache);
 }
 
 /* Alias for "target record-full".  */