<br><br><div class="gmail_quote">On Wed, Feb 8, 2012 at 12:25 PM,  <span dir="ltr"><<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Addressed most of the comments, except the<br>
_WIN32 vs WINDOWS ones.<br>
<br>
Forgot to mention:<br>
This code in asan_win.cc is not production quality, it's just an early<br>
prototype; it works on some simple tests but not expected to work on any<br>
non-trivial app.<br>
I expect much of it to be rewritten when I write more tests, do more<br>
debug and read more docs - especially the code near the "FIXME"<br>
comments.<br>
If it's OK I'd like to defer polishing the code there until we have<br>
"this code is bad" data from tests and runs on non-trivial apps.<br></blockquote><div><br></div><div>I am ok with it, as long as the code builds and does not hurt anyone's build bots. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<br>
Kostya,<br>
Re: _WIN32 vs WINDOWS - is this a big issue?<br></blockquote><div><br></div><div>yes. Please do </div><div><br></div><div>#if _WIN32 || _WIN64</div><div>#define ASAN_WINDOWS 1</div><div>#else </div><div># define ASAN_WINDOWS 0</div>

<div>#endif</div><div><br></div><div>and use if (ASAN_WINDOWS) everywhere else in the code. </div><div><br></div><div>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. </div>

<div><br></div><div>--kcc  </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I expect to remove the new ifdefs eventually and there aren't too many<br>
"old" ones in the other places.<br>
If it turns out to be a problem - it'd be easy to `sed` through the<br>
code; but if possible I'd like to do it in a separate patch and only<br>
when it's a real problem.<br>
<br>
Please note that _WIN32/_WIN64 are the default macros "cl" automatically<br>
defines, see <a href="http://msdn.microsoft.com/en-us/library/b0084kay.aspx" target="_blank">http://msdn.microsoft.com/en-<u></u>us/library/b0084kay.aspx</a><br>
Having our custom WINDOWS macro (what about 64-bits? WINDOWS64?) will be<br>
somewhat against the standard win-code convention.<br>
<br>
Also, my simple "cl *.cc" command line will become more than twice as<br>
long :)<br>
I don't want to rely on Makefiles/CMake (yet?) to build ASan/RTL for<br>
Windows.<div class="im"><br>
<br>
<br>
<a href="http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_interceptors.cc" target="_blank">http://codereview.appspot.com/<u></u>5647052/diff/6001/lib/asan/<u></u>asan_interceptors.cc</a><br>
File lib/asan/asan_interceptors.cc (right):<br>
<br>
<a href="http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_interceptors.cc#newcode32" target="_blank">http://codereview.appspot.com/<u></u>5647052/diff/6001/lib/asan/<u></u>asan_interceptors.cc#newcode32</a><br>


lib/asan/asan_interceptors.cc:<u></u>32: # include <intrin.h>  // FIXME: remove<br>
when we start intercepting on Windows.<br></div><div class="im">
On 2012/02/08 18:03:10, kcc wrote:<br>
</div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Why do you need this now?<br>
</blockquote></div>
Added a comment. Note it's a FIXME item, I'll need to re-do this anyways<br>
once we start intercepting functions properly.<div class="im"><br>
<br>
<a href="http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc" target="_blank">http://codereview.appspot.com/<u></u>5647052/diff/6001/lib/asan/<u></u>asan_win.cc</a><br>
File lib/asan/asan_win.cc (right):<br>
<br>
</div><div class="im"><a href="http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode37" target="_blank">http://codereview.appspot.com/<u></u>5647052/diff/6001/lib/asan/<u></u>asan_win.cc#newcode37</a><br>


lib/asan/asan_win.cc:37: // FIXME: what is mem_type?<br></div><div class="im">
On 2012/02/08 18:03:10, kcc wrote:<br>
</div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
memtype is a string used for reporting mmap errors<br>
</blockquote></div>
Got it, removed the comment.<div class="im"><br>
<br>
<a href="http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode88" target="_blank">http://codereview.appspot.com/<u></u>5647052/diff/6001/lib/asan/<u></u>asan_win.cc#newcode88</a><br>
lib/asan/asan_win.cc:88: CHECK(VirtualQuery(&mbi /* on stack */,<br></div><div class="im">
On 2012/02/08 18:19:20, Aaron.Ballman wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The stack will always be a single block, but I think what you're<br>
</blockquote>
getting there<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
is including the stack's guard page.  Are you sure you want that?<br>
</blockquote>
Also, I think<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
that's only getting you the reserved size and not the committed size.<br>
</blockquote></div>
I'd like to defer answering this question until we add tests for stack<br>
accesses (correct and incorrect ones).<br>
I'll definitely return to it once we have more tests to judge the<br>
correctness.<br>
Is it OK to commit without investigating it given I've added the FIXME?<div class="im"><br>
<br>
<a href="http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode98" target="_blank">http://codereview.appspot.com/<u></u>5647052/diff/6001/lib/asan/<u></u>asan_win.cc#newcode98</a><br>
lib/asan/asan_win.cc:98: ScopedLock lock(&dbghelp_lock);<br>
On 2012/02/08 18:03:10, kcc wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
OMG. Ok for now, but in the long run this is not going to work.<br>
We will need our own unwinder.<br>
</blockquote>
<br></div>
I might have misread MSDN earlies (or misreading it now?) and/or<br>
copy-pasted this from some other code<br>
but turns out CaptureStackBackTrace DOES NOT require the dbghelp lock.<br>
I've removed it for now.<div class="im"><br>
<br>
<a href="http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode103" target="_blank">http://codereview.appspot.com/<u></u>5647052/diff/6001/lib/asan/<u></u>asan_win.cc#newcode103</a><br>
lib/asan/asan_win.cc:103: size_t cs_ret = CaptureStackBackTrace(1,<br>
max_size, tmp, NULL),<br></div>
Added a few FIXMEs to look at if the performance is a problem<div class="im"><br>
<br>
On 2012/02/08 18:19:20, Aaron.Ballman wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Since you're already using DbgHelp APIs, would StackWalk64 make a bit<br>
</blockquote>
more<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
sense?  IIRC, it's a bit more accurate than<br>
CaptureStackBackTrace.  Certainly it gives you more control.<br>
</blockquote>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
You may want to look at LLVMUnhandledExceptionFilter in Signals.inc<br>
</blockquote>
because a<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
lot of the stack walking fun is already done there.  Perhaps we could<br>
</blockquote>
refactor<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
it to use common code?<br>
</blockquote>
<br>
</div><div class="im"><a href="http://codereview.appspot.com/5647052/diff/6001/lib/asan/asan_win.cc#newcode167" target="_blank">http://codereview.appspot.com/<u></u>5647052/diff/6001/lib/asan/<u></u>asan_win.cc#newcode167</a><br>


lib/asan/asan_win.cc:167: AsanLock::AsanLock(<u></u>LinkerInitialized li) {<br></div><div class="im">
On 2012/02/08 18:03:10, kcc wrote:<br>
</div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This is in fact *not* linker-initialized.<br>
Is there a way to create a linker-initialized lock on windows?<br>
</blockquote></div>
CRITICAL_SECTION contains nonzero values after InitializeCriticalSection<br>
call, so it can't simply be zero-initizlized.<br>
<br>
<a href="http://codereview.appspot.com/5647052/" target="_blank">http://codereview.appspot.com/<u></u>5647052/</a><br>
</blockquote></div><br>