diff mbox

Incorrect handling of PPC64 rldcl insn

Message ID 5187ECAD.4050901@suse.de
State New
Headers show

Commit Message

Alexander Graf May 6, 2013, 5:47 p.m. UTC 
On 05/06/2013 07:00 PM, Torbjorn Granlund wrote:
> I could finally make Debian GNU/Linux install and run under
> qemu-system-ppc64.  I used Debian 7.0.0 and qemu from the main git repo,
> updated a few days ago.
>
> While Debian runs well and not too slowly, GMP fails badly under all
> ABIs, and in many different ways.  I have isolated the first problem.
>
> Test case:
>
> #include<stdio.h>
> int
> main ()
> {
>    unsigned long r;
>    asm ("rldcl\t%0, %1, %2, 0" : "=r" (r) : "r" (0xcafebabedeadbeeful), "r" (16));
>    printf ("%lx\n", r);
>    return 0;
> }
>
> Expected output:
> babedeadbeefcafe
>
> Output under qemu:
> 0
>
> I have single stepped in gdb to determine that it is indeed rldcl that
> misbehaves.

Thanks a lot for the bug report and test case! Please CC qemu-ppc 
whenever you find issues or have patches for PPC. That makes filtering 
for important mails a lot easier.

Does the patch below fix the issue for you?


Alex

      tcg_gen_rotl_tl(t0, cpu_gpr[rS(ctx->opcode)], t0);

Comments

Torbjorn Granlund May 6, 2013, 6:13 p.m. UTC | #1 
Alexander Graf <agraf@suse.de> writes:

  Thanks a lot for the bug report and test case! Please CC qemu-ppc
  whenever you find issues or have patches for PPC. That makes filtering
  for important mails a lot easier.
  
Would that make my complaints be considered more or less important?  :-)

  Does the patch below fix the issue for you?
  
It indeed does.  (I actually tried that already, but I cannot follow the
data flow into these functions, so cannot tell if that patch is
sufficient.  This bug indicates complete non-testing status of these
insns, which are mainstream enough to be generated by gcc.  I suppose
there will likely be more such fundamental errors if more instructions
are also completely untested.)
Alexander Graf May 6, 2013, 10:14 p.m. UTC | #2 
On 06.05.2013, at 20:13, Torbjorn Granlund wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>  Thanks a lot for the bug report and test case! Please CC qemu-ppc
>  whenever you find issues or have patches for PPC. That makes filtering
>  for important mails a lot easier.
> 
> Would that make my complaints be considered more or less important?  :-)
> 
>  Does the patch below fix the issue for you?
> 
> It indeed does.  (I actually tried that already, but I cannot follow the
> data flow into these functions, so cannot tell if that patch is
> sufficient.  

Yes, it is. It's a leftover bug from converting the code to TCG I assume.

> This bug indicates complete non-testing status of these
> insns, which are mainstream enough to be generated by gcc.  I suppose
> there will likely be more such fundamental errors if more instructions
> are also completely untested.)

There's a certain chance that happens, yes. We don't have instruction test suites for the PPC target.


Alex
Aurelien Jarno May 6, 2013, 11:12 p.m. UTC | #3 
On Tue, May 07, 2013 at 12:14:47AM +0200, Alexander Graf wrote:
> 
> On 06.05.2013, at 20:13, Torbjorn Granlund wrote:
> 
> > Alexander Graf <agraf@suse.de> writes:
> > 
> >  Thanks a lot for the bug report and test case! Please CC qemu-ppc
> >  whenever you find issues or have patches for PPC. That makes filtering
> >  for important mails a lot easier.
> > 
> > Would that make my complaints be considered more or less important?  :-)
> > 
> >  Does the patch below fix the issue for you?
> > 
> > It indeed does.  (I actually tried that already, but I cannot follow the
> > data flow into these functions, so cannot tell if that patch is
> > sufficient.  
> 
> Yes, it is. It's a leftover bug from converting the code to TCG I assume.

Yes, looks like I am the culprit here.

> > This bug indicates complete non-testing status of these
> > insns, which are mainstream enough to be generated by gcc.  I suppose
> > there will likely be more such fundamental errors if more instructions
> > are also completely untested.)
> 
> There's a certain chance that happens, yes. We don't have instruction test suites for the PPC target.
> 

We have the Gwenole Beauschene testsuite for the PPC32 target, even if
it doesn't work when compiled on a recent distribution, one has to use
the old binary. It currently passes, so the PPC32 and Altivec
instructions should be fine.

On the contrary, the PPC64 instructions are untested, and there are
likely a few bugs like this one left, especially on complex
instructions.
Torbjorn Granlund May 7, 2013, 10:27 a.m. UTC | #4 
Alexander Graf <agraf@suse.de> writes:

  There's a certain chance that happens, yes. We don't have instruction
  test suites for the PPC target.
  
There certainly are more bugs.  GMP still crashes all over the place.
I have semi-isolated one more.

Extracted stand-alone sources:
Asm code generated on gcc110 from the source file:
Generate executable and execute:

gcc -m32 -mpowerpc64 bug-qemu-ppc-again.s && ./a.out

This runs silently as it should on real hardware.  Under qemu (from
2013-05-02 plus the rldcl patch) I incorrectly get the error message:

GMP_NUMB_CEIL_MAX_DIV3 too small

This seems reproducible every time, unlike most qemu bugs that hit GMP.
I haven't isolated this bug to a single instruction, but if rldcl was
untested, expecting all of there here used rldicl rldimi rlwinm to be
tested is perhaps over-optimistic?
Peter Maydell May 7, 2013, 10:39 a.m. UTC | #5 
On 7 May 2013 11:27, Torbjorn Granlund <tg@gmplib.org> wrote:
> Alexander Graf <agraf@suse.de> writes:
>
>   There's a certain chance that happens, yes. We don't have instruction
>   test suites for the PPC target.
>
> There certainly are more bugs.  GMP still crashes all over the place.

If you want to more seriously test the PPC instructions
it might be worth extending risu to cope with more than
just the ARM architecture...
 https://wiki.linaro.org/PeterMaydell/Risu
That would be a moderate chunk of work -- about 300 lines
of C code in the client, plus refactoring the generator
perl script to separate out the architecture-dependent
bits and add ppc support. The advantage is that once
you've done it it's very easy to add support for testing
a single instruction, provided you have a known-good
hardware implementation to be the reference.

(one day soon I will have to make it cope with aarch64)

-- PMM
Torbjorn Granlund May 7, 2013, 11:48 a.m. UTC | #6 
Peter Maydell <peter.maydell@linaro.org> writes:

  On 7 May 2013 11:27, Torbjorn Granlund <tg@gmplib.org> wrote:
  > Alexander Graf <agraf@suse.de> writes:
  >
  >   There's a certain chance that happens, yes. We don't have instruction
  >   test suites for the PPC target.
  >
  > There certainly are more bugs.  GMP still crashes all over the place.
  
  If you want to more seriously test the PPC instructions
  it might be worth extending risu to cope with more than
  just the ARM architecture...

I am an involuntary tester of lots of software, and a voluntary tester
of my own software.

I think we should do testing of the software we write ourselves, since we
else hurt the productivity of the community.

I waste half my hacking time on other hackers' plain bugs.

But no, I will not become a voluntary tester of the qemu hackers'
untested code...
Peter Maydell May 7, 2013, 11:51 a.m. UTC | #7 
On 7 May 2013 12:48, Torbjorn Granlund <tg@gmplib.org> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>   On 7 May 2013 11:27, Torbjorn Granlund <tg@gmplib.org> wrote:
>   > Alexander Graf <agraf@suse.de> writes:
>   >   There's a certain chance that happens, yes. We don't have instruction
>   >   test suites for the PPC target.
>   >
>   > There certainly are more bugs.  GMP still crashes all over the place.
>
>   If you want to more seriously test the PPC instructions
>   it might be worth extending risu to cope with more than
>   just the ARM architecture...
>
> I am an involuntary tester of lots of software, and a voluntary tester
> of my own software.
>
> I think we should do testing of the software we write ourselves, since we
> else hurt the productivity of the community.
>
> I waste half my hacking time on other hackers' plain bugs.
>
> But no, I will not become a voluntary tester of the qemu hackers'
> untested code...

Sorry, I should have phrased myself more clearly. It was more
meant as a suggestion for anybody reading qemu-ppc/qemu-devel
who was interested.

-- PMM
diff mbox

Patch

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 0886f4d..a018616 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -1733,8 +1733,6 @@  static inline void gen_rldnm(DisasContext *ctx, 
uint32_t mb, uint32_t me)
  {
      TCGv t0;

-    mb = MB(ctx->opcode);
-    me = ME(ctx->opcode);
      t0 = tcg_temp_new();
      tcg_gen_andi_tl(t0, cpu_gpr[rB(ctx->opcode)], 0x3f);