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

[samsonov at google.com]

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:
On 2012/02/07 13:14:29, glider wrote:
> Please rewrite this comment without mentioning ASan.

Done.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newcode38
interception/interception.h:38: //           DECLARE_REAL(...); will be
located inside namespaces.
On 2012/02/07 12:39:16, timurrrr_at_google_com wrote:
> "if ... are",
> not
> "if ... will be ..."

Done.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newcode87
interception/interception.h:87: // foo with interceptor for other
function.
On 2012/02/07 13:14:29, glider wrote:
> "with an interceptor"

Done.

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);
On 2012/02/07 13:14:29, glider wrote:
> You should not need these functions in the interface header.
Not sure. When the macros expand, the actual code in ASan RTL will have
calls to OverrideFunction(...), so we need to add them to some headers
anyway.
> Moreover, you don't need to define them for each platform.
Done, although I don't like the idea of different sets of functions
present in header 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);
On 2012/02/07 13:14:29, glider wrote:
> Any reason not to return the address instead of bool here?

To simplify a macro to a plain function call and avoid casts.

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) {
On 2012/02/07 13:14:29, glider wrote:
> Please remove this function.

Done.

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"
On 2012/02/07 13:14:29, glider wrote:
> Please put mach_override/ under interception/ as the former should now
become a
> part of the latter.

Added a FIXME. I'd rather do this in other commit.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc#newcode34
interception/interception_mac.cc:34: # define kIslandEnd (0xffdf0000 -
kPageSize)
On 2012/02/07 13:14:29, glider wrote:
> 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.

Right. Returned island allocation to asan_mac.cc and did a re-naming.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc#newcode71
interception/interception_mac.cc:71: orig_old_func,
On 2012/02/07 13:14:29, glider wrote:
> mach_override_ptr_custom should have a custom prefix, too.

So, you'd need different mach_override_ptr for each tool? (disregarding
custom allocate/deallocate island)

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc#newcode79
interception/interception_mac.cc:79: // not implemented
On 2012/02/07 13:14:29, glider wrote:
> Please remove this function.

Done.

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