[llvm-commits] Stub for LLVM-based sanitizer for ASan/TSan tools. (issue 6258065)

[dvyukov at google.com]

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 at google.com> wrote:

> > On Thu, May 31, 2012 at 5:22 PM, <mailto:samsonov at 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/6258065/>
> >>
> >> Affected files:
> >>  M     sanitizer_common/mini_libc.h
> >>  A     sanitizer_common/symbolizer.cc
> >>  A     sanitizer_common/symbolizer.h
> >>
> >>
> >>
> >


> --
> Alexey Samsonov, MSK



http://codereview.appspot.com/6258065/