Created attachment 159659 [details] Suggested fix for this issue SUMMARY The bne instruction used in the s390x implementation of XCHG_M_R(_addr,_lval) expects an absolute address and shouldn't be used in this case. I'm attaching a patch fixing this issue. STEPS TO REPRODUCE 1. ./configure && make && make check 2. Disassemble the test with: objdump -d ./helgrind/tests/tc11_XCHG | less OBSERVED RESULT Look for the child_fn function and see how bne (address 0x100128e) may jump to nowhere (address 646): 0000000001001260 <child_fn>: ... 1001286: 58 00 20 00 l %r0,0(%r2) 100128a: ba 01 20 00 cs %r0,%r1,0(%r2) 100128e: 47 70 02 86 bne 646 EXPECTED RESULT That instruction should jump to 2 instructions before at address 0x1001286. SOFTWARE/OS VERSIONS Windows: macOS: Linux/KDE Plasma: (available in About System) KDE Plasma Version: KDE Frameworks Version: Qt Version: ADDITIONAL INFORMATION
I don't fully understand the s390x assembly, but this fix looks correct to me. With the patch the code becomes: 1001286: 58 00 20 00 l %r0,0(%r2) 100128a: ba 01 20 00 cs %r0,%r1,0(%r2) 100128e: a7 74 ff fc jne 1001286 <child_fn+0x26> Andreas, do you approve of this fix?
(In reply to Mark Wielaard from comment #1) > I don't fully understand the s390x assembly, but this fix looks correct to > me. > With the patch the code becomes: > > 1001286: 58 00 20 00 l %r0,0(%r2) > 100128a: ba 01 20 00 cs %r0,%r1,0(%r2) > 100128e: a7 74 ff fc jne 1001286 <child_fn+0x26> > > Andreas, do you approve of this fix? Wow, interesting find. Yes, the fix looks good to me, thanks!
commit 5a97c06080078aab8adfcc8985aecce7bfa5a738 Author: Tulio Magno Quites Machado Filho <tuliom@redhat.com> Date: Wed Jun 14 11:28:38 2023 -0300 s390x: Replace absolute jump for a relative one The bne instruction expects an absolute target address and it isn't best-suited for implementing a short range jump, such as the one in XCHG_M_R(). Replace it with jne which expects a relative address that can be correctly computed a link time. Interestingly, the jump is almost never taken. If it would, this would crash the test. However, linkers may complain when relacating the target address used in bne.