[Sanitizer] external symbolizer

[glider1, 2012-08-22 10:51:21]

looks good in general

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc
File llvm-symbolizer.cc (right):

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode1
llvm-symbolizer.cc:1: #ifndef __STDC_LIMIT_MACROS
Please add an LLVM-style header comment.

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode12
llvm-symbolizer.cc:12: // #include "llvm/Support/Debug.h"
Please remove this header if you don't need it.

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode30
llvm-symbolizer.cc:30: UseSymbolTable("use-symbol-table", cl::init(true),
Will there be any tests?

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode44
llvm-symbolizer.cc:44: errs().flush();
I haven't seen anyone calling flush() for errs() previously. Is this needed?

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode53
llvm-symbolizer.cc:53: ModuleInfo(ObjectFile *obj, DIContext *di_ctx)
Please be consistent with spaces between function parameters, their types, "&"
and "*".
You have "type *name", but "type& name" now.

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode105
llvm-symbolizer.cc:105: static ModuleInfo *getOrCreateModuleInfo(const string&
module_name) {
Why is this outside of the anonymous namespace?

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode130
llvm-symbolizer.cc:130: name = name.substr(8); // Skip "__DWARF," prefix.
I suggest to make this "startswith(kDwarfPrefix)" and "strlen(kDwarfPrefix)"

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode147
llvm-symbolizer.cc:147: /*FIXME*/true, DebugInfoSection, DebugAbbrevSection,
Please write a FIXME comment on a separate line and annotate "true" with a
comment denoting its purpose.

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode162
llvm-symbolizer.cc:162: if (function_name == "<invalid>") function_name = "??";
FYI my own rule of thumb is: if a string literal occurs at least twice in the
program then it's time to introduce a string constant.

https://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode181
llvm-symbolizer.cc:181: ToolName = argv[0];
Please be aware that argv[0] can look like "../../relative/./path/tool_name",
not just "tool_name".

[samsonov, 2012-08-22 15:53:12]

Thanks for the review!

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc
File llvm-symbolizer.cc (right):

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode1
llvm-symbolizer.cc:1: #ifndef __STDC_LIMIT_MACROS
On 2012/08/22 10:51:21, glider1 wrote:
> Please add an LLVM-style header comment.

Done.

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode12
llvm-symbolizer.cc:12: // #include "llvm/Support/Debug.h"
On 2012/08/22 10:51:21, glider1 wrote:
> Please remove this header if you don't need it.

Done.

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode30
llvm-symbolizer.cc:30: UseSymbolTable("use-symbol-table", cl::init(true),
On 2012/08/22 10:51:21, glider1 wrote:
> Will there be any tests?
Yeah... though I first have to integrate this binary in the build system...

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode44
llvm-symbolizer.cc:44: errs().flush();
On 2012/08/22 10:51:21, glider1 wrote:
> I haven't seen anyone calling flush() for errs() previously. Is this needed?

Done.

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode53
llvm-symbolizer.cc:53: ModuleInfo(ObjectFile *obj, DIContext *di_ctx)
On 2012/08/22 10:51:21, glider1 wrote:
> Please be consistent with spaces between function parameters, their types, "&"
> and "*".
> You have "type *name", but "type& name" now.

Done.

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode105
llvm-symbolizer.cc:105: static ModuleInfo *getOrCreateModuleInfo(const string&
module_name) {
On 2012/08/22 10:51:21, glider1 wrote:
> Why is this outside of the anonymous namespace?
It is static.

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode130
llvm-symbolizer.cc:130: name = name.substr(8); // Skip "__DWARF," prefix.
On 2012/08/22 10:51:21, glider1 wrote:
> I suggest to make this "startswith(kDwarfPrefix)" and "strlen(kDwarfPrefix)"

Done.

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode147
llvm-symbolizer.cc:147: /*FIXME*/true, DebugInfoSection, DebugAbbrevSection,
On 2012/08/22 10:51:21, glider1 wrote:
> Please write a FIXME comment on a separate line and annotate "true" with a
> comment denoting its purpose.

Done.

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode162
llvm-symbolizer.cc:162: if (function_name == "<invalid>") function_name = "??";
On 2012/08/22 10:51:21, glider1 wrote:
> FYI my own rule of thumb is: if a string literal occurs at least twice in the
> program then it's time to introduce a string constant.

Done.

http://codereview.appspot.com/6465080/diff/1/llvm-symbolizer.cc#newcode181
llvm-symbolizer.cc:181: ToolName = argv[0];
On 2012/08/22 10:51:21, glider1 wrote:
> Please be aware that argv[0] can look like "../../relative/./path/tool_name",
> not just "tool_name".

I think it's fine that we'll be pointed to the actual executable, not the tool
name. Fixed the variable name.

[kcc1, 2012-08-23 10:28:51]

LGTM

http://codereview.appspot.com/6465080/diff/4001/llvm-symbolizer.cc
File llvm-symbolizer.cc (right):

http://codereview.appspot.com/6465080/diff/4001/llvm-symbolizer.cc#newcode53
llvm-symbolizer.cc:53: errs() << ToolInvocationPath << ": error reading file: "
<< ec.message() << ".\n";
80 chars