[ASan] Intercept CreateThread on Windows

[ASan] Intercept CreateThread on Windows

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

[timurrrr_at_google_com, 2012-02-24 14:14:28]

Hi Alexander, Alexey,

Can you please review this patch?

Thanks!

http://codereview.appspot.com/5699064/diff/1/lib/asan/asan_interceptors_win.h
File lib/asan/asan_interceptors_win.h (right):

http://codereview.appspot.com/5699064/diff/1/lib/asan/asan_interceptors_win.h...
lib/asan/asan_interceptors_win.h:16: // Reverse include guard.
Please pay special attention to this part.
Alternative suggestions are welcome!

Please note that we do want to re-use some of the macros defined in
asan_interceptors.cc but don't want to move them to asan_interceptors.h

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

http://codereview.appspot.com/5699064/diff/1/lib/asan/asan_win.cc#newcode50
lib/asan/asan_win.cc:50: CHECK(VirtualFree(addr, size, MEM_DECOMMIT));
Threaded tests fail if we have CreateThread but not AsanUnmap

[samsonov, 2012-02-24 14:35:52]

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

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_interceptors_wi...
lib/asan/asan_interceptors_win.h:21: 
iwyu "interception/interception.h" ? Or it's 2-step reverse include guard?

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_interceptors_wi...
lib/asan/asan_interceptors_win.h:24: DWORD (__stdcall *start_routine)(void*),
void* arg,
You use DWORD, GET_STACK_TRACE_HERE, AsanThread etc. and don't include any
headers. Is this planned, i.e. do you really want to assume that someone will
include "asan_thread.h" etc. before including your header along the lines of
"asan_interceptors.cc"?

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

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_thread.h#newcode73
lib/asan/asan_thread.h:73: ThreadReturnType ThreadStart();
ThreadStartRoutineReturnType?

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_thread.h#newcode99
lib/asan/asan_thread.h:99: ThreadProc start_routine_;
ThreadStartRoutineType?

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
File lib/asan/interception/interception.h (right):

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:109: // foo with an interceptor for other
function.
Move this comment before DEFINE_REAL?

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:126: INTERCEPTOR_EX(ret_type, , func,
__VA_ARGS__)
...ret_type, /*no convention*/, func, ... ?

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:128: #define INTERCEPTOR_WINAPI(ret_type,
func, ...) \
Do we want to have similar macro for other OS? Maybe, it's better to move this
into interception_win.h and add comment here?

[ramosian.glider, 2012-02-24 14:36:40]

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

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_internal.h#newc...
lib/asan/asan_internal.h:263: # define THREADPROC
THREAD_CALLING_CONV or something more readable.

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_internal.h#newc...
lib/asan/asan_internal.h:281: typedef ThreadReturnType (THREADPROC
*ThreadProc)(void* param);
I think "thread_return_t" is more consistent with the rest of the code.

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

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_thread.h#newcode68
lib/asan/asan_thread.h:68: static AsanThread *Create(int parent_tid, ThreadProc
start_routine,
thread_callback_t, worker_t?

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
File lib/asan/interception/interception.h (right):

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:108: // without defining INTERCEPTOR(...,
foo, ...). For example, if you override
Please move this comment below or rewrite it.

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:117: DEFINE_REAL_EX(ret_type, , func,
__VA_ARGS__);
I think it's better to put something here as a missing argument.
(), maybe?

[timurrrr_at_google_com, 2012-02-24 15:10:39]

Done, PTAL

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

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_interceptors_wi...
lib/asan/asan_interceptors_win.h:21: 
On 2012/02/24 14:35:53, samsonov wrote:
> iwyu "interception/interception.h" ? Or it's 2-step reverse include guard?

On the second thought, I've decided to move the below code into
asan_interceptors.cc and avoid creating a new file for now.

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

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_internal.h#newc...
lib/asan/asan_internal.h:263: # define THREADPROC
On 2012/02/24 14:36:40, ramosian.glider wrote:
> THREAD_CALLING_CONV or something more readable.

Done.

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_internal.h#newc...
lib/asan/asan_internal.h:281: typedef ThreadReturnType (THREADPROC
*ThreadProc)(void* param);
On 2012/02/24 14:36:40, ramosian.glider wrote:
> I think "thread_return_t" is more consistent with the rest of the code.

Done.

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

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_thread.h#newcode68
lib/asan/asan_thread.h:68: static AsanThread *Create(int parent_tid, ThreadProc
start_routine,
On 2012/02/24 14:36:40, ramosian.glider wrote:
> thread_callback_t, worker_t?

Done.

http://codereview.appspot.com/5699064/diff/4001/lib/asan/asan_thread.h#newcode73
lib/asan/asan_thread.h:73: ThreadReturnType ThreadStart();
On 2012/02/24 14:35:53, samsonov wrote:
> ThreadStartRoutineReturnType?
thread_return_t

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
File lib/asan/interception/interception.h (right):

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:108: // without defining INTERCEPTOR(...,
foo, ...). For example, if you override
On 2012/02/24 14:36:40, ramosian.glider wrote:
> Please move this comment below or rewrite it.

Done.

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:109: // foo with an interceptor for other
function.
On 2012/02/24 14:35:53, samsonov wrote:
> Move this comment before DEFINE_REAL?

Done.

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:117: DEFINE_REAL_EX(ret_type, , func,
__VA_ARGS__);
On 2012/02/24 14:36:40, ramosian.glider wrote:
> I think it's better to put something here as a missing argument.
> (), maybe?

Done.

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:126: INTERCEPTOR_EX(ret_type, , func,
__VA_ARGS__)
On 2012/02/24 14:35:53, samsonov wrote:
> ...ret_type, /*no convention*/, func, ... ?

Done.

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:128: #define INTERCEPTOR_WINAPI(ret_type,
func, ...) \
On 2012/02/24 14:35:53, samsonov wrote:
> Do we want to have similar macro for other OS?
We might, but highly unlikely we'll need to.

> Maybe, it's better to move this
> into interception_win.h and add comment here?

[samsonov, 2012-02-24 15:20:37]

lg modulo comments

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
File lib/asan/interception/interception.h (right):

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:128: #define INTERCEPTOR_WINAPI(ret_type,
func, ...) \
On 2012/02/24 15:10:39, timurrrr_at_google_com wrote:
> On 2012/02/24 14:35:53, samsonov wrote:
> > Do we want to have similar macro for other OS?
> We might, but highly unlikely we'll need to.
> 
> > Maybe, it's better to move this
> > into interception_win.h and add comment here?
Not sure if we need to have INTERCEPTOR_WINAPI on Linux and Mac OS, but you may
leave it here if you wish.

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

http://codereview.appspot.com/5699064/diff/1017/lib/asan/asan_interceptors.cc...
lib/asan/asan_interceptors.cc:585: DWORD (__stdcall *start_routine)(void*),
void* arg,
why not use thread_callback_t here and in pthread_create interceptor?

http://codereview.appspot.com/5699064/diff/1017/lib/asan/asan_interceptors.cc...
lib/asan/asan_interceptors.cc:600: }  // namespace
// namespace __asan

[timurrrr_at_google_com, 2012-02-24 15:24:58]

PTAL

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
File lib/asan/interception/interception.h (right):

http://codereview.appspot.com/5699064/diff/4001/lib/asan/interception/interce...
lib/asan/interception/interception.h:128: #define INTERCEPTOR_WINAPI(ret_type,
func, ...) \
On 2012/02/24 15:20:38, samsonov wrote:
> On 2012/02/24 15:10:39, timurrrr_at_google_com wrote:
> > On 2012/02/24 14:35:53, samsonov wrote:
> > > Do we want to have similar macro for other OS?
> > We might, but highly unlikely we'll need to.
> > 
> > > Maybe, it's better to move this
> > > into interception_win.h and add comment here?
> Not sure if we need to have INTERCEPTOR_WINAPI on Linux and Mac OS, but you
may
> leave it here if you wish.

Done.

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

http://codereview.appspot.com/5699064/diff/1017/lib/asan/asan_interceptors.cc...
lib/asan/asan_interceptors.cc:585: DWORD (__stdcall *start_routine)(void*),
void* arg,
On 2012/02/24 15:20:38, samsonov wrote:
> why not use thread_callback_t here and in pthread_create interceptor?
I want the types here to be comparable with that in the docs (e.g. MSDN).

If the types begin to mismatch, the compile will report an error at the first
use (line 592)

http://codereview.appspot.com/5699064/diff/1017/lib/asan/asan_interceptors.cc...
lib/asan/asan_interceptors.cc:600: }  // namespace
On 2012/02/24 15:20:38, samsonov wrote:
> // namespace __asan

Done.

[ramosian.glider, 2012-02-24 15:27:57]

LGTM

[samsonov, 2012-02-24 15:32:34]

LGTM

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

http://codereview.appspot.com/5699064/diff/1017/lib/asan/asan_interceptors.cc...
lib/asan/asan_interceptors.cc:585: DWORD (__stdcall *start_routine)(void*),
void* arg,
On 2012/02/24 15:24:59, timurrrr_at_google_com wrote:
> On 2012/02/24 15:20:38, samsonov wrote:
> > why not use thread_callback_t here and in pthread_create interceptor?
> I want the types here to be comparable with that in the docs (e.g. MSDN).
> 
> If the types begin to mismatch, the compile will report an error at the first
> use (line 592)

reasonable

[timurrrr_at_google_com, 2012-02-24 15:34:04]

r151366, thanks!