Bug 402480 - Do not use %rsp in clobber list
Summary: Do not use %rsp in clobber list
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: 3.15 SVN
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-22 23:31 UTC by Khem Raj
Modified: 2019-01-11 19:02 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch to fix the issue (1.44 KB, application/mbox)
2018-12-22 23:31 UTC, Khem Raj
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Khem Raj 2018-12-22 23:31:39 UTC
Created attachment 117070 [details]
Patch to fix the issue

SUMMARY

This is a testcase where we are expecting %rsp to be clobbered but its a fixed register that compiler does not save. This was silently ignored thus far by gcc, however gcc 9.0 has fixed it and now generates a dignostics error

see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52813

STEPS TO REPRODUCE
1. build valgrind with gcc trunk ( would be gcc 9 soon)


OBSERVED RESULT

Compiler error
 ../../../../valgrind-3.14.0/none/tests/amd64-linux/bug345887.c
| ../../../../valgrind-3.14.0/none/tests/amd64-linux/bug345887.c: In function 'inner':
| ../../../../valgrind-3.14.0/none/tests/amd64-linux/bug345887.c:7:4: error: Stack Pointer register clobbered by '%rsp' in 'a
sm'
|     7 |    __asm__ __volatile__(
|       |    ^~~~~~~
| make[5]: *** [Makefile:667: bug345887.o] Error 1

EXPECTED RESULT

No compile error

ADDITIONAL INFORMATION

Seeing this with OpenEmbedded/Yocto cross compile system when upgrading gcc to trunk
Comment 1 Mark Wielaard 2018-12-23 21:18:00 UTC
Comment on attachment 117070 [details]
Patch to fix the issue

While I agree that trying to clobber the stack pointer doesn't make sense (and it only "works" because we never return from the asm), the changing of the stack pointer itself is deliberate and part of the test (bug #345887 was about checking the stack pointer was bogus and not crashing valgrind, even though the test program itself does crash).

Also without the bogus stack pointer the expected output doesn't match anymore (because now we get a full backtrace because we can follow the stack).

So how about just removing the clobber, but keeping stack pointer change itself:

diff --git a/none/tests/amd64-linux/bug345887.c b/none/tests/amd64-linux/bug345887.c
index 0f9237d..269bd70 100644
--- a/none/tests/amd64-linux/bug345887.c
+++ b/none/tests/amd64-linux/bug345887.c
@@ -20,13 +20,17 @@ static void inner(void)
       "movq $0x10d, %%r14\n"
       "movq $0x10e, %%r15\n"
       // not %rbp as mdb is then not able to reconstruct stack trace
+      // Do change %rsp (to test a bogus stack pointer),
+      // but don't add %rsp to the clobber list since gcc ignores it
+      // and since gcc >= 9.0 errors about it
+      // see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52813
       "movq $0x10f, %%rsp\n"
       "movq $0x1234, (%%rax)\n"  // should cause SEGV here
       "ud2"                      // should never get here
       : // no output registers
       : // no input registers
       : "memory", "%rax", "%rbx", "%rcx", "%rdx", "%rsi", "%rdi",
-        "%r8", "%r9", "%r10", "%r11", "%r12", "%r13", "%r14", "%r15", "%rsp");
+        "%r8", "%r9", "%r10", "%r11", "%r12", "%r13", "%r14", "%r15");
 }
 
 __attribute__((noinline))
Comment 2 Khem Raj 2018-12-23 21:28:46 UTC
(In reply to Mark Wielaard from comment #1)
> Comment on attachment 117070 [details]
> Patch to fix the issue
> 
> While I agree that trying to clobber the stack pointer doesn't make sense
> (and it only "works" because we never return from the asm), the changing of
> the stack pointer itself is deliberate and part of the test (bug #345887 was
> about checking the stack pointer was bogus and not crashing valgrind, even
> though the test program itself does crash).
> 
> Also without the bogus stack pointer the expected output doesn't match
> anymore (because now we get a full backtrace because we can follow the
> stack).
> 
> So how about just removing the clobber, but keeping stack pointer change
> itself:
> 
> diff --git a/none/tests/amd64-linux/bug345887.c
> b/none/tests/amd64-linux/bug345887.c
> index 0f9237d..269bd70 100644
> --- a/none/tests/amd64-linux/bug345887.c
> +++ b/none/tests/amd64-linux/bug345887.c
> @@ -20,13 +20,17 @@ static void inner(void)
>        "movq $0x10d, %%r14\n"
>        "movq $0x10e, %%r15\n"
>        // not %rbp as mdb is then not able to reconstruct stack trace
> +      // Do change %rsp (to test a bogus stack pointer),
> +      // but don't add %rsp to the clobber list since gcc ignores it
> +      // and since gcc >= 9.0 errors about it
> +      // see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52813
>        "movq $0x10f, %%rsp\n"
>        "movq $0x1234, (%%rax)\n"  // should cause SEGV here
>        "ud2"                      // should never get here
>        : // no output registers
>        : // no input registers
>        : "memory", "%rax", "%rbx", "%rcx", "%rdx", "%rsi", "%rdi",
> -        "%r8", "%r9", "%r10", "%r11", "%r12", "%r13", "%r14", "%r15",
> "%rsp");
> +        "%r8", "%r9", "%r10", "%r11", "%r12", "%r13", "%r14", "%r15");
>  }
>  
>  __attribute__((noinline))


OK that seems fine to me. It will work.
Comment 3 Mark Wielaard 2018-12-23 22:13:12 UTC
(In reply to Khem Raj from comment #2)
> (In reply to Mark Wielaard from comment #1)
> > So how about just removing the clobber, but keeping stack pointer change
> > itself:
>
> OK that seems fine to me. It will work.

Thanks. Pushed that to master as:

commit 022f5af61bc3cbfa2b74ab355b0d2d30b3dab027
Author: Khem Raj <raj.khem@gmail.com>
Date:   Sat Dec 22 15:28:40 2018 -0800

    tests/amd64: Do not clobber %rsp register
    
    This is seen with gcc-9.0 compiler now which is fix that gcc community
    did recently
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52813
    
    Signed-off-by: Khem Raj <raj.khem@gmail.com>
Comment 4 Khem Raj 2018-12-23 23:46:44 UTC
(In reply to Mark Wielaard from comment #3)
> (In reply to Khem Raj from comment #2)
> > (In reply to Mark Wielaard from comment #1)
> > > So how about just removing the clobber, but keeping stack pointer change
> > > itself:
> >
> > OK that seems fine to me. It will work.
> 
> Thanks. Pushed that to master as:
> 
> commit 022f5af61bc3cbfa2b74ab355b0d2d30b3dab027
> Author: Khem Raj <raj.khem@gmail.com>
> Date:   Sat Dec 22 15:28:40 2018 -0800
> 
>     tests/amd64: Do not clobber %rsp register
>     
>     This is seen with gcc-9.0 compiler now which is fix that gcc community
>     did recently
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52813
>     
>     Signed-off-by: Khem Raj <raj.khem@gmail.com>

Superb Mark, that was quick. Merry Christmas
Comment 5 Mark Wielaard 2019-01-11 19:02:09 UTC
There was a similar issue for the x86-linux testcase, fixed with:

commit c512949082c4fc2285a82e102d4212c66e034a31
Author: Mark Wielaard <mark@klomp.org>
Date:   Fri Jan 11 20:00:17 2019 +0100

    Bug 402480 Do not use %esp in clobber list.
    
    This is the same fix as for amd64-linux, but now for x86-linux.