[llvm-commits] AddressSanitizer: start factoring out interception machinery (issue 5642046)

glider at google.com glider at google.com
Tue Feb 7 05:14:29 PST 2012


2 general comments:

1. You'll need to get rid of all the ASanish stuff in these files. This
is a bit tricky on Mac, so I'm not insisting on factoring out the branch
island allocator now.
2. Please keep interception.h clean. You don't need to put anything but
the macros intended to be used by the clients there.


http://codereview.appspot.com/5642046/diff/1/interception/interception.h
File interception/interception.h (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newcode33
interception/interception.h:33: // asan_interceptors.cc, you'll instead
need to:
Please rewrite this comment without mentioning ASan.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newcode87
interception/interception.h:87: // foo with interceptor for other
function.
"with an interceptor"

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newcode100
interception/interception.h:100: bool OverrideFunction(void *old_func,
void *new_func, void **orig_old_func);
You should not need these functions in the interface header.
Moreover, you don't need to define them for each platform.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newcode102
interception/interception.h:102: bool GetRealFunctionAddress(const char
*func_name, void **func_addr);
Any reason not to return the address instead of bool here?

http://codereview.appspot.com/5642046/diff/1/interception/interception_linux.cc
File interception/interception_linux.cc (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception_linux.cc#newcode10
interception/interception_linux.cc:10: bool OverrideFunction(void
*old_func, void *new_func, void **orig_old_func) {
Please remove this function.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc
File interception/interception_mac.cc (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc#newcode18
interception/interception_mac.cc:18: #include
"../mach_override/mach_override.h"
Please put mach_override/ under interception/ as the former should now
become a part of the latter.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc#newcode34
interception/interception_mac.cc:34: # define kIslandEnd (0xffdf0000 -
kPageSize)
This stuff is in fact ASan-dependent and may not work with TSan. We need
to implement those __asan_[de]allocate_island routines in each tool. And
they certainly should not have the "__asan" prefix.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc#newcode71
interception/interception_mac.cc:71: orig_old_func,
mach_override_ptr_custom should have a custom prefix, too.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc#newcode79
interception/interception_mac.cc:79: // not implemented
Please remove this function.

http://codereview.appspot.com/5642046/



More information about the llvm-commits mailing list