Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(935)

Issue 5642046: AddressSanitizer: start factoring out interception machinery (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 1 month ago by samsonov
Modified:
12 years, 1 month ago
CC:
llvm-commits_cs.uiuc.edu
Base URL:
https://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/asan/
Visibility:
Public.

Patch Set 1 #

Total comments: 24

Patch Set 2 : (addresses glider's and timurrrr's comments) #

Total comments: 2

Patch Set 3 : (add __interception namespace, move OS-specific details under private headers) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+363 lines, -177 lines) Patch
M Makefile.mk View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M Makefile.old View 1 2 5 chunks +10 lines, -3 lines 0 comments Download
M asan_interceptors.h View 1 2 1 chunk +1 line, -60 lines 0 comments Download
M asan_interceptors.cc View 1 2 4 chunks +37 lines, -89 lines 0 comments Download
M asan_mac.h View 1 2 2 chunks +0 lines, -10 lines 0 comments Download
M asan_mac.cc View 1 2 3 chunks +11 lines, -12 lines 0 comments Download
M asan_malloc_mac.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
A interception/Makefile.mk View 1 chunk +22 lines, -0 lines 0 comments Download
A interception/interception.h View 1 2 1 chunk +136 lines, -0 lines 0 comments Download
A interception/interception_linux.h View 1 2 1 chunk +33 lines, -0 lines 0 comments Download
A interception/interception_linux.cc View 1 2 1 chunk +27 lines, -0 lines 0 comments Download
A interception/interception_mac.h View 1 2 1 chunk +47 lines, -0 lines 0 comments Download
A interception/interception_mac.cc View 1 2 1 chunk +34 lines, -0 lines 0 comments Download

Messages

Total messages: 12
samsonov
12 years, 1 month ago (2012-02-07 12:30:28 UTC) #1
timurrrr_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#newcode38 interception/interception.h:38: // DECLARE_REAL(...); will be located inside namespaces. "if ...
12 years, 1 month ago (2012-02-07 12:39:16 UTC) #2
glider
2 general comments: 1. You'll need to get rid of all the ASanish stuff in ...
12 years, 1 month ago (2012-02-07 13:14:28 UTC) #3
samsonov
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, ...
12 years, 1 month ago (2012-02-07 14:31:32 UTC) #4
glider
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#newcode100 interception/interception.h:100: bool OverrideFunction(void *old_func, void *new_func, void **orig_old_func); #define INTERCEPT_FUNCTION(func) ...
12 years, 1 month ago (2012-02-07 16:42:36 UTC) #5
kcc1
In general, this is what we need. - Please make sure that the real_* functions ...
12 years, 1 month ago (2012-02-07 18:01:05 UTC) #6
samsonov
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#newcode100 interception/interception.h:100: bool OverrideFunction(void *old_func, void *new_func, void **orig_old_func); On 2012/02/07 ...
12 years, 1 month ago (2012-02-08 07:35:54 UTC) #7
glider
LGTM with nit. http://codereview.appspot.com/5642046/diff/1012/asan_interceptors.h File asan_interceptors.h (right): http://codereview.appspot.com/5642046/diff/1012/asan_interceptors.h#newcode34 asan_interceptors.h:34: size_t internal_strlen(const char *s); BTW aren't ...
12 years, 1 month ago (2012-02-08 08:19:17 UTC) #8
samsonov
kcc@: 1) Moved auxiliary functions under __interception namespace. 2) Why can't we for simplicity declare ...
12 years, 1 month ago (2012-02-08 15:38:11 UTC) #9
glider
LGTM
12 years, 1 month ago (2012-02-08 15:43:11 UTC) #10
kcc1
Looks good! On Wed, Feb 8, 2012 at 7:38 AM, <samsonov@google.com> wrote: > kcc@: > ...
12 years, 1 month ago (2012-02-08 17:30:39 UTC) #11
samsonov
12 years, 1 month ago (2012-02-08 20:05:40 UTC) #12
r150083

> Looks good!
> 
> On Wed, Feb 8, 2012 at 7:38 AM, <mailto:samsonov@google.com> wrote:
> 
> > kcc@:
> > 1) Moved auxiliary functions under __interception namespace.
> > 2) Why can't we for simplicity declare real_f inside __interception
> > namespace as well?
> 
> ok
> 
> > This may break if two different libraries using
> > interception are linked together, but it's a mess anyway, as they'll
> > have interceptors for the same functions.
> > 3) This CL is large already, so I'd prefer to resolve some FIXMEs after
> > this CL is submitted, if you don't mind.
> >
> ok
> 
> >
> > glider@ suggested we should remove all OS-specific
> > details from the header "interception.h", so I moved them
> > to OS-specific headers. In this way the user code will not include
> > <dlfcn.h> or mach_override.h
> >
> >
> >
> >
>
http://codereview.appspot.com/**5642046/diff/1012/asan_**interceptors.h%3Chtt...>
> > File asan_interceptors.h (right):
> >
> > http://codereview.appspot.com/**5642046/diff/1012/asan_**
> >
>
interceptors.h#newcode34<http://codereview.appspot.com/5642046/diff/1012/asan_interceptors.h#newcode34>
> > asan_interceptors.h:34: size_t internal_strlen(const char *s);
> > On 2012/02/08 08:19:17, glider wrote:
> >
> >> BTW aren't these functions already declared in asan_internal.h?
> >>
> >
> > Not yet.
> >
> >
>
http://codereview.appspot.com/**5642046/%3Chttp://codereview.appspot.com/5642...>
> >
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b