Annotate V8 for MemorySanitizer.

Annotate V8 for MemorySanitizer.

There are 4 kinds of annotations here:
- We mark memory allocated for the V8 heap as initialized.
  This is needed because MemorySanitizer can not detect memory writes from
  JIT code and will erroneously think that entire V8 heap is not initialized.
- On JIT->C++ transition, we mark a large chunk of stack and function
  arguments as initialized.
- In stack unwind code we mark return address slots on the stack as
  uninitialized.
- Workarounds for lack of inline assembly support in sanitizers:
  + disable STOS
  + pre-initialize outputs of cpuid

[danno, 2013-10-10 14:18:06]

Why does there need to be specific support for MEMORY_SANITIZER at all in V8?
There is no ADDRESS_SANITIZER support, and I think that this falls in the same
category and I would argue it should be handled in the same way. Before pursing
this patch any further, I'd like to understand why V8's handling of MSAN should
be different that ASAN.

Also, if memory sanitizer is an absolute requirement, then please build V8 for
MSAN with the ARM simulator, which doesn't JIT code. This should work out of the
box without any code changes, or at least with a much smaller, less fragile CL.


Otherwise, my fundamental issue with the CL is how sloppily it handles
unpoisoning of the JITTed stack. I would much prefer a tighter approach that
unpoisoned precisely. I think that would involve some combination of the
following:

- Track the "top" sp per-thread precisely when entering JIT code. When entering
the runtime, find the bottom precisely. Only unpoison the delta. Probably
involves some short assembly snippets. Even better: understand what gets
accessed by the C++ code (parameters, frame pointer stack crawl with some info
earch frame, etc.) and only unpoison that.
- Don't unpoison all JIT stack space when entering the deoptimizer. Only
unpoision the copied frames in the input buffer. 
- Don't explicitly unpoison parameters. They are simply part of the region of
stack above the current C++ frame and the top of JITTED code frames.

https://codereview.chromium.org/26006004/diff/33001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/26006004/diff/33001/src/deoptimizer.cc#newcod...
src/deoptimizer.cc:114: Deoptimizer* deoptimizer = new Deoptimizer(isolate,
This big unpoison of the stack is total overkill. The deoptimizer copies a
specific number of frames from the stack to the input_ of buffer of the
Deoptimizer. This is done after returning from this routine in the hand-written
assembly code of the deoptimizer. After that, the
Deoptimizer::ComputeOutputFrames method inspect this buffer to build the output
frames. You should be able to precisely unpoison just the input buffer for which
you have the exact size at the beginning of Deoptimizer::ComputeOutputFrames.

https://codereview.chromium.org/26006004/diff/33001/src/msan.h
File src/msan.h (right):

https://codereview.chromium.org/26006004/diff/33001/src/msan.h#newcode49
src/msan.h:49: __msan_unpoison(&dummy, 65536);     \
This seems totally arbitrary and extremely fragile. Why 64K? You will end up
unpoisoning stuff in C++ frames above small stacks of JIT frames. If you need to
specifically unpoison parts of the stack, you should know how much and be
precise about it.

I don't feel comfortable at all adding this in this form, since it seems to be a
bug just waiting to happen and it feels likely to break through unrelated to V8
in the future.

https://codereview.chromium.org/26006004/diff/33001/src/msan.h#newcode52
src/msan.h:52: __msan_unpoison(&(c), sizeof(c));   \
Why are these not covered by the 64K above? From the usage, it appears the
arguments are always parameters to the function this macro is embedded in. If
they are pushed before the call, then they lie at an address above locals in the
current frame?, so the &dummy call should cover them.

https://codereview.chromium.org/26006004/diff/33001/src/platform-posix.cc
File src/platform-posix.cc (right):

https://codereview.chromium.org/26006004/diff/33001/src/platform-posix.cc#new...
src/platform-posix.cc:71: #include "msan.h"
Here and elsewhere: None of these includes should be hard wired (they should be
conditionally included based on whether MEMORY_SANITIZER is active).

https://codereview.chromium.org/26006004/diff/33001/src/platform-posix.cc#new...
src/platform-posix.cc:211: #if defined(ADDRESS_SANITIZER) ||
defined(MEMORY_SANITIZER) || \
This change been landed separately, why is this in this CL?

[Evgeniy Stepanov, 2013-10-10 15:08:46]

On 2013/10/10 14:18:06, danno wrote:
> Why does there need to be specific support for MEMORY_SANITIZER at all in V8?
> There is no ADDRESS_SANITIZER support, and I think that this falls in the same
> category and I would argue it should be handled in the same way. Before
pursing
> this patch any further, I'd like to understand why V8's handling of MSAN
should
> be different that ASAN.

The difference is that ASan algorithm is conservative - it is OK to miss part of
the program execution (JIT, uninstrumented libraries, etc) as long as all memory
allocation/deallocation is intercepted.

MSan, on the other hand, needs to see all memory stores in the program.
Otherwise there is a possibility that we will report a memory location as
uninitialized when in fact it is not.

ASan w/o specific V8 support fails to detect addressability issues in JIT code,
which is something we can live with. MSan w/o V8 support prints lots of false
warnings. This prevents us from using MSan on any process that includes V8, and
those are pretty common.

 
> Also, if memory sanitizer is an absolute requirement, then please build V8 for
> MSAN with the ARM simulator, which doesn't JIT code. This should work out of
the
> box without any code changes, or at least with a much smaller, less fragile
CL.

Do you mean I can build Chrome that runs V8 without JIT that way? From the v8
wiki pages I'd need to run the resulting binary in a simulator shell...

> 
> 
> Otherwise, my fundamental issue with the CL is how sloppily it handles
> unpoisoning of the JITTed stack. I would much prefer a tighter approach that
> unpoisoned precisely. I think that would involve some combination of the
> following:
> 
> - Track the "top" sp per-thread precisely when entering JIT code. When
entering
> the runtime, find the bottom precisely. Only unpoison the delta. Probably
> involves some short assembly snippets. Even better: understand what gets
> accessed by the C++ code (parameters, frame pointer stack crawl with some info
> earch frame, etc.) and only unpoison that.

I think the "even better" part will make the patch larger, more intrusive and
fragile. Unpoisoning the stack delta only sound really good, I'll look into this
(it seems like there are very few places where C++ -> JIT transition happens?).
Sorry this CL was kind of rushed.

> - Don't unpoison all JIT stack space when entering the deoptimizer. Only
> unpoision the copied frames in the input buffer. 

Sounds great, thanks.

> - Don't explicitly unpoison parameters. They are simply part of the region of
> stack above the current C++ frame and the top of JITTED code frames.

See inline comments.

> 
> https://codereview.chromium.org/26006004/diff/33001/src/deoptimizer.cc
> File src/deoptimizer.cc (right):
> 
>
https://codereview.chromium.org/26006004/diff/33001/src/deoptimizer.cc#newcod...
> src/deoptimizer.cc:114: Deoptimizer* deoptimizer = new Deoptimizer(isolate,
> This big unpoison of the stack is total overkill. The deoptimizer copies a
> specific number of frames from the stack to the input_ of buffer of the
> Deoptimizer. This is done after returning from this routine in the
hand-written
> assembly code of the deoptimizer. After that, the
> Deoptimizer::ComputeOutputFrames method inspect this buffer to build the
output
> frames. You should be able to precisely unpoison just the input buffer for
which
> you have the exact size at the beginning of Deoptimizer::ComputeOutputFrames.
> 
> https://codereview.chromium.org/26006004/diff/33001/src/msan.h
> File src/msan.h (right):
> 
> https://codereview.chromium.org/26006004/diff/33001/src/msan.h#newcode49
> src/msan.h:49: __msan_unpoison(&dummy, 65536);     \
> This seems totally arbitrary and extremely fragile. Why 64K? You will end up
> unpoisoning stuff in C++ frames above small stacks of JIT frames. If you need
to
> specifically unpoison parts of the stack, you should know how much and be
> precise about it.
> 
> I don't feel comfortable at all adding this in this form, since it seems to be
a
> bug just waiting to happen and it feels likely to break through unrelated to
V8
> in the future.
> 
> https://codereview.chromium.org/26006004/diff/33001/src/msan.h#newcode52
> src/msan.h:52: __msan_unpoison(&(c), sizeof(c));   \
> Why are these not covered by the 64K above? From the usage, it appears the
> arguments are always parameters to the function this macro is embedded in. If
> they are pushed before the call, then they lie at an address above locals in
the
> current frame?, so the &dummy call should cover them.
> 
> https://codereview.chromium.org/26006004/diff/33001/src/platform-posix.cc
> File src/platform-posix.cc (right):
> 
>
https://codereview.chromium.org/26006004/diff/33001/src/platform-posix.cc#new...
> src/platform-posix.cc:71: #include "msan.h"
> Here and elsewhere: None of these includes should be hard wired (they should
be
> conditionally included based on whether MEMORY_SANITIZER is active).
> 
>
https://codereview.chromium.org/26006004/diff/33001/src/platform-posix.cc#new...
> src/platform-posix.cc:211: #if defined(ADDRESS_SANITIZER) ||
> defined(MEMORY_SANITIZER) || \
> This change been landed separately, why is this in this CL?

[Evgeniy Stepanov, 2013-10-10 15:08:54]

https://codereview.chromium.org/26006004/diff/33001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/26006004/diff/33001/src/deoptimizer.cc#newcod...
src/deoptimizer.cc:114: Deoptimizer* deoptimizer = new Deoptimizer(isolate,
On 2013/10/10 14:18:07, danno wrote:
> This big unpoison of the stack is total overkill. The deoptimizer copies a
> specific number of frames from the stack to the input_ of buffer of the
> Deoptimizer. This is done after returning from this routine in the
hand-written
> assembly code of the deoptimizer. After that, the
> Deoptimizer::ComputeOutputFrames method inspect this buffer to build the
output
> frames. You should be able to precisely unpoison just the input buffer for
which
> you have the exact size at the beginning of Deoptimizer::ComputeOutputFrames.

Thanks, this is very helpful!

https://codereview.chromium.org/26006004/diff/33001/src/msan.h
File src/msan.h (right):

https://codereview.chromium.org/26006004/diff/33001/src/msan.h#newcode52
src/msan.h:52: __msan_unpoison(&(c), sizeof(c));   \
On 2013/10/10 14:18:07, danno wrote:
> Why are these not covered by the 64K above? From the usage, it appears the
> arguments are always parameters to the function this macro is embedded in. If
> they are pushed before the call, then they lie at an address above locals in
the
> current frame?, so the &dummy call should cover them.

Uninit information for function arguments is passed though a special TLS slot,
unpoisoning stack won't affect it. This is an unfortunate limitation of
operating at LLVM IR level - we don't know the details of the function calling
convention and have to work with an abstact argument list.

https://codereview.chromium.org/26006004/diff/33001/src/platform-posix.cc
File src/platform-posix.cc (right):

https://codereview.chromium.org/26006004/diff/33001/src/platform-posix.cc#new...
src/platform-posix.cc:71: #include "msan.h"
On 2013/10/10 14:18:07, danno wrote:
> Here and elsewhere: None of these includes should be hard wired (they should
be
> conditionally included based on whether MEMORY_SANITIZER is active). 

But msan.h is in fact the place where things like MSAN_RUNTIME_FUNCTION are
defined in different ways depending on MEMORY_SANITIZER. If we include it
conditionally, then all uses of those annotations would also need to be
surrounded by #ifdef MEMORY_SANITIZER.

https://codereview.chromium.org/26006004/diff/33001/src/platform-posix.cc#new...
src/platform-posix.cc:211: #if defined(ADDRESS_SANITIZER) ||
defined(MEMORY_SANITIZER) || \
On 2013/10/10 14:18:07, danno wrote:
> This change been landed separately, why is this in this CL?

That was in a different repository, I think.

[eugenis, 2014-03-26 13:03:31]

We've decided to use the awesome simulator-arm64 in MSan builds. Most of these
changes are no longer needed, and we can test JITted code with MSan, too.

I've split the rest in several issues, please review them separately:

https://codereview.chromium.org/212863002/
https://codereview.chromium.org/212723003/
https://codereview.chromium.org/212833002/
https://codereview.chromium.org/212483004/