Bug 471032

Summary: s390x: helgrind/tests/tc11_XCHG.c expects code to be at a low address space
Product: [Developer tools] valgrind Reporter: Tulio Magno Quites Machado Filho <tuliom>
Component: helgrindAssignee: Julian Seward <jseward>
Status: RESOLVED FIXED    
Severity: normal CC: arnez, mark
Priority: NOR    
Version: 3.22 GIT   
Target Milestone: ---   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:
Attachments: Suggested fix for this issue

Description Tulio Magno Quites Machado Filho 2023-06-14 18:17:47 UTC
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
Comment 1 Mark Wielaard 2023-06-14 20:57:09 UTC
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?
Comment 2 Andreas Arnez 2023-06-15 12:13:55 UTC
(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!
Comment 3 Mark Wielaard 2023-06-15 15:02:34 UTC
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.