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 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))
(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.
(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>
(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
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.