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

[Alexey Samsonov]

I've uploaded an updated patch.

On Thu, May 31, 2012 at 5:35 PM, Dmitry Vyukov <dvyukov at google.com> wrote:

> On Thu, May 31, 2012 at 5:22 PM, <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/<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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120531/f6835718/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: san_symbolizer3.patch
Type: application/octet-stream
Size: 4098 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120531/f6835718/attachment.obj>