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.
Created attachment 82266 [details] Problem analysis.
Created attachment 82267 [details] Proposed patch to fix the problem.
(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.
Related to: https://bugs.kde.org/show_bug.cgi?id=324518
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.
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.
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.
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.
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
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.
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.
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.
(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.
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.