Re: [PATCH 13/17] [PowerPC] Add support for HTM registers

["Ulrich Weigand" <uweigand@de.ibm.com>]

Pedro Franco de Carvalho wrote:

> The set of checkpointed general-purpose registers is returned by the
> linux kernel in the same format as the regular general-purpose
> registers, defined in struct pt_regs. However, the architecture
> specifies that only some of the registers present in pt_regs are
> checkpointed (GPRs 0-31, CR, XER, LR and CTR). The kernel fills the
> slots for other registers with other info (e.g., nip is filled with
> the contents of TFHAR). GDB doesn't handle these other registers. This
> means that core files generated by GDB will show values of zero for
> these registers, while core files generated by the kernel will have
> other values. Core files generated by the kernel have a note section
> for checkpointed GPRs with the same size for both 32-bit and 64-bit
> threads, and the values for the registers of a 32-bit thread are
> squeezed in the first half, with no useful data in the second
> half. GDB generates a smaller note section for 32-bit threads, but can
> read both sizes.

Well, this is weird.  Either the kernel should be fixed to use the
smaller size as well, or else GDB should also generate the large
section (and zero out the second half).

> Access to the checkpointed CR (condition register) can be
> confusing. The architecture only specifies that CR fields 1 to 7 (the
> 24 least significant bits) are checkpointed, but the kernel provides
> all 8 fields (32 bits). The value of field 0 is not masked by ptrace,
> so it will sometimes show the result of some kernel operation,
> probably treclaim., which sets this field.

Huh. We shold report this to the kernel people.

> The checkpointed registers are marked not to be saved and
> restored. Inferior function calls during an active transaction don't
> work well, and it's unclear what should be done in this case. TEXASR
> and TFIAR can be altered asynchronously, during transaction failure
> recording, so they are also not saved and restored. For consistency
> neither is TFHAR.
> 
> Record and replay also doesn't work well when transactions are
> involved. This patch doesn't address this, so the values of the HTM
> SPRs will sometimes be innacurate when the record/relay target is
> enabled. For instance, executing a "tbegin." alters TFHAR and TEXASR,
> but these changes are not currently recorded.
> 
> Because the checkpointed registers are only available when a
> transaction is active (or suspended), ptrace can return ENODATA when
> gdb tries to read these registers and the inferior is not in a
> transactional state. The registers are set to the unavailable state
> when this happens. When gbd tries to write to one of these registers,
> and it is unavailable, an error is raised. When gdb tries to store to
> all registers in one go (when store_registers called with -1), the
> state of these registers is checked. If they are all unavailable, no
> write is attempted, so that writes to all the other registers are
> unaffected. If all are available, the write is attempted. Otherwise an
> internal_error is raised.

This all makes sense to me.

> Like for the previous patches, configure.srv is updated here to avoid
> breaking the build at this patch.

Same comment as before :-)

> +		    tdep->wordsize == 4? PPC32_LINUX_SIZEOF_CGPRREGSET
> +		    : PPC64_LINUX_SIZEOF_CGPRREGSET,

Use parentheses for formatting.

> +/* Get the checkpointed GPR regset that matches the target wordsize
> +   and byteorder of GDBARCH.  The collect function of the returned
> +   regset will clear the buffer passed to it before collecting the
> +   registers to 0 if CLEAR_ON_COLLECT is true and the collect function
> +   is called with regno == -1 (collect all regs in the regset). This
> +   is useful for generating core files without garbage in the register
> +   slots that are not seen by gdb.  When collecting for storing the
> +   registers in the inferior, these slots should not be cleared.  */

I'm not sure I like this approach.  The problem is only about
generating core files, right?  Why don't we instead simply
change linux-tdep.c:linux_collect_regset_section_cb from

  buf = (char *) xmalloc (size);
  regset->collect_regset (regset, data->regcache, -1, buf, size);

to use xcalloc instead?  This seems prudent anyway.  (The same applies
to the similar code in fbsd-tdep.c.)

Then we can probably even get rid of ppc_linux_collect_vrregset as well.


>      return vsx_pseudo_register_read (gdbarch, regcache, reg_nr, buffer);
>    else if (IS_EFP_PSEUDOREG (tdep, reg_nr))
>      return efpr_pseudo_register_read (gdbarch, regcache, reg_nr, buffer);
> +  else if (IS_CVSX_PSEUDOREG (tdep, reg_nr))
> +    return cvsx_pseudo_register_read (gdbarch, regcache, reg_nr, buffer);

Hmm.  The DFP and EFP pseudo registers are simply GDB views onto the
normal FP / VR register set.  For completeness, shouldn't we also offer
similar views onto the checkpointed register set?

> +      if (IS_VSX_PSEUDOREG (tdep, reg_nr))
> +	{
> +	  reg_index = reg_nr - tdep->ppc_vsr0_regnum;
> +	  vr0 = tdep->ppc_vr0_regnum;
> +	  fp0 = tdep->ppc_fp0_regnum;
> +	  vsr0_upper = tdep->ppc_vsr0_upper_regnum;
> +	}
> +      else
> +	{
> +	  reg_index = reg_nr - tdep->ppc_cvsr0_regnum;
> +	  vr0 = PPC_CVR0_REGNUM;
> +	  fp0 = PPC_CF0_REGNUM;
> +	  vsr0_upper = PPC_CVS0H_REGNUM;
> +	}

It looks a bit odd that we need to use variable numbers for
the vr0 / fp0 / vsr0 registers, but can hard-code the numbers
for the checkpointed register set.  Why is this the case?

Bye,
Ulrich

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