https://bugs.kde.org/show_bug.cgi?id=369509
--- Comment #3 from ahashmi <assad.has...@linaro.org> --- Created attachment 123930 --> https://bugs.kde.org/attachment.cgi?id=123930&action=edit Patch implements and tests Arm v8.1 LSE atomics (addresses first set of review comments) Thanks for reviewing. New patch attached with all but the Iop_CasCmpNE64 change, see why below. > My only big concern here is the lack of hwcaps support in Vex/Valgrind. > That could be done in a followup bug, but it needs to happen fairly > soon. Sorry, I'm not clear about this. Do you mean we need to add new IR primitives to Valgrind to support new hardware capabilities or to be able to detect the current hardware capabilities in order to determine if the subject binary can be executed? If it's the latter I've posted a patch for detecting AArch64 hardware capabilities to https://bugs.kde.org/show_bug.cgi?id=413547 > Let me ask: are the 8.1 extensions a single all-or-nothing thing > (meaning, they have to be all implemented, or none?) Or are they > split up into smaller subsets of instructions, and each group of > instructions can be implemented or > not implemented? It's the latter. They're split up into smaller subsets. I'm planning to post a patch for the v8.1 arithmetic instructions shortly, see https://bugs.kde.org/show_bug.cgi?id=413634 which will be the last patch for v8.1. > I'd prefer this just to be INSN(15,12) <= BITS4(1,0,0,0) Done. > We also need to check that the assembler can assemble the instructions. Done. > You need to use CasCmpNE64, not CmpNE64 here. I tried that but the first test fails with a vpanic("iselCondCode"): - - - snip CasCmpNE64(t21,t20) vex: the `impossible' happened: iselCondCode vex storage: T total 36020704 bytes allocated vex storage: P total 0 bytes allocated valgrind: the 'impossible' happened: LibVEX called failure_exit(). host stacktrace: at 0x........: show_sched_status_wrk (m_libcassert.c:406) by 0x........: report_and_quit (m_libcassert.c:477) by 0x........: vgPlain_core_panic_at (m_libcassert.c:553) by 0x........: vgPlain_core_panic (m_libcassert.c:563) by 0x........: failure_exit (m_translate.c:749) by 0x........: vpanic (main_util.c:253) by 0x........: iselCondCode_wrk (host_arm64_isel.c:1450) <--- by 0x........: iselStmt (host_arm64_isel.c:1293) by 0x........: iselSB_ARM64 (host_arm64_isel.c:4209) by 0x........: LibVEX_Translate (main_main.c:1099) by 0x........: vgPlain_translate (m_translate.c:1816) by 0x........: vgPlain_scheduler (scheduler.c:1138) by 0x........: run_a_thread_NORETURN (syswrap-linux.c:101) by 0x........: ??? . . . - - - snip It looks like the conditonal instruction selector, iselCondCode_wrk(), doesn't handle Iop_CasCmpNE64 at all! Is that missing functionality in iselCondCode_wrk() or is there a valid reason why it's intentionally not there? As to the reason for the change, does switching Iop_CmpNE64 to Iop_CasCmpNE64 mean that with Iop_CmpNE64 memcheck would not have instrumented these instructions correctly? The comment in libvex_ir.h seems to imply memcheck needs to know that there are two instances of the same representative value: /* Exactly like CmpEQ8/16/32/64, but carrying the additional hint that these compute the success/failure of a CAS operation, and hence are almost certainly applied to two copies of the same value, which in turn has implications for Memcheck's instrumentation. */ Presumably, memcheck then treats them as effectively a single value? -- You are receiving this mail because: You are watching all bug changes.
