Issue 6258065: Stub for LLVM-based sanitizer for ASan/TSan tools. (Closed)
Patch Set 1 #
Total comments: 12
Patch Set 2 : Stub for LLVM-based sanitizer for ASan/TSan tools. #Patch Set 3 : Stub for LLVM-based sanitizer for ASan/TSan tools. #Patch Set 4 : Stub for LLVM-based sanitizer for ASan/TSan tools. #MessagesTotal messages: 5
http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h File lib/sanitizer_common/symbolizer.h (right): http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:1: //===-- symbolizer.h --------------------------------------------*- C++ -*-===// Let's prefix files in sanitizer_common with some prefixes. http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:12: #ifndef SYMBOLIZER_H I would put the prefix here as well. http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:19: typedef unsigned long uptr; Move this to another file. http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:23: char *module; What are these pointers? Who frees them? What is the lifetime? http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:31: class Symbolizer { Are you going to use pimpl? http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:36: bool SymbolizeCode(uptr address, AddressInfo* info); It must return a set of AddressInfo's with inlining info.
Sign in to reply to this message.
http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h File lib/sanitizer_common/symbolizer.h (right): http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:1: //===-- symbolizer.h --------------------------------------------*- C++ -*-===// On 2012/05/31 12:24:58, dvyukov wrote: > Let's prefix files in sanitizer_common with some prefixes. Isn't directory name enough? What is your suggestion: sanitizer_? san_? http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:12: #ifndef SYMBOLIZER_H On 2012/05/31 12:24:58, dvyukov wrote: > I would put the prefix here as well. ditto. http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:19: typedef unsigned long uptr; On 2012/05/31 12:24:58, dvyukov wrote: > Move this to another file. Done. http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:23: char *module; On 2012/05/31 12:24:58, dvyukov wrote: > What are these pointers? Who frees them? What is the lifetime? On a second thought, I though that Symbolizer should better return a pointer to AddressInfo and transfer ownership. http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:31: class Symbolizer { On 2012/05/31 12:24:58, dvyukov wrote: > Are you going to use pimpl? Hm-m, I hoped to get away with OS-specific files with implementation details (as we do in ASan now). http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.... lib/sanitizer_common/symbolizer.h:36: bool SymbolizeCode(uptr address, AddressInfo* info); On 2012/05/31 12:24:58, dvyukov wrote: > It must return a set of AddressInfo's with inlining info. Can a FIXME be fine for now?
Sign in to reply to this message.
On Thu, May 31, 2012 at 5:22 PM, <samsonov@google.com> wrote: > Reviewers: llvm-commits_cs.uiuc.edu, dvyukov, > > > > http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > common/symbolizer.h<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h> > File lib/sanitizer_common/**symbolizer.h (right): > > http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > common/symbolizer.h#newcode1<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode1> > lib/sanitizer_common/**symbolizer.h:1: //===-- symbolizer.h > ------------------------------**--------------*- C++ -*-===// > On 2012/05/31 12:24:58, dvyukov wrote: > >> Let's prefix files in sanitizer_common with some prefixes. >> > Isn't directory name enough? What is your suggestion: sanitizer_? san_? Object files sometimes clash. sanitizer_ > > > http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > common/symbolizer.h#newcode12<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode12> > lib/sanitizer_common/**symbolizer.h:12: #ifndef SYMBOLIZER_H > On 2012/05/31 12:24:58, dvyukov wrote: > >> I would put the prefix here as well. >> > ditto. > > > http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > common/symbolizer.h#newcode19<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode19> > lib/sanitizer_common/**symbolizer.h:19: typedef unsigned long uptr; > On 2012/05/31 12:24:58, dvyukov wrote: > >> Move this to another file. >> > > Done. > > > http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > common/symbolizer.h#newcode23<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode23> > lib/sanitizer_common/**symbolizer.h:23: char *module; > On 2012/05/31 12:24:58, dvyukov wrote: > >> What are these pointers? Who frees them? What is the lifetime? >> > On a second thought, I though that Symbolizer should better return a > pointer to AddressInfo and transfer ownership. What about strings? > http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > common/symbolizer.h#newcode31<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode31> > lib/sanitizer_common/**symbolizer.h:31: class Symbolizer { > On 2012/05/31 12:24:58, dvyukov wrote: > >> Are you going to use pimpl? >> > Hm-m, I hoped to get away with OS-specific files with implementation > details (as we do in ASan now). The header will have a lot of unnecessary details. Actually, what you need is a single function SymbolizeCode(...). You don't even need Initialize(), it won't do anything useful and won't return a useful return value. > > > http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > common/symbolizer.h#newcode36<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode36> > lib/sanitizer_common/**symbolizer.h:36: bool SymbolizeCode(uptr address, > AddressInfo* info); > On 2012/05/31 12:24:58, dvyukov wrote: > >> It must return a set of AddressInfo's with inlining info. >> > Can a FIXME be fine for now? > Let's include it into the interface, but leave implementation as FIXME. Please review this at http://codereview.appspot.com/**6258065/<http://codereview.appspot.com/6258065/> > > Affected files: > M sanitizer_common/mini_libc.h > A sanitizer_common/symbolizer.cc > A sanitizer_common/symbolizer.h > > >
Sign in to reply to this message.
I've uploaded an updated patch. On Thu, May 31, 2012 at 5:35 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Thu, May 31, 2012 at 5:22 PM, <samsonov@google.com> wrote: > >> Reviewers: llvm-commits_cs.uiuc.edu, dvyukov, >> >> >> >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** >> common/symbolizer.h<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h> >> File lib/sanitizer_common/**symbolizer.h (right): >> >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** >> common/symbolizer.h#newcode1<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode1> >> lib/sanitizer_common/**symbolizer.h:1: //===-- symbolizer.h >> ------------------------------**--------------*- C++ -*-===// >> On 2012/05/31 12:24:58, dvyukov wrote: >> >>> Let's prefix files in sanitizer_common with some prefixes. >>> >> Isn't directory name enough? What is your suggestion: sanitizer_? san_? > > > Object files sometimes clash. > sanitizer_ > Done. >> >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** >> common/symbolizer.h#newcode12<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode12> >> lib/sanitizer_common/**symbolizer.h:12: #ifndef SYMBOLIZER_H >> On 2012/05/31 12:24:58, dvyukov wrote: >> >>> I would put the prefix here as well. >>> >> ditto. >> >> >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** >> common/symbolizer.h#newcode19<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode19> >> lib/sanitizer_common/**symbolizer.h:19: typedef unsigned long uptr; >> On 2012/05/31 12:24:58, dvyukov wrote: >> >>> Move this to another file. >>> >> >> Done. >> >> >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** >> common/symbolizer.h#newcode23<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode23> >> lib/sanitizer_common/**symbolizer.h:23: char *module; >> On 2012/05/31 12:24:58, dvyukov wrote: >> >>> What are these pointers? Who frees them? What is the lifetime? >>> >> On a second thought, I though that Symbolizer should better return a >> pointer to AddressInfo and transfer ownership. > > > What about strings? > Make caller delete them as well? > > > >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** >> common/symbolizer.h#newcode31<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode31> >> lib/sanitizer_common/**symbolizer.h:31: class Symbolizer { >> On 2012/05/31 12:24:58, dvyukov wrote: >> >>> Are you going to use pimpl? >>> >> Hm-m, I hoped to get away with OS-specific files with implementation >> details (as we do in ASan now). > > > The header will have a lot of unnecessary details. > Actually, what you need is a single function SymbolizeCode(...). You don't > even need Initialize(), it won't do anything useful and won't return a > useful return value. > We would have to store symbolizer state in some place. OK, it may be not that good idea to expose it in header. Removed the class. > > >> >> >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** >> common/symbolizer.h#newcode36<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode36> >> lib/sanitizer_common/**symbolizer.h:36: bool SymbolizeCode(uptr address, >> AddressInfo* info); >> On 2012/05/31 12:24:58, dvyukov wrote: >> >>> It must return a set of AddressInfo's with inlining info. >>> >> Can a FIXME be fine for now? >> > > Let's include it into the interface, but leave implementation as FIXME. > Done. > > > Please review this at http://codereview.appspot.com/**6258065/<http://codereview.appspot.com/6258065/> >> >> Affected files: >> M sanitizer_common/mini_libc.h >> A sanitizer_common/symbolizer.cc >> A sanitizer_common/symbolizer.h >> >> >> > -- Alexey Samsonov, MSK
Sign in to reply to this message.
LGTM On 2012/05/31 14:31:58, samsonov wrote: > I've uploaded an updated patch. > > On Thu, May 31, 2012 at 5:35 PM, Dmitry Vyukov <mailto:dvyukov@google.com> wrote: > > > On Thu, May 31, 2012 at 5:22 PM, <mailto:samsonov@google.com> wrote: > > > >> Reviewers: llvm-commits_cs.uiuc.edu, dvyukov, > >> > >> > >> > >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > >> > common/symbolizer.h<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h> > >> File lib/sanitizer_common/**symbolizer.h (right): > >> > >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > >> > common/symbolizer.h#newcode1<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode1> > >> lib/sanitizer_common/**symbolizer.h:1: //===-- symbolizer.h > >> ------------------------------**--------------*- C++ -*-===// > >> On 2012/05/31 12:24:58, dvyukov wrote: > >> > >>> Let's prefix files in sanitizer_common with some prefixes. > >>> > >> Isn't directory name enough? What is your suggestion: sanitizer_? san_? > > > > > > Object files sometimes clash. > > sanitizer_ > > > > Done. > > > >> > >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > >> > common/symbolizer.h#newcode12<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode12> > >> lib/sanitizer_common/**symbolizer.h:12: #ifndef SYMBOLIZER_H > >> On 2012/05/31 12:24:58, dvyukov wrote: > >> > >>> I would put the prefix here as well. > >>> > >> ditto. > >> > >> > >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > >> > common/symbolizer.h#newcode19<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode19> > >> lib/sanitizer_common/**symbolizer.h:19: typedef unsigned long uptr; > >> On 2012/05/31 12:24:58, dvyukov wrote: > >> > >>> Move this to another file. > >>> > >> > >> Done. > >> > >> > >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > >> > common/symbolizer.h#newcode23<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode23> > >> lib/sanitizer_common/**symbolizer.h:23: char *module; > >> On 2012/05/31 12:24:58, dvyukov wrote: > >> > >>> What are these pointers? Who frees them? What is the lifetime? > >>> > >> On a second thought, I though that Symbolizer should better return a > >> pointer to AddressInfo and transfer ownership. > > > > > > What about strings? > > > > Make caller delete them as well? > > > > > > > > > >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > >> > common/symbolizer.h#newcode31<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode31> > >> lib/sanitizer_common/**symbolizer.h:31: class Symbolizer { > >> On 2012/05/31 12:24:58, dvyukov wrote: > >> > >>> Are you going to use pimpl? > >>> > >> Hm-m, I hoped to get away with OS-specific files with implementation > >> details (as we do in ASan now). > > > > > > The header will have a lot of unnecessary details. > > Actually, what you need is a single function SymbolizeCode(...). You don't > > even need Initialize(), it won't do anything useful and won't return a > > useful return value. > > > > We would have to store symbolizer state in some place. OK, it may be not > that good idea to expose it in header. Removed the class. > > > > > > >> > >> > >> http://codereview.appspot.com/**6258065/diff/1/lib/sanitizer_** > >> > common/symbolizer.h#newcode36<http://codereview.appspot.com/6258065/diff/1/lib/sanitizer_common/symbolizer.h#newcode36> > >> lib/sanitizer_common/**symbolizer.h:36: bool SymbolizeCode(uptr address, > >> AddressInfo* info); > >> On 2012/05/31 12:24:58, dvyukov wrote: > >> > >>> It must return a set of AddressInfo's with inlining info. > >>> > >> Can a FIXME be fine for now? > >> > > > > Let's include it into the interface, but leave implementation as FIXME. > > > > Done. > > > > > > > > Please review this at > http://codereview.appspot.com/**6258065/%3Chttp://codereview.appspot.com/6258...> > >> > >> Affected files: > >> M sanitizer_common/mini_libc.h > >> A sanitizer_common/symbolizer.cc > >> A sanitizer_common/symbolizer.h > >> > >> > >> > > > > > -- > Alexey Samsonov, MSK
Sign in to reply to this message.
|
