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#newcode114
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#newcode71
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#newcode211
src/platform-posix.cc:211: #if defined(ADDRESS_SANITIZER) ||
defined(MEMORY_SANITIZER) || \
This change been landed separately, why is this in this CL?
https://codereview.chromium.org/26006004/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
