[llvm-commits] [ASan] The first version of the RTL for Windows (issue 5647052)

[timurrrr at google.com]

Addressed most of the comments, except the
_WIN32 vs WINDOWS ones.

Forgot to mention:
This code in asan_win.cc is not production quality, it's just an early
prototype; it works on some simple tests but not expected to work on any
non-trivial app.
I expect much of it to be rewritten when I write more tests, do more
debug and read more docs - especially the code near the "FIXME"
comments.
If it's OK I'd like to defer polishing the code there until we have
"this code is bad" data from tests and runs on non-trivial apps.

Kostya,
Re: _WIN32 vs WINDOWS - is this a big issue?
I expect to remove the new ifdefs eventually and there aren't too many
"old" ones in the other places.
If it turns out to be a problem - it'd be easy to `sed` through the
code; but if possible I'd like to do it in a separate patch and only
when it's a real problem.

Please note that _WIN32/_WIN64 are the default macros "cl" automatically
defines, see http://msdn.microsoft.com/en-us/library/b0084kay.aspx
Having our custom WINDOWS macro (what about 64-bits? WINDOWS64?) will be
somewhat against the standard win-code convention.

Also, my simple "cl *.cc" command line will become more than twice as
long :)
I don't want to rely on Makefiles/CMake (yet?) to build ASan/RTL for
Windows.


http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_interceptors.cc
File lib/asan/asan_interceptors.cc (right):

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_interceptors.cc#newcode32
lib/asan/asan_interceptors.cc:32: # include <intrin.h>  // FIXME: remove
when we start intercepting on Windows.
On 2012/02/08 18:03:10, kcc wrote:
> Why do you need this now?
Added a comment. Note it's a FIXME item, I'll need to re-do this anyways
once we start intercepting functions properly.

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc
File lib/asan/asan_win.cc (right):

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode37
lib/asan/asan_win.cc:37: // FIXME: what is mem_type?
On 2012/02/08 18:03:10, kcc wrote:
> memtype is a string used for reporting mmap errors
Got it, removed the comment.

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode88
lib/asan/asan_win.cc:88: CHECK(VirtualQuery(&mbi /* on stack */,
On 2012/02/08 18:19:20, Aaron.Ballman wrote:
> The stack will always be a single block, but I think what you're
getting there
> is including the stack's guard page.  Are you sure you want that?
Also, I think
> that's only getting you the reserved size and not the committed size.
I'd like to defer answering this question until we add tests for stack
accesses (correct and incorrect ones).
I'll definitely return to it once we have more tests to judge the
correctness.
Is it OK to commit without investigating it given I've added the FIXME?

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode98
lib/asan/asan_win.cc:98: ScopedLock lock(&dbghelp_lock);
On 2012/02/08 18:03:10, kcc wrote:
> OMG. Ok for now, but in the long run this is not going to work.
> We will need our own unwinder.

I might have misread MSDN earlies (or misreading it now?) and/or
copy-pasted this from some other code
but turns out CaptureStackBackTrace DOES NOT require the dbghelp lock.
I've removed it for now.

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode103
lib/asan/asan_win.cc:103: size_t cs_ret = CaptureStackBackTrace(1,
max_size, tmp, NULL),
Added a few FIXMEs to look at if the performance is a problem

On 2012/02/08 18:19:20, Aaron.Ballman wrote:
> Since you're already using DbgHelp APIs, would StackWalk64 make a bit
more
> sense?  IIRC, it's a bit more accurate than
> CaptureStackBackTrace.  Certainly it gives you more control.

> You may want to look at LLVMUnhandledExceptionFilter in Signals.inc
because a
> lot of the stack walking fun is already done there.  Perhaps we could
refactor
> it to use common code?

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode167
lib/asan/asan_win.cc:167: AsanLock::AsanLock(LinkerInitialized li) {
On 2012/02/08 18:03:10, kcc wrote:
> This is in fact *not* linker-initialized.
> Is there a way to create a linker-initialized lock on windows?
CRITICAL_SECTION contains nonzero values after InitializeCriticalSection
call, so it can't simply be zero-initizlized.

http://codereview.appspot.com/5647052/