Bug 254646 - Wrapped functions cause stack misalignment on OS X (and possibly Linux)
Summary: Wrapped functions cause stack misalignment on OS X (and possibly Linux)
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (show other bugs)
Version: 3.6 SVN
Platform: Unlisted Binaries macOS
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-19 12:55 UTC by Alexander Potapenko
Modified: 2012-03-27 09:41 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Preserve esp/rsp 16-alignment on x86/amd64 platforms (12.53 KB, patch)
2010-10-20 15:48 UTC, Julian Seward
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Potapenko 2010-10-19 12:55:02 UTC
I'm running Valgrind v11457 on Mac OS X

======================
$ cat rwlock.c 
#include <pthread.h>
int main() {
  pthread_rwlock_t rw;
  pthread_rwlock_init(&rw, NULL);
  return 0;
}
======================
$ gcc rwlock.c -o rwlock
$ inst/bin/valgrind --tool=helgrind ./rwlock

--60790-- /Users/glider/src/data-race-test/rwlock:
--60790-- dSYM directory is missing; consider using --dsymutil=yes
==60790== 
==60790== Process terminating with default action of signal 11 (SIGSEGV)
==60790==  General Protection Fault
==60790==    at 0x8FE18C02: misaligned_stack_error (in /usr/lib/dyld)
==60790==    by 0x22FE2C: pthread_rwlock_init$UNIX2003 (in /usr/lib/libSystem.B.dylib)
==60790==    by 0x182E9: pthread_rwlock_init* (hg_intercepts.c:1305)
==60790==    by 0x1FF8: main (in /Users/glider/src/data-race-test/rwlock)
==60790== 

This error is exposed by tools that wrap functions (e.g. Helgrind and ThreadSanitizer), but not those that replace them (Memcheck).

We've seen related reports when running ThreadSanitizer on our Linux buildbot: http://build.chromium.org/buildbot/tsan/builders/buildbot-experimental/builds/5388/steps/shell_4/logs/stdio
There's also a bug issue for NativeClient, which exposes the same behavior: http://code.google.com/p/nativeclient/issues/detail?id=1056
Comment 1 Alexander Potapenko 2010-10-20 11:05:40 UTC
Looks like this was brought by r2057 (which fixes https://bugs.kde.org/show_bug.cgi?id=153699), particularly the following diff:


-----------------------
Index: priv/guest_x86_toIR.c
===================================================================
--- priv/guest_x86_toIR.c       (revision 2056)
+++ priv/guest_x86_toIR.c       (revision 2057)
@@ -10037,6 +10046,7 @@
       } else {
          addr = disAMode( &alen, sorb, delta+2, dis_buf );
          delta += 2+alen;
+         gen_SEGV_if_not_16_aligned( addr );
          storeLE( mkexpr(addr), getXMMReg(gregOfRM(modrm)) );
          DIP("movdqa %s, %s\n", nameXMMReg(gregOfRM(modrm)), dis_buf);
       } 
-----------------------

This means that the wrappers were broken long ago, but the error wasn't reported properly.

Side note: the proposed test case works for me only in 32-bit mode.
Comment 2 Julian Seward 2010-10-20 13:38:19 UTC
What are the required alignments, for 32- and 64-bit processes?
Comment 3 Alexander Potapenko 2010-10-20 13:47:40 UTC
SSE requires 16-byte data alignment for both 32-bit and 64-bit. 
Yet again, the reported problem also happened to us on 64-bit platforms (Linux and NaCl), but is a bit harder to reproduce.
Comment 4 Tom Hughes 2010-10-20 13:55:06 UTC
The required alignment is defined by the various ABIs. For amd64 the ABI document (http://www.x86-64.org/documentation/abi.pdf) says:

In addition to registers, each function has a frame on the run-time stack. This stack grows downwards from high addresses. Figure 3.3 shows the stack organization.

The end of the input argument area shall be aligned on a 16 (32, if __m256 is passed on stack) byte boundary. In other words, the value (%rsp + 8) is always
a multiple of 16 (32) when control is transferred to the function entry point.
Comment 5 Tom Hughes 2010-10-20 13:56:36 UTC
That's the linux ABI of course, so the Mac one might be different.
Comment 6 Julian Seward 2010-10-20 14:06:03 UTC
(In reply to comment #5)
> That's the linux ABI of course, so the Mac one might be different.

Sure; but if we stick to the rule that if we move %rsp or %esp then
it must be by a multiple of 16 bytes, then everything would be OK,
I think.  That would preserve whatever alignment was there originally,
up to and including 16-alignment.  This assumes that neither Linux nor
Darwin requires more than 16-alignment, but I can't see that being the
case, at least for now.
Comment 7 Julian Seward 2010-10-20 15:48:30 UTC
Created attachment 52703 [details]
Preserve esp/rsp 16-alignment on x86/amd64 platforms 

Alexander, can you try this patch?
Comment 8 Alexander Potapenko 2010-10-20 16:10:41 UTC
(In reply to comment #7)
This works for me on Mac OS 10.5, thanks!
> Created an attachment (id=52703) [details]
> Preserve esp/rsp 16-alignment on x86/amd64 platforms 
> 
> Alexander, can you try this patch?
Comment 9 Julian Seward 2010-10-20 16:29:59 UTC
Both 32- and 64-bit ?
Comment 10 Julian Seward 2010-10-20 17:57:04 UTC
Fixed, r11461.
Comment 11 Alexander Potapenko 2010-10-27 13:36:39 UTC
We're still having the same problem in ThreadSanitizer after updating to Valgrind 11461, see http://code.google.com/p/data-race-test/issues/detail?id=49
However Helgrind works with this patch.
Comment 12 Julian Seward 2010-10-27 15:20:50 UTC
(In reply to comment #11)
> We're still having the same problem in ThreadSanitizer after updating to
> Valgrind 11461,

Do you have a way to reproduce this with a vanilla, unmodified Valgrind
trunk now?
Comment 13 Bart Van Assche 2011-03-06 11:42:51 UTC
(In reply to comment #12)
> Do you have a way to reproduce this with a vanilla, unmodified Valgrind
> trunk now?

Just run none/tests/pending.vgtest:

$ perl tests/vg_regtest none/tests/pending >&/dev/null
$ cat none/tests/pending.stderr.diff 
--- pending.stderr.exp	2011-02-12 13:07:20.000000000 +0100
+++ pending.stderr.out	2011-03-06 11:38:29.000000000 +0100
@@ -1,2 +1,9 @@
 
 
+Process terminating with default action of signal 11 (SIGSEGV)
+ General Protection Fault
+   at 0x........: dyld_stub_binder (in /...libc...)
+   by 0x........: ??? (in ./pending)
+   by 0x........: ???
+   by 0x........: (below main)
+
Comment 14 Julian Seward 2012-03-27 09:41:58 UTC
I believe this has been fixed properly by r12461 and 12462,
so I'm going to close it.  Please reopen if it's still a problem.