Bug 324765 - ppc64: illegal instruction when executing none/tests/ppc64/jm-misc
Summary: ppc64: illegal instruction when executing none/tests/ppc64/jm-misc
Status: CLOSED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: 3.9.0.SVN
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-10 19:58 UTC by Florian Krohm
Modified: 2013-09-18 16:12 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Problem analysis. (22.96 KB, text/plain)
2013-09-11 01:37 UTC, Anmol P. Paralkar
Details
Proposed patch to fix the problem. (1.16 KB, patch)
2013-09-11 01:39 UTC, Anmol P. Paralkar
Details
Respun (v1) proposed patch. (2.76 KB, patch)
2013-09-11 21:01 UTC, Anmol P. Paralkar
Details
Respun (v2) proposed patch. (3.11 KB, patch)
2013-09-13 02:13 UTC, Anmol P. Paralkar
Details
Respun (v3) proposed patch. (3.89 KB, patch)
2013-09-17 21:49 UTC, Anmol P. Paralkar
Details
Notes for (v3) patch. (4.10 KB, text/plain)
2013-09-17 21:58 UTC, Anmol P. Paralkar
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Krohm 2013-09-10 19:58:11 UTC
Revision: 13538 / 2755
Distro: Fedora release 18 (Spherical Cow)
GCC: gcc (GCC) 4.7.2 20121109 (Red Hat 4.7.2-8)
CPU:
processor	: 63
cpu		: POWER7 (architected), altivec supported
clock		: 3550.000000MHz
revision	: 2.1 (pvr 003f 0201)

timebase	: 512000000
platform	: pSeries
model		: IBM,8231-E2B
machine		: CHRP IBM,8231-E2B

After svn up; autogen.sh etc...
[florian@gcc1-power7 trunk]$ perl tests/vg_regtest none/tests/ppc64
-- Running  tests in none/tests/ppc64 ----------------------------------
jm-fp:           valgrind   ./jm-insns -f
jm-int:          valgrind   ./jm-insns -i
jm-misc:         valgrind   ./jm-insns -m
sh: line 1:  1942 Illegal instruction
VALGRIND_LIB=/home/florian/trunk/.in_place
VALGRIND_LIB_INNER=/home/florian/trunk/.in_place
/home/florian/trunk/./coregrind/valgrind --command-line-only=yes
--memcheck:leak-check=no --tool=none ./jm-insns -m > jm-misc.stdout.out
2> jm-misc.stderr.out
*** jm-misc failed (stdout) ***
*** jm-misc failed (stderr) ***

If you need more info let me know.
Comment 1 Anmol P. Paralkar 2013-09-11 01:37:24 UTC
Created attachment 82266 [details]
Problem analysis.
Comment 2 Anmol P. Paralkar 2013-09-11 01:39:01 UTC
Created attachment 82267 [details]
Proposed patch to fix the problem.
Comment 3 Florian Krohm 2013-09-11 10:50:27 UTC
(In reply to comment #2)
> Created attachment 82267 [details]
> Proposed patch to fix the problem.

I can confirm that the patch fixes the problem. I can't say whether this is the proper fix. 
Asuming it is:  The patch assumes that the host is at least Power4. Whether that is reasonable or not is for the PPC maintainers to say.  If not, some configury bits are needed to probe the host for Power4 or later and make the test conditonal on the outcome.
In either case, it should be documented what the minimum POWER architecture is that valgrind supports.
Comment 4 Anmol P. Paralkar 2013-09-11 14:27:13 UTC
 Related to: https://bugs.kde.org/show_bug.cgi?id=324518
Comment 5 Anmol P. Paralkar 2013-09-11 21:01:00 UTC
Created attachment 82286 [details]
Respun (v1) proposed patch.


 (In reference to comment # 3). Thank you, Florian. The respun (v1) patch adds support for
 probing if the host is at-least at a given Power Architecture level and makes the execution of
 the jm-misc tests conditional upon the result of probing that the host is at-least a POWER4.
Comment 6 Maynard Johnson 2013-09-11 23:00:36 UTC
Anmol, thank you for your detailed problem analysis and patch.  The development machines I use for testing are all running enterprise distros (RHEL or SLES), and I don't see the error.  You are encountering this problem on Fedora, but it's not clear to me yet why the assembler on Fedora should behave differently.  Nevertheless, we should support the Fedora assembler, so thank you for bringing this to our attention.

As Florian mentioned, your first version of the patch would restrict the successful execution of jm-misc testcase to POWER4 and higher.  Since it's only a matter of this one small testcase, I think that would be OK.  Your version 2 patch is an improvement, but there is a small bug:

+	host_arch_level=`LD_SHOW_AUXV=1 $DIR/true | grep "AT_PLATFORM:" | awk '{ print $2; }' | sed 's/power//g'`
+	result=`echo "$host_arch_level >= $expected_min_arch_level" | bc`

On the PowerPC970 processor (a successor to POWER4), AT_PLATFORM is 'ppc970', so the above logic would fail to give the expected result.
Comment 7 Anmol P. Paralkar 2013-09-13 02:13:01 UTC
Created attachment 82304 [details]
Respun (v2) proposed patch.

 Use the .machine "<cpu>" PowerPC assembler directive provided by GAS
 *as a workaround* to make it use the correct syntax in the assembling
 of dcbtst and dcbt. Using POWER4 as the value for cpu, we make GAS use
 the right syntax indirectly, exactly in this one instance only, yet do not make the
 compilation of jm-insns.c nor the execution of jm-misc restricted to
 POWER4 alone.

 Regression tested against pristine trunk (Valgrind: 13545, VEX: 2760)
 on an IBM POWER7 running Linux 3.1.5-6.fc16.ppc64. No new regressions
 introduced and none/tests/{ppc32, ppc64}/jm-misc succeeds.
Comment 8 Anmol P. Paralkar 2013-09-13 02:29:09 UTC
 Maynard, thank you for your review. I am hoping that v2 solves the problem of restricting
 to POWER4 and also works on the RHEL and SLES systems.

 Also, Florian, I gave some thought to being able to probe that the host is 'at-least a POWER<X>".
 I looked  at the .platform values in the Linux kernel's arch/powerpc/kernel/cputable.c file:

 pa6t, power3, power4, power5, power5+, power6, power6x, power7, power7+, power8, powerpc, 
 ppc403, ppc405, ppc440, ppc440gp, ppc470, ppc5554, ppc601, ppc603, ppc604, ppc7400, 
 ppc7450, ppc750, ppc823, ppc8540, ppc8548, ppc970, ppca2, ppc-cell-be, ppce500mc, 
 ppce5500, ppce6500, rs64

 and I wonder if there is some way of coming up with an inclusion relation of sorts that might
 aid in ascertaining that the host is "greater-than-or-equal-to" the minimum POWER 
architecture that Valgrind supports or would we just mark a yes/no against each element in the list above indicating if it is supported? Thank you.
Comment 9 Anmol P. Paralkar 2013-09-13 22:58:21 UTC
 I wrote to the binutils folks about this issue; here's the clarifying response (In reply to comment 
 #6) about why we might be seeing difference in behaviour across assemblers:

 https://sourceware.org/ml/binutils/2013-09/msg00065.html
Comment 10 Florian Krohm 2013-09-15 19:58:43 UTC
I have had some pain with binutils on s390 as well. Along the lines that the host would support insns that the tool-chain would not.
What I've done is to not rely on binutils to recognise those insns and instead I encode them using .word, .short etc directives
See none/tests/s390/opcodes.h. You can be sure that all binutils support those directives.
Problem solved.
Comment 11 Anmol P. Paralkar 2013-09-17 21:49:07 UTC
Created attachment 82382 [details]
Respun (v3) proposed patch.


 Introduce a new header file: none/tests/{ ppc32, ppc64 }/opcodes.h that contains macro 
 definitions to hand-assemble instructions known to cause problems with assemblers or
 across assembler versions.
Comment 12 Anmol P. Paralkar 2013-09-17 21:58:49 UTC
Created attachment 82383 [details]
Notes for (v3) patch.

 (In reply to Comment 10) Thank you for the advice Florian. I created an opcodes.h along the lines 
 you suggested. Please could you take a look at patch (v3) and the accompanying notes. Thanks.
Comment 13 Maynard Johnson 2013-09-18 14:17:07 UTC
(In reply to comment #11)
> Created attachment 82382 [details]
> Respun (v3) proposed patch.
> 
> 
>  Introduce a new header file: none/tests/{ ppc32, ppc64 }/opcodes.h that
> contains macro 
>  definitions to hand-assemble instructions known to cause problems with
> assemblers or
>  across assembler versions.
Ack.
I'm not really sure what the "link" is supposed to do in none/tests/ppc64/opcodes.h (maybe it's just a reminder to create a link), but after I manually created a symbolic link to ../ppc32/opcodes.h, the testcase compiled and ran successfully.  Inspecting an 'objdump -d' of jm-insns showed that we're getting the desired dcbt* instructions in server format.

Thanks much for handling this issue.
Comment 14 Carl Love 2013-09-18 16:11:30 UTC
I reviewed and tested version 3 of the patch.  It looks good. It was committed, valgrind commit 13562.

Unfortunately, the patches don't get the soft links correct.  So, I had to manually create the soft link from none/tests/ppc64/opcodes.h to the actual file in none/tests/ppc32/opcodes.h.  The "link" is useful to remind me to manually fix the soft link since the patch command doesn't seem to correctly create the soft link.