[llvm-commits] [ASan] The first version of the RTL for Windows (issue 5647052)
Aaron Ballman
aaron at aaronballman.com
Wed Feb 8 07:30:58 PST 2012
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
More information about the llvm-commits
mailing list