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

[Aaron Ballman]

On Wed, Feb 8, 2012 at 8:49 AM,  <timurrrr at 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#newcode230
> 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