[2/8] gdb/s390: Fill write_guessed_tracepoint_pc hook.

  gdb/ChangeLog:

	* s390-linux-tdep.c (s390_write_guessed_tracepoint_pc): New function.
	(s390_gdbarch_init): Fill write_guessed_tracepoint_pc hook.
---
 gdb/ChangeLog         |  5 +++++
 gdb/s390-linux-tdep.c | 17 +++++++++++++++++
 2 files changed, 22 insertions(+)
  

On Sun, Jan 24 2016, Marcin Kościelnicki wrote:

> diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
> index e827684..c4d25d2 100644
> --- a/gdb/s390-linux-tdep.c
> +++ b/gdb/s390-linux-tdep.c
> @@ -167,6 +167,22 @@ s390_write_pc (struct regcache *regcache, CORE_ADDR pc)
>      regcache_cooked_write_unsigned (regcache, S390_SYSTEM_CALL_REGNUM, 0);
>  }
>  
> +static void
> +s390_write_guessed_tracepoint_pc (struct regcache *regcache, CORE_ADDR pc)

Please add documentation for this function, as described here:

  https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Document_Every_Subprogram

Note that this was not always done in the past; thus there are still
many undocumented functions in this file.  But for new functions we
should stick to it.

> +{
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +  int sz = register_size (gdbarch, S390_PSWA_REGNUM);
> +  gdb_byte *reg = (gdb_byte *) alloca (sz);
> +
> +  /* 31-bit PSWA needs high bit set.  */
> +  if (tdep->abi == ABI_LINUX_S390)
> +    pc |= 0x80000000;

This is done differently in s390_pseudo_register_write: the high bit is
copied from the original PSWA.  Of course, this only makes a difference
if a program ever switches to 24-bit mode (yikes).  I just wonder
whether both cases should be treated the same, or whether there's good
reason not to.

> +
> +  store_unsigned_integer (reg, sz, gdbarch_byte_order (gdbarch), pc);
> +  regcache_raw_supply (regcache, S390_PSWA_REGNUM, reg);
> +}
> +
>  
>  /* DWARF Register Mapping.  */
>  
> @@ -7857,6 +7873,7 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  					    s390_iterate_over_regset_sections);
>    set_gdbarch_cannot_store_register (gdbarch, s390_cannot_store_register);
>    set_gdbarch_write_pc (gdbarch, s390_write_pc);
> +  set_gdbarch_write_guessed_tracepoint_pc (gdbarch, s390_write_guessed_tracepoint_pc);
>    set_gdbarch_pseudo_register_read (gdbarch, s390_pseudo_register_read);
>    set_gdbarch_pseudo_register_write (gdbarch, s390_pseudo_register_write);
>    set_tdesc_pseudo_register_name (gdbarch, s390_pseudo_register_name);
  

On 26/01/16 19:12, Andreas Arnez wrote:
> On Sun, Jan 24 2016, Marcin Kościelnicki wrote:
>
>> diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
>> index e827684..c4d25d2 100644
>> --- a/gdb/s390-linux-tdep.c
>> +++ b/gdb/s390-linux-tdep.c
>> @@ -167,6 +167,22 @@ s390_write_pc (struct regcache *regcache, CORE_ADDR pc)
>>       regcache_cooked_write_unsigned (regcache, S390_SYSTEM_CALL_REGNUM, 0);
>>   }
>>
>> +static void
>> +s390_write_guessed_tracepoint_pc (struct regcache *regcache, CORE_ADDR pc)
>
> Please add documentation for this function, as described here:
>
>    https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Document_Every_Subprogram
>
> Note that this was not always done in the past; thus there are still
> many undocumented functions in this file.  But for new functions we
> should stick to it.

OK, will do.
>
>> +{
>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +  int sz = register_size (gdbarch, S390_PSWA_REGNUM);
>> +  gdb_byte *reg = (gdb_byte *) alloca (sz);
>> +
>> +  /* 31-bit PSWA needs high bit set.  */
>> +  if (tdep->abi == ABI_LINUX_S390)
>> +    pc |= 0x80000000;
>
> This is done differently in s390_pseudo_register_write: the high bit is
> copied from the original PSWA.  Of course, this only makes a difference
> if a program ever switches to 24-bit mode (yikes).  I just wonder
> whether both cases should be treated the same, or whether there's good
> reason not to.

If write_guessed_tracepoint_pc is called, we explicitely have no 
registers data other than PC itself - there's no original PSWA to speak 
of.  The state of PSWA high bit probably doesn't matter all that much, 
but since we want to mark PSWA as known, we'd better write the 
reasonable value to it.
>
>> +
>> +  store_unsigned_integer (reg, sz, gdbarch_byte_order (gdbarch), pc);
>> +  regcache_raw_supply (regcache, S390_PSWA_REGNUM, reg);
>> +}
>> +
>>
>>   /* DWARF Register Mapping.  */
>>
>> @@ -7857,6 +7873,7 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>   					    s390_iterate_over_regset_sections);
>>     set_gdbarch_cannot_store_register (gdbarch, s390_cannot_store_register);
>>     set_gdbarch_write_pc (gdbarch, s390_write_pc);
>> +  set_gdbarch_write_guessed_tracepoint_pc (gdbarch, s390_write_guessed_tracepoint_pc);
>>     set_gdbarch_pseudo_register_read (gdbarch, s390_pseudo_register_read);
>>     set_gdbarch_pseudo_register_write (gdbarch, s390_pseudo_register_write);
>>     set_tdesc_pseudo_register_name (gdbarch, s390_pseudo_register_name);
>
  

On Tue, Jan 26 2016, Marcin Kościelnicki wrote:

> If write_guessed_tracepoint_pc is called, we explicitely have no
> registers data other than PC itself - there's no original PSWA to
> speak of.  The state of PSWA high bit probably doesn't matter all that
> much, but since we want to mark PSWA as known, we'd better write the
> reasonable value to it.

That's true if the new gdbarch method implements a very specific
semantic, tailored to the singular use in tracefile_fetch_registers and
not necessarily usable for any other purposes.  But I interpreted the
semantic more generically: "Supply VAL as the new PC to the given
REGCACHE".

Maybe the generic approach could be implemented like this (completely
untested):

  /* Adjust PSWA high bit if in 31-bit mode.  */
  if (sz == 4)
    {
      if (regcache_register_status (regcache, S390_PSWA_REGNUM)
	  == REG_VALID)
	{
	  gdb_byte pswa[4];

	  regcache_raw_collect (regcache, S390_PSWA_REGNUM, pswa);
	  pc |= 0x80000000 & extract_unsigned_integer (pswa, 4, byte_order);
	}
      else
	pc |= 0x80000000;
    }

BTW, even this may be considered a special case of a hypothetical
gdbarch method pseudo_register_supply().  *Or* we might think of it as
writing to a "cache-only" (rather than read-only) regcache via a usual
regcache_cooked_write().  In this case no new gdbarch method would be
needed.  Such a feature might also come handy when trying to support the
modification of cooked registers during core file debugging, such as
discussed in this thread:

  https://www.sourceware.org/ml/gdb/2011-02/msg00042.html

--
Andreas