Re: [PATCH v5 00/12] GDB support for more powerpc registers on linux

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Pedro Franco de Carvalho wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> > I.e., what is the conclusion wrt to the differences in one of the note
> > sections generated by the kernel for a core file and the one generated
> > by GDB?  Was that resolved?  Will it be?

As Pedro mentioned, I believe the implementation in this latest patch
set as as good as we're going to get, and should result in GDB core
files that are mostly equivalent to kernel core files (and in the
minor details where they're not, I'd argue those are kernel bugs that
should be fixed there).

> > As for the EBB/PMU patch, about gdbserver writing all registers in one go
> > before resuming the inferior, and the error would not being detected at the
> > time the user tries to write them, did you try making gdbserver flush
> > the regcache after handling 'G' and 'P' packets?  From the previous
> > discussion, it sounded like that would work?

There's really two issues here.  One is that optionally present, but writable,
registers aren't currently supported well in GDB in my opinion.  The second
is that the current PowerPC kernel is really buggy with how it handles the
EBB register set in particular:
- First of all, there no reason at all why this regset should be optional
  in the first place; it might as well just be always available.
- Even so, the test the kernel used to implement the availability check
  is just broken, it uses the inverse of the test in PTRACE_SETREGSET vs.
  PTRACE_GETSETRET.
Because of the latter, I think you'll never be able to complete the
read-modify-write cycle to write an EBB register via ptrace regsets
with a current kernel, and so it is effectively read-only anyway.

But even if we assume the EBB regset will be fixed, we still should
handle writable optional regsets better, since at least the HTM
checkpointed registers are really of this type.  With those, there
are two problems:
- The user ought to be warned early with a reasonable message when
  attempting to modify an unvailable register.
- The mere presence of unavailable registers should not cause spurious
  errors when attempting to write *other* registers.

For the second part, this can happen in two places currently.  One is
if the native target store_registers routine is called with regnum -1.
But this is actually done only from proc-service.c code which handles
only general-purpose register or floating-point registers.  So it should
be fine to just ignore optional regsets in that case (as Pedro's current
patch set does).  Maybe the documentation of that routine should make
this explicit ...

The other is gdbserver.  As far as I can see, gdbserver currently does
not implement the 'p'/'P' packets at all, only 'g'/'G'.  Therefore it
will always attempt to write all registers.  The problem is that this
will show an error if any of those writes fail.

This is because, while there is code in regsets_fetch_inferior_registers
to ignore optional regsets when unavailable, there is no corresponding
code in regsets_store_inferior_registers.  This may simply be because
all other optional regsets on existing targets so far have been read-
only ...

Pedro's current patches just effectively make the new optional regsets
read-only for gdbserver too.  In my mind, this would be an acceptable
solution at least for now, which can hopefully be improved upon further.

Another simple solution would be to add code to ignore unavailable
regsets to regsets_store_inferior_registers as well.  This will work,
but might silently ignore some situations that really could be problems.
To improve upon that, we might try to track whether there are any
registers in the regset to be written that are actually REG_VALID
(if the regset was really unvailable, all registers in it will in
fact be REG_UNAVAILABLE anyway), and skip the attempt to write it
just in that case.


But really, the user-visible error should not come from gdbserver
anyway IMO.  This should really be handled by GDB up front.  One
way might be to just always throw an error right in regcache::raw_write
when attempting to modify a REG_UNAVAILABLE register in the first
place.  But that's a rather significant change, I'm not sure if that
might cause other problems on some platforms ...


In any case, I'd really like to get Pedro's patch set in as-is or
with just a simple fix (like the regsets_store_inferior_registers
change described above).  The more sophisticated solution can
still be done later ...


Pedro, I've looked over the patch set again, and (apart from this
discussion here), everything looks good to me.

Bye,
Ulrich

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