Re: [PATCH 6/6] gdb/record: Define new version of the record-save section

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Guinevere Larsen <guinevere@redhat.com> writes:

> With the changes to the internal representation of the history, we can
> no longer support the previous record save format. This commit makes it
> official, documenting the new format and changing the magic number.
> ---
>  gdb/NEWS          |  3 +++
>  gdb/record-full.c | 35 +++++++++++++++++++++++++++++++----
>  2 files changed, 34 insertions(+), 4 deletions(-)

Assuming that the order switch between number of effect entries and
signal I mention below is fixed (or I misunderstood it):

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 297b5b76ae7..f7cfe5ca1e9 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -76,7 +76,8 @@
>    ( (record_full_next_insn != record_full_list.size ()) \
>       || ::execution_direction == EXEC_REVERSE)
>  
> -#define RECORD_FULL_FILE_MAGIC	netorder32(0x20091016)
> +#define RECORD_FULL_FILE_MAGIC_OLD	netorder32(0x20091016)
> +#define RECORD_FULL_FILE_MAGIC	netorder32(0x20260415)

Out of curiosity: how was the magic number chosen? It doesn't seem to
based on ASCII. Is it worth adding a comment to explain it?

>  /* These are the core structs of the process record functionality.
>  
> @@ -2161,6 +2162,27 @@ record_full_core_target::has_execution (inferior *inf)
>         8 bytes: memory address (network byte order).
>         n bytes: memory value (n == memory length).
>  
> +   Version 3 (all numbers are in network order).
> +     4 bytes: Magic number (0x20260415).
> +       NOTE: be sure to change whenever this file format changes!
> +
> +    Records:
> +      record_full_instruction:
> +	4 bytes: number of reg and mem entries for this instruction.
> +	1 byte: signal.

Looking at the code in record_full_restore, signal comes before the
number of effect entries.

> +	4 bytes: instruction count.

Maybe it's just me or I'm a bit tired, but "instruction sequence number"
sounds clearer to me than "instruction count".

> +	4 bytes: PC register ID.

Considering that we know that it will be the PC in this "register slot",
it's slightly wasteful to include the register ID here. But I suppose
it's not a problem in practice.

> +	N bytes: PC address of instruction (N == size of PC).
> +	Effects:
> +	  record_full_reg:
> +	    1 byte: record_type (record_full_reg, see enum record_full_type).
> +	    4 bytes: Register ID.
> +	    n bytes: register value (n == actual register size).
> +	  record_full_mem:
> +	    1 byte: record_type (record_full_mem, see enum record_full_type).
> +	    4 bytes: memory length.
> +	    8 bytes: memory address.
> +	    n bytes: memory value (n = memory length).
>  */
>  
>  /* bfdcore_read -- read bytes from a core file section.  */

-- 
Thiago