When using Valgrind's memcheck on s390x for Python, I see the following: $ valgrind python --help ==3451325== Memcheck, a memory error detector ==3451325== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==3451325== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info ==3451325== Command: python --help ==3451325== ==3451325== Conditional jump or move depends on uninitialised value(s) ==3451325== at 0x4889F82: __internal_ascii_loop_vx (loop.c:336) ==3451325== by 0x4889F82: ____gconv_transform_internal_ascii_vx (skeleton.c:620) ... The instruction Valgrind complains about is a conditional branch. The condition code has last been updated by the instruction VSTRC (vector string range compare). That instruction has three vector input operands, one of which is only partially initialized and has the remaining bits undefined, so Valgrind probably deduces that the output operand and resulting condition code are undefined as well. But the undefined elements in the vector operand are actually not used by the instruction. The usage of these elements is influenced by corresponding elements in the third input operand. The VSTRC instruction is currently implemented with a dirty helper. This also applies to some other vector string instructions, so those are likely affected by similar issues.
As a heads up, I verified that all vector string instructions currently have the potential to yield memcheck false positives. Thus I'm working on a fix that re-implements all vector string instructions. Instead of implementing them with a dirty helper, they will then be transformed to normal IR instead.
Created attachment 138034 [details] Fix the memcheck false positives with s390x vector string insns This patch set re-implements the IR transformation of all s390x vector string instructions, fixing all the false positives that I have observed with them. Test cases are added as well. In addition, some improvements to the code generation are made that particularly affect the refactored instructions. Even with the optimizations the generated code can get quite long in some cases. The worst probably being "vector string range compare", where I see up to ca. 1700 s390-insns being emitted. A few instructions, such as VFENE, might be slightly more efficient than before. Suggestions for further improvements are welcome. Note that the patch set is based on the patch for Bug 433863 that removes the s390x-specific compare-and-swap test cases for memcheck.
From an implementation view this all looks fine. I have only the minor query: - /* Check for specification exception */ - vassert(m3 < 3); - vassert((m5 & 0b1110) == 0); + s390_insn_assert("vistr", m3 < 3 && m5 == (m5 & 1)); Is it possible for any incoming instruction to cause this assertion to fail? If so that should be reported as SIGILL, and not cause an assertion failure. From the code-expansion point of view, I am somewhat concerned that this may cause failures in VEX due to generating too many instructions in the back end, etc. But I can't think of any obvious improvement and it's clear you've spent time thinking about this too. The one thing I would suggest is that, for the instructions in question, in the front end, set the returned DisResult::hint field to Dis_HintVerbose. This tells the top level block-disassembly logic that the instruction just translated to IR is "verbose" and so will cause it to stop pulling new insns into the block sooner rather than later. For examples look for dres->hint = Dis_HintVerbose; in guest_amd64_toIR.c.
(In reply to Julian Seward from comment #3) > Is it possible for any incoming instruction to cause this assertion to > fail? Yes. Of course, only invalid code would do that. > If so that should be reported as SIGILL, and not cause an assertion > failure. Right. The s390_insn_assert macro does not behave like vassert(), though. If its argument is false, it reports a "specification exception" up through the call chain. The user would see something like this: vex s390->IR: specification exception: E712 30F0 F082 ==2699373== valgrind: Unrecognised instruction at address 0x1000518. ... ==2699373== ==2699373== Process terminating with default action of signal 4 (SIGILL): dumping core ... I think it's an improvement over the previous logic, which simply called vassert(). Although in practice, I haven't heard about that causing trouble, either. So my take is, whenever I touch an existing IR translator function that uses vassert() for indicating a specification exception, I replace that by s390_insn_assert() instead. > [...] > The one thing I would suggest is that, for the instructions in question, in > the front end, set the returned DisResult::hint field to Dis_HintVerbose. Ah, good point, will do. Is there a recommended threshold when to set this flag?
(In reply to Andreas Arnez from comment #4) > > the front end, set the returned DisResult::hint field to Dis_HintVerbose. > Ah, good point, will do. Is there a recommended threshold when to set > this flag? No .. it's an ad-hoc hack :-) but I would suggest you do it for all of the insns covered by this bug, since they look like they could each generate 20 or more IRStmts per guest insn.
(In reply to Julian Seward from comment #5) > [...] I would suggest you do it for all of the > insns covered by this bug, since they look like they could each generate > 20 or more IRStmts per guest insn. OK, done. Unless there's anything else, I'm going to push with this change then.
Created attachment 138170 [details] Updated patch set For reference, this is the updated patch set with the "verbose" hints added as discussed above.
(In reply to Andreas Arnez from comment #6) > OK, done. Unless there's anything else, I'm going to push with this change > then. Yes, pls land. Looks fine to me.
(In reply to Julian Seward from comment #8) > Yes, pls land. Looks fine to me. Thanks, pushed.