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

Wed Feb 8 13:00:44 PST 2012

On Wed, Feb 8, 2012 at 12:25 PM, <timurrrr at 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.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<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://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<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/>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120208/0fd7bd20/attachment.html>