[ASan] The first version of the RTL for Windows

[ASan] The first version of the RTL for Windows

Committed:
http://llvm.org/viewvc/llvm-project?view=rev&revision=150185

[timurrrr_at_google_com, 2012-02-08 14:49:12]

Hi Alexander, Kostya,

Can you please review this patch?

It makes some simple C tests pass when the tests are compiled with Clang and
linked with the runtime built using MSVS cl.

It has a lot of FIXMEs but I'd like to have this code under source control and
available to someone besides me.

This patch should be a no-op change for Linux and Mac.

Thanks,
Timur.

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_internal.h
File lib/asan/asan_internal.h (right):

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_internal.h#newc...
lib/asan/asan_internal.h:230: #  define ASAN_USE_EXTERNAL_SYMBOLIZER
__asan::WinSymbolize
Alternative suggestions are welcome if you believe this is too hacky.

[glider, 2012-02-08 15:30:09]

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_internal.h
File lib/asan/asan_internal.h (right):

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_internal.h#newc...
lib/asan/asan_internal.h:230: #  define ASAN_USE_EXTERNAL_SYMBOLIZER
__asan::WinSymbolize
On 2012/02/08 14:49:13, timurrrr_at_google_com wrote:
> Alternative suggestions are welcome if you believe this is too hacky.

LG

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

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode59
lib/asan/asan_win.cc:59: CHECK(fd == 2);  // Looks like we only use stderr for
AsanWrite?
I think we may want to use other fds someday (I'm considering output redirection
as one of the possible ways to integrate with Breakpad)

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode62
lib/asan/asan_win.cc:62: return fwrite(buf, 1, count, stderr);
The point of AsanWrite is to avoid memory allocations during writes, that's why
we're using syscalls on Linux and Mac.

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode142
lib/asan/asan_win.cc:142: IMAGEHLP_LINE64 info;
Shouldn't this be arch-dependent?

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode155
lib/asan/asan_win.cc:155: " %s+0x%lx", symbol->Name, offset);
Again, http://mail.python.org/pipermail/patches/2000-June/000871.html

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode201
lib/asan/asan_win.cc:201: // FIXME: is __declspec enough?
You may want to take a look at the TLS implementation in Chrome (I've no idea
:))

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode231
lib/asan/asan_win.cc:231: return getenv(name);
Does getenv() allocate memory? If so, you may run into a trouble.

[aaron_aaronballman.com, 2012-02-08 15:30:59]

On Wed, Feb 8, 2012 at 8:49 AM,  <timurrrr@google.com> wrote:
> Reviewers: kcc1, glider,
>
> Message:
> Hi Alexander, Kostya,
>
> Can you please review this patch?
>
> It makes some simple C tests pass when the tests are compiled with Clang
> and linked with the runtime built using MSVS cl.
>
> It has a lot of FIXMEs but I'd like to have this code under source
> control and available to someone besides me.
>
> This patch should be a no-op change for Linux and Mac.
>
> Thanks,
> Timur.
>
>
> http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_internal.h
> File lib/asan/asan_internal.h (right):
>
>
http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_internal.h#newc...
> lib/asan/asan_internal.h:230: #  define ASAN_USE_EXTERNAL_SYMBOLIZER
> __asan::WinSymbolize
> Alternative suggestions are welcome if you believe this is too hacky.
>
> Description:
> [ASan] The first version of the RTL for Windows
>
> Please review this at http://codereview.appspot.com/5647052/
>
> Affected files:
>   M lib/asan/asan_allocator.cc
>   M lib/asan/asan_interceptors.cc
>   M lib/asan/asan_internal.h
>   M lib/asan/asan_rtl.cc
>   A lib/asan/asan_win.cc

> +++ b/lib/asan/asan_win.cc

> +// Disable "getenv and strcpy are unsafe" warnings.
> +#pragma warning(disable: 4996)

Instead of the pragma to disable warnings, why not just fix getenv and strcpy?

> +void *AsanMprotect(uintptr_t fixed_addr, size_t size) {
> +  return VirtualAlloc((LPVOID)fixed_addr, size,
> +                      MEM_RESERVE | MEM_COMMIT, PAGE_NOACCESS);
> +}

Shouldn't this be doing a VirtualProtect instead of allocating?

> +void AsanThread::SetThreadStackTopAndBottom() {
> +  MEMORY_BASIC_INFORMATION mbi;
> +  CHECK(VirtualQuery(&mbi /* on stack */,
> +                    &mbi, sizeof(mbi)) != 0);
> +  // FIXME: is it possible for the stack to not be a single allocation?
> +  stack_top_ = (uintptr_t)mbi.BaseAddress + mbi.RegionSize;
> +  stack_bottom_ = (uintptr_t)mbi.AllocationBase;
> +}

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.

> +  size_t cs_ret = CaptureStackBackTrace(1, max_size, tmp, NULL),
> +         offset = 0;

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?

~Aaron

[glider, 2012-02-08 15:40:33]

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

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_interceptors.cc...
lib/asan/asan_interceptors.cc:32: # include <intrin.h>  // FIXME: remove when we
start intercepting on Windows.
Do you still need it after you've declared _ReturnAddress?

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

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode231
lib/asan/asan_win.cc:231: return getenv(name);
Hint:
http://code.google.com/p/gperftools/source/browse/trunk/src/base/sysinfo.cc

[timurrrr_at_google_com, 2012-02-08 16:16:01]

PTAL


On Wed, Feb 8, 2012 at 7:30 PM, Aaron Ballman <aaron@aaronballman.com> wrote:
Aaron,
can you please make the comments in the web interface of rietveld?
(by double-clicking the source line you'd like to comment on) 
It's a bit more convenient to see all the comments in the web diff than having a
half there and another half in the email.
Thanks!

>> +// Disable "getenv and strcpy are unsafe" warnings.
>> +#pragma warning(disable: 4996)
> Instead of the pragma to disable warnings, why not just fix getenv and strcpy?
Turns out I've forgotten to remove "strcpy" from this comment;
and now I've got rid of getenv() too.

>> +void *AsanMprotect(uintptr_t fixed_addr, size_t size) {
>> +  return VirtualAlloc((LPVOID)fixed_addr, size,
>> +                      MEM_RESERVE | MEM_COMMIT, PAGE_NOACCESS);
>> +}
> Shouldn't this be doing a VirtualProtect instead of allocating?
It turns out AsanMprotect must allocate + protect, so VirtualProtect is not
enough.
I don't like the definition of AsanMprotect too :)

> 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.
That's a good question.
I'm not sure. I've added a comment so I'll definitely come back to this question
once I start writing tests for stack OOBs.

> 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?
I'll decide later.
First we need to be sure that CaptureStackBackTrace is not what we want.

--
Thanks,
Timur

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

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_interceptors.cc...
lib/asan/asan_interceptors.cc:32: # include <intrin.h>  // FIXME: remove when we
start intercepting on Windows.
On 2012/02/08 15:40:33, glider wrote:
> Do you still need it after you've declared _ReturnAddress?
Yes, intrinsic versions of memcpy/memset are defined there.

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

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode59
lib/asan/asan_win.cc:59: CHECK(fd == 2);  // Looks like we only use stderr for
AsanWrite?
On 2012/02/08 15:30:09, glider wrote:
> I think we may want to use other fds someday (I'm considering output
redirection
> as one of the possible ways to integrate with Breakpad)
Replaced CHECK with "if()"

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode62
lib/asan/asan_win.cc:62: return fwrite(buf, 1, count, stderr);
On 2012/02/08 15:30:09, glider wrote:
> The point of AsanWrite is to avoid memory allocations during writes, that's
why
> we're using syscalls on Linux and Mac.
I know, that's why I've added a FIXME.

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode142
lib/asan/asan_win.cc:142: IMAGEHLP_LINE64 info;
On 2012/02/08 15:30:09, glider wrote:
> Shouldn't this be arch-dependent?
No, see the link in the code above
+
http://msdn.microsoft.com/en-us/library/ms681330(v=vs.85).aspx

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode155
lib/asan/asan_win.cc:155: " %s+0x%lx", symbol->Name, offset);
On 2012/02/08 15:30:09, glider wrote:
> Again, http://mail.python.org/pipermail/patches/2000-June/000871.html

Done.

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode201
lib/asan/asan_win.cc:201: // FIXME: is __declspec enough?
On 2012/02/08 15:30:09, glider wrote:
> You may want to take a look at the TLS implementation in Chrome (I've no idea
> :))
That's what FIXME is about.
I'd like to decide later.

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode231
lib/asan/asan_win.cc:231: return getenv(name);
On 2012/02/08 15:30:09, glider wrote:
> Does getenv() allocate memory? If so, you may run into a trouble.
Hm, in fact we can just return NULL for now as it's only used for ASAN_OPTIONS.

[glider, 2012-02-08 16:41:55]

I see no problems with this code, awaiting for others.

[kcc, 2012-02-08 18:03:10]

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

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_allocator.cc#ne...
lib/asan/asan_allocator.cc:692: #if defined(_WIN32)
instead of the ifdef clatter, I'd prefer to have 
if (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...
lib/asan/asan_interceptors.cc:32: # include <intrin.h>  // FIXME: remove when we
start intercepting on Windows.
Why do you need this now?

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_interceptors.cc...
lib/asan/asan_interceptors.cc:626: #ifdef _WIN32
if (WINDOWS) please
or "if (ASAN_WINDOWS)"

also, feel free to change the code below to if (ASAN_APPLE)

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?
memtype is a string used for reporting mmap errors

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

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode167
lib/asan/asan_win.cc:167: AsanLock::AsanLock(LinkerInitialized li) {
This is in fact *not* linker-initialized.
Is there a way to create a linker-initialized lock on windows?

[Aaron.Ballman, 2012-02-08 18:19:20]

Added previous comments as well as one new one about DbgHelp being thread
unsafe.

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#newcode88
lib/asan/asan_win.cc:88: CHECK(VirtualQuery(&mbi /* on stack */,
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.

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. 

Even with our own unwinder, the symbolicator is single-threaded only on Windows.
 So we'll need some form of locking in WinSymbolize.

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),
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?

[kcc, 2012-02-08 18:38:07]

On Wed, Feb 8, 2012 at 10:19 AM, <Aaron.Ballman@gmail.com> wrote:

> Added previous comments as well as one new one about DbgHelp being
> thread unsafe.
>
>
>
>
http://codereview.appspot.com/**5647052/diff/6001/lib/asan/**asan_win.cc<http...
> File lib/asan/asan_win.cc (right):
>
> http://codereview.appspot.com/**5647052/diff/6001/lib/asan/**
>
asan_win.cc#newcode88<http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode88>
> lib/asan/asan_win.cc:88: CHECK(VirtualQuery(&mbi /* on stack */,
> 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.
>
>
> http://codereview.appspot.com/**5647052/diff/6001/lib/asan/**
>
asan_win.cc#newcode98<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.
>>
>
> Even with our own unwinder, the symbolicator is single-threaded only on
> Windows.  So we'll need some form of locking in WinSymbolize.
>

I don't care about the performance of symbolicator too much -- it is
invoked only once.
But the performance of the unwinder is very important since it is invoked
on every malloc/free.
Even our super fast unwinder on linux is responsible for up-to 20% of asan
run-time cost.



>
> http://codereview.appspot.com/**5647052/diff/6001/lib/asan/**
>
asan_win.cc#newcode103<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),
> 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/<http://codereview.appspot.com/5647052/>
>

[kcc1, 2012-02-08 18:39:34]

On Wed, Feb 8, 2012 at 10:19 AM, <Aaron.Ballman@gmail.com> wrote:

> Added previous comments as well as one new one about DbgHelp being
> thread unsafe.
>
>
>
>
http://codereview.appspot.com/**5647052/diff/6001/lib/asan/**asan_win.cc<http...
> File lib/asan/asan_win.cc (right):
>
> http://codereview.appspot.com/**5647052/diff/6001/lib/asan/**
>
asan_win.cc#newcode88<http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode88>
> lib/asan/asan_win.cc:88: CHECK(VirtualQuery(&mbi /* on stack */,
>
> 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.
>
> http://codereview.appspot.com/**5647052/diff/6001/lib/asan/**
>
asan_win.cc#newcode98<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.
>>
>
> Even with our own unwinder, the symbolicator is single-threaded only on
> Windows.  So we'll need some form of locking in WinSymbolize.
>

[replying using the kosher e-mail]
I don't care about the performance of symbolicator too much -- it is
invoked only once.
But the performance of the unwinder is very important since it is invoked
on every malloc/free.
Even our super fast unwinder on linux is responsible for up-to 20% of asan
run-time cost.

--kcc


>
> http://codereview.appspot.com/**5647052/diff/6001/lib/asan/**
>
asan_win.cc#newcode103<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),
>
> 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/<http://codereview.appspot.com/5647052/>
>

[timurrrr_at_google_com, 2012-02-08 20:25:41]

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

[kcc, 2012-02-08 21:01:05]

On Wed, Feb 8, 2012 at 12:25 PM, <timurrrr@google.com> wrote:

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

I am ok with it, as long as the code builds and does not hurt anyone's
build bots.


>
> Kostya,
> Re: _WIN32 vs WINDOWS - is this a big issue?
>

yes. Please do

#if _WIN32 || _WIN64
#define ASAN_WINDOWS 1
#else
# define ASAN_WINDOWS 0
#endif

and use if (ASAN_WINDOWS) everywhere else in the code.

This way is much more readable and we can make sure that the code compiles
on all platforms and does not depend on OS-specific stuff.

--kcc


> 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<http://msdn.microsoft...
> 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<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<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<http...
> File lib/asan/asan_win.cc (right):
>
> http://codereview.appspot.com/**5647052/diff/6001/lib/asan/**
>
asan_win.cc#newcode37<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<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<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<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<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/<http://codereview.appspot.com/5647052/>
>

[timurrrr_at_google_com, 2012-02-09 13:50:11]

PTAL

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

http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_allocator.cc#ne...
lib/asan/asan_allocator.cc:692: #if defined(_WIN32)
On 2012/02/08 18:03:10, kcc wrote:
> instead of the ifdef clatter, I'd prefer to have 
> if (WINDOWS)

Done.

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...
lib/asan/asan_interceptors.cc:626: #ifdef _WIN32
On 2012/02/08 18:03:10, kcc wrote:
> if (WINDOWS) please
> or "if (ASAN_WINDOWS)"
You can't do that - it won't compile on Linux (where we don't include the
headers defining memcpy/memset )
Here "#ifdef _WIN32" is doing what it should -> compile some code only on
Windows
 
> also, feel free to change the code below to if (ASAN_APPLE)
Leaving this up to Alexander, as this might also not compile on non-Apple

http://codereview.appspot.com/5647052/diff/10/lib/asan/asan_internal.h
File lib/asan/asan_internal.h (right):

http://codereview.appspot.com/5647052/diff/10/lib/asan/asan_internal.h#newcode63
lib/asan/asan_internal.h:63: # define ASAN_LINUX   1
Defined the ASAN_* macros.

I don't like the N^2 number of definitions though

[glider, 2012-02-09 13:53:10]

http://codereview.appspot.com/5647052/diff/10/lib/asan/asan_internal.h
File lib/asan/asan_internal.h (right):

http://codereview.appspot.com/5647052/diff/10/lib/asan/asan_internal.h#newcode63
lib/asan/asan_internal.h:63: # define ASAN_LINUX   1
#if defined(__linux__)
# define ASAN_LINUX 1
#else
# define ASAN_LINUX 0
#endif

[timurrrr_at_google_com, 2012-02-09 13:55:41]

Shame on me :)

http://codereview.appspot.com/5647052/diff/10/lib/asan/asan_internal.h
File lib/asan/asan_internal.h (right):

http://codereview.appspot.com/5647052/diff/10/lib/asan/asan_internal.h#newcode63
lib/asan/asan_internal.h:63: # define ASAN_LINUX   1
On 2012/02/09 13:53:11, glider wrote:
> #if defined(__linux__)
> # define ASAN_LINUX 1
> #else
> # define ASAN_LINUX 0
> #endif

Done.

[timurrrr_at_google_com, 2012-02-09 16:50:24]

Patch set 6 - rebased against the fresh trunk, otherwise the patch wasn't
applying anymore.

No new features/code/etc changed between PS5 and PS6

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

http://codereview.appspot.com/5647052/diff/6002/lib/asan/asan_interceptors.cc...
lib/asan/asan_interceptors.cc:29: # include <dlfcn.h>
this was removed upstream

[kcc, 2012-02-09 17:21:57]

Looks good, please commit (and watch the bots).

[timurrrr_at_google_com, 2012-02-09 17:24:36]

r150185, thanks a lot!