Philippe, do you know why this happens?  I also looked at bit at the two files
that Carl mentions, and although I don't claim to understand the code, it doesn't
look obviously wrong to me.

Carl, which GDB version is this?  Are you sure it "knows" about the VR registers,
independent of Valgrind?  In other words, is it new enough?

Sorry, I saw this bug but forgot to work on it, thanks for the reminder.

Here is what I think is wrong:
valgrind on ppc64 implements a ppc64 version that provides the registers vs0 .. vs63.
The 'old' vr0 .. vr31 registers are mapped to vs32..vs63.
The 'old' floating point registers f0 .. f31 are mapped to the 'low' part of the register vs0 .. vs31.
The 'high' part of vs0 .. vs31 is new.

However, the xml files describing the ppc64 target are still describing only the 'old' ppc64
architecture.

When gdb connects to valgrind --vgdb-shadow-registers=no, then valgrind gives to gdb
an xml file that describes the old arch. 
I suspect that when giving  --vgdb-shadow-registers=yes, the reply packet is not containing
the values as expected by gdb, due to the missing 'new registers high parts' of vs0 .. vs31.

Looking at the recent gdb descriptions of power, we see the following:
<!DOCTYPE target SYSTEM "gdb-target.dtd">
<target>
  <architecture>powerpc:common64</architecture>
  <xi:include href="power64-core.xml"/>
  <xi:include href="power-fpu-isa205.xml"/>
  <xi:include href="power64-linux.xml"/>
  <xi:include href="power-altivec.xml"/>
  <xi:include href="power-vsx.xml"/>
</target>

The valgrind xml target is missing the power-vsx.xml, that describes the
<!-- POWER7 VSX registers that do not overlap existing FP and VMX
 *!registers.  -->
<!DOCTYPE feature SYSTEM "gdb-target.dtd">
<feature name="org.gnu.gdb.power.vsx">
  <reg name="vs0h" bitsize="64" type="uint64"/>
  <reg name="vs1h" bitsize="64" type="uint64"/>
....

So, I think  that adding power-vsx.xml and the shadow  power-vsx-valgrind-s1 and s2.xml
will solve the problem.
Note that gdb understands how to rebuild vs0 .. vs31 from the f0 .. f31 and vs0h .. vs31h
registers.  I think gdb will not understand how to rebuild vs0s1 .. vs31s1 from
f0s1 and vs0hs1 (problem similar to intel avx registers, described in user manual
in  3.2.7. Examining and modifying Valgrind shadow registers).

Some more info:
* As discussed in comment 2, the vs registers high parts should be described.
   It is however not clear to me if these registers should always be reported to GDB
   or if they should be reported only depending on some hwcaps.
   If that is the case, then some logic similar to 'have_avx()' in valgrind-low-amd64.c should
   be implemented, so as to choose an xml file that only reports the registers present
   in the currently used arch.

* On top of adding the xml file for the vs registers, I suspect that it will be needed
  to either fix or remove the regnum notation in s1 and s2 xml files:
  in the x86 and amd64 xml files, either the regnum components were updated in the s1 and s2
  xml files, or the regnum components were removed.
  I see that many xml files (e.g. for power64, but also e.g. for mips) have kept the regnum
  of the 'original non shadow' xml file.  Having such regnum is for sure also causing problems.
  It might even be the main cause for the vr shadow registers being incorrect.

Finally, a few hints that can help investigating:
* in GDB, you can use the command
     maint print remote-registers
  to see the definition of registers as understood by gdb.
  The columns 'Rmt Nr  g/G Offset' are giving the protocol register number and the
   offset in the g/G packets.

* to examine directly the valgrind threads registers, you can do in gdb+vgdb:
     mo v.set h
  then
    add-symbol-file <full path name of tool file> 0x38000000

  Then e.g. to print a register of thread 1, you can from gdb do
     (gdb) p /x vgPlain_threads[1].arch.vex.guest_GPR13
     $2 = 0x0
     (gdb) p /x vgPlain_threads[1].arch.vex_shadow1.guest_GPR13
     $3 = 0x0
    (gdb) 
(all valgrind global variables can be examined once host visibility was activated using
   mo v.set h

Created attachment 98384 [details]
proposed patch to fix issue

Attached is a patch to deal with the issue.  Turns out there are several issues.  1) there is a big Endian/ little Endian issue that was missed earlier. The patch fixes that.  Once the Endian issue was fixed, then the vsx register definition file that Phillippe mentions was added.  That requires adding the new registers to the register structure and the case statement to get fetch the register values from the guest state.  Finally, the order GDB prints the real guest register contents and the order the shadow registers in was not consistent.  Specifically, the HW register print order was: GPRs, Floating point registers, then the pc, msr, cr, lr, ctr, etc registers.  However, the order GDB printed the shadow registers was: GPRs, pc, msr, cr, lr, ctr, etc, then the Floating point registers.  The issue was the shadow register name and contents didn't line up.  The register contents order matched the HW register print order.  So initially the fp0s1 register value was printed with pcs1.  The shadow register xml definition file for the GPRs and pc, msr, lr, etc had to be split into two files.  The first file contains the GPRs.  The second file contains the pc,msr, lr, etc.  The second file with the shadow pc, msr, lr, etc. definitions was then included after the shadow floating point register xml file.  This results in GDB printing the shadow register names in the same order as the HW registers and the register contents now align to the correct register name.  

Please review the patch and let me know if you have any suggested changes/fixes.  Thanks.

I (quickly) read the patch (did not test), a few minor comments/questions:

* typo in power64-core.xml : typo: regect -> reject
* powerpc-altivec64l-valgrind.xml : I am not sure to fully understand why we have 2 new
  includes for   power64-core2-valgrind-s1.xml and power64-core2-valgrind-s1.xml, but there
  was no addition of power64-core2.xml after power-fpu.xml : normally, the s1 and s2 files
  should be similar in structure to the 'non shadow' register files.
  I see that the power64-core2*xml files are defining the shadow registers for e.g. pc/msr/cr
  while the equivalent non shadow registers are in power64-core.xml
  It is unclear to me why the shadow registers for these cannot be defined in files that are
  'similar in structure' to the non shadow files.
* valgrind-low-ppc64.c : typos fpmap -> fp map
     lower lower -> lower
    psuedo -> pseudo  (twice this typo)
    remove final , after +  { "vs31h", 10720, 64 },
* valgrind-low-ppc64.c : we have a bunch of lines that are (almost) duplicated for big/little
    endian.
   As far as I can see, the only difference is that the offset is [0] or [2].
   So, if this is really the only difference, it would be better to use something like
          [offset]
   and have offset set to 0 or 2, depending on endianness.

Thanks for looking at this

> * typo in power64-core.xml : typo: regect -> reject    FIXED

> * powerpc-altivec64l-valgrind.xml : I am not sure to fully understand why we have 2 new 
> includes for power64-core2-valgrind-s1.xml and power64-core2-valgrind-s1.xml, but there was
> no addition of power64-core2.xml after power-fpu.xml : normally, the s1 and s2 files should be
> similar in structure to the 'non shadow' register files. I see that the power64-core2*xml files are
> defining the shadow registers for e.g. pc/msr/cr while the equivalent non shadow registers are
> in power64-core.xml It is unclear to me why the shadow registers for these cannot be defined 
> in files that are 'similar in structure' to the non shadow files. 

 In  power64-core.xml, there is a comment about why the GPRs and pc/msr/cr have to be in that file.   In  power64-core-valgrind-s*xml, I tried to explain why the definitions of the pc/msr/cr registers had to be moved to  power64-core2-valgrind-s*xml but I guess the explanation wasn't clear. 

I reworked the comments in the power64-core-valgrind-s*xml files to try and make the explanation clearer.  Obviously, I didn't get the message across the first time.  I think the reworked comments are a lot better.  See what you think.

> * valgrind-low-ppc64.c : typos fpmap -> fp map lower lower -> lower psuedo -> pseudo (twice this typo) remove final , after + { "vs31h", 10720, 64 },   FIXED

> * valgrind-low-ppc64.c : we have a bunch of lines that are (almost) duplicated for big/little ....
Good suggestion, I added two variables low_offset and high_offset and then set them using the ifdefs for big and little endian.  That way we have the one copy of the code which uses low_offset and high_offset to index the correct 64-bits in the 128-bit array.  The code is much cleaner and more compact.

Created attachment 98449 [details]
Updated patch to fix GDB issue

Attached the updated patch

Created attachment 98450 [details]
Secone update to patch to fix GDB issue, changed some comments

Noticed a comment that needed a little work when I was verifying the correct patch was in the attachment.

Re-reading the comment at the beginning of valgrind-low-ppc64.c, the register maps is 
still not clear to me.
The comment tells there are 64 VSR registers of 64 bits.
But afterwards; the same commen tells that "The 32 vr[0] to vr[31] registers of size 128-bits map to VSR[31] to VSR[63].".
Probably a VSR register is 128 bits.

The second not very clear thing is for the fp registers: when the comment tells
that 'however, these are not "real" floating point registers':  is that speaking about fp[0..31]
or rather about upper 64 bits of vsr[32] to vsr[63] ? I guess the later, so maybe tells something
like floating point instructions are referencing the fp registers, and not the vsr registers.

Finally, the last paragraph tells 'the 32 floating point registers  (AKA VSR[0] to VSR[31])'
which contradicts the previous paragraph that told the fp register sare in vsr[32..63] upper bits.


In the code that initialises low_offset and high_offset : for VG_BIGENDIAN, the comment tells
that the 64-bits are stored as Little Endian. Are the values really stored as little endian, even
on a big endian platform ? The little endian comment says everything is little endian (so sounds logical); but the big endian case seems half big/half little. If this is really the case, then maybe
add a sentence such as:  "In the big endian case, a little endian convention is still partially used."

For the xml core2 file:  after re-reading the comment and playing with gdb on gcc110,
I finally understood :).  GDB is somewhat bizarre: it prints the general registers, then the
fp register 0 .. 31, then pc etc,  even if the xml file defines them in the order general registers
followed by pc etc   followed by fp 0 .. 31.
Thanks for clarifying this.

Otherwise, patch looks good to me. Up to you to see which additional updates you do
for the comments above, but feel free to commit.

Thanks

Yes, there are typos in that comment which is causing confusion.

> The comment tells there are 64 VSR registers of 64 bits.
Should say 128-bits not 64 bits.

> that 'however, these are not "real" floating point registers': is that speaking about fp[0..31]
Should say fp[32..63] 

> Finally, the last paragraph tells 'the 32 floating point registers (AKA VSR[0] to VSR[31])'
   yea, the AKA confuses things.  I removed   "(AKA VSR[0] to VSR[31])"

I also added a diagram that shows the register layout.  Basically copied from the ISA document.  That should help the reader to visualize the layout.

> In the code that initialises low_offset and high_offset : for VG_BIGENDIAN, the comment tells 
> that the 64-bits are stored as Little Endian. Are the values really stored as little endian, even 
> on a big endian platform ? 

I think the comment confused things more then it should have.  I was trying to describe where 
the 64-bit values were but I see how you interpreted it.  The four 32-bit values are stored in little endian or big endian format no mixed/partial format.  So, I changed the comment to state where the four 32-bit values map to the array indicies to remove the confusion.

I will re-spin the patch, update it in this bugzilla and retest everything on the various platforms before committing the patch. 

Thanks for the help.

Created attachment 98496 [details]
final patch update

Final update to patch.  Updated the comments to fix typos and improve the clarity of the comments.  Will commit this version of the patch.

Patch applied.  Valgrind commit 15864.

R15864/5 fails to build (tested on MIPS and x86) due to missing files:

m_gdbserver/power64-core2-valgrind-s1.xml
m_gdbserver/power64-core2-valgrind-s2.xml
m_gdbserver/power-vsx-valgrind-s1.xml
m_gdbserver/power-vsx-valgrind-s2.xml
m_gdbserver/power-vsx.xml

gcc -m64 -O2 -g -std=gnu99 -Wall -Wmissing-prototypes -Wshadow -Wpointer-arith -Wstrict-prototypes -Wmissing-declarations -Wcast-align -Wcast-qual -Wwrite-strings -Wempty-body -Wformat -Wformat-security -Wignored-qualifiers -Wmissing-parameter-type -Wold-style-declaration -fno-stack-protector -fno-strict-aliasing -fno-builtin  -O -g -fno-omit-frame-pointer -fno-strict-aliasing -fpic -fno-builtin -fno-ipa-icf  -nodefaultlibs -shared -Wl,-z,interpose,-z,initfirst  -m64   -o vgpreload_core-amd64-linux.so vgpreload_core_amd64_linux_so-vg_preloaded.o  
make[3]: *** No rule to make target `m_gdbserver/power64-core2-valgrind-s1.xml', needed by `all-am'.  Stop.
make[3]: Leaving directory `/home/aca/posao/valgrind/coregrind'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/home/aca/posao/valgrind/coregrind'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/aca/posao/valgrind'
make: *** [all] Error 2