[Sanitizer] wrapper for external symbolizer program

[dvyukov, 2012-08-21 10:52:55]

Other than that LGTM

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
File sanitizer_common/sanitizer_symbolizer.cc (right):

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:96: CHECK(input_fd_);
Invalid fd value is platform-dependent. On Unices it's -1. 0 is a correct fd.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:104: static const int kMaxBufferSize =
1024;
Please leave a TODO here.
It may be not enough for some debug info. Then, you will end up with trash in
the pipe that will read next time.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:111: return false;
Leave a TODO that we need to respawn the subprocess.

[glider1, 2012-08-21 11:29:19]

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
File sanitizer_common/sanitizer_symbolizer.cc (right):

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:83: // ExternalSymbolizer encapsulates
communication between sanitizer and
"between the tool" maybe?

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:89: //   <function_name>
I suggest to start thinking about future extensions to the protocol right now.
For example, the client may want to receive mangled/demangled function name, or
get all the inlined functions this address belongs to.
It might be helpful to introduce some overhead into the requests and symbolizer
responses that will help to determine the information type being sent and ease
its parsing.

E.g. the request may look like:
FILE_OFFSET
chrome 0x12345

and response:
FUNC_FILE_LINE_COL 1
main()
main.cc:4:2

Or:
FILE_OFFSET FLAG_INLINE
chrome 0x12345

FUNC_FILE_LINE_COL 3
foo()
foo.h:1:1
bar()
bar.h:10:20
main()
main.cc:4:2

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:90: //  
<file_name>:<line_number>:<column_number>
Does DWARF allow line ranges or column ranges for a certain location? Which
line/column will you get?

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:117: // FIXME: shoule we handle cases
when function/filename is "<unknown>"
This is another case where you may need to send some overhead to distinguish
unknown addresses.

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:127: if (read_len == 0 || read_len ==
(uptr)-1) {
Isn't read_len==0 a valid situation when you've exhausted all the data?

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:156: uptr GetListOfModules(LoadedModule
*modules, uptr max_modules);
Please move the prototypes of functions used in more than one module to some
common header.

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:170: if (external_symbolizer_ != 0) {
Please remove the extra space.

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:171:
external_symbolizer_->getFileLineInfo(info->module,
Please remove the space.

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:178: Report("WARNING: Sanitizer tries
to symbolize code, but external "
Dunno if referring to the tool as "Sanitizer" is a good idea. Leaving this up to
you.

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
File sanitizer_common/sanitizer_symbolizer_linux.cc (right):

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer_linux.cc:31: if (pipe(&outfd[0])) {
A single call to pipe() might be insufficient: if the process closes
std{in,out,error} before forking the symbolizer (which surprisingly happens in
the wild), pipe() will reuse the closed descriptor. If you then close
STDOUT_FILENO etc. the communication between the tool and the symbolizer will be
broken.
Please see
http://code.google.com/p/gperftools/source/browse/trunk/src/symbolize.cc#137 for
an example how to deal with this problem.

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer_linux.cc:53: Exit(0);
Perhaps this is Exit(1)

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer_linux.cc:100: info->dlpi_addr);
Please fix the spacing.

[dvyukov, 2012-08-21 11:58:52]

On 2012/08/21 11:29:19, glider1 wrote:
>
https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
> sanitizer_common/sanitizer_symbolizer.cc:89: //   <function_name>
> I suggest to start thinking about future extensions to the protocol right now.
> For example, the client may want to receive mangled/demangled function name,
or
> get all the inlined functions this address belongs to.
> It might be helpful to introduce some overhead into the requests and
symbolizer
> responses that will help to determine the information type being sent and ease
> its parsing.
> 
> E.g. the request may look like:
> FILE_OFFSET
> chrome 0x12345
> 
> and response:
> FUNC_FILE_LINE_COL 1
> main()
> main.cc:4:2
> 
> Or:
> FILE_OFFSET FLAG_INLINE
> chrome 0x12345
> 
> FUNC_FILE_LINE_COL 3
> foo()
> foo.h:1:1
> bar()
> bar.h:10:20
> main()
> main.cc:4:2

+1
I think we do not case about forward/backward-compatibility. But still we may
have different commands like "purge caches" or... and yeah we need "symbolize
global data" and potentially "symbolize tls". Inlining information is a good
point. I think sanitizer need to clearly understand when the symbolizer failed
to obtain func name and/or file:line:pos.
We may employ something HTTP-like:
sanitizer->symbolizer
COMMAND params params\n
COMMAND params params\n
COMMAND params params\n
...
symbolizer->sanitizer
REPLY\n
optional multiline body\n
optional multiline body\n
\n
REPLY\n
optional multiline body\n
optional multiline body\n
\n
....

Own IPC protocol, oh boy.

[samsonov, 2012-08-21 15:15:45]

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
File sanitizer_common/sanitizer_symbolizer.cc (right):

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:83: // ExternalSymbolizer encapsulates
communication between sanitizer and
On 2012/08/21 11:29:19, glider1 wrote:
> "between the tool" maybe?

Done.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:89: //   <function_name>
On 2012/08/21 11:29:19, glider1 wrote:
> I suggest to start thinking about future extensions to the protocol right now.
> For example, the client may want to receive mangled/demangled function name,
or
> get all the inlined functions this address belongs to.
> It might be helpful to introduce some overhead into the requests and
symbolizer
> responses that will help to determine the information type being sent and ease
> its parsing.
> 
> E.g. the request may look like:
> FILE_OFFSET
> chrome 0x12345
> 
> and response:
> FUNC_FILE_LINE_COL 1
> main()
> main.cc:4:2
> 
> Or:
> FILE_OFFSET FLAG_INLINE
> chrome 0x12345
> 
> FUNC_FILE_LINE_COL 3
> foo()
> foo.h:1:1
> bar()
> bar.h:10:20
> main()
> main.cc:4:2

I want the external symbolizer binary to be usable by human as well. I think
we'd be able to get away from headers of different kind and simply use
<module> <offset> [<flags>] where flags can mimic addr2line flags (-C for
demangling, -i for inlining etc), and symbolizer will just return a number of
<function>
<file>:<line>:<column>
(or <function>:<file>:<line>:<column>) pieces, with trailing '\0' to mark the
end of output. That doesn't help with symbolizing data, though...



Also, as we control both sanitizer and symbolizer, we don't _necessary_ have to
decide upon the final version of protocol and may change it in process (at
least, I'd like to do this in a separate change :))

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:90: //  
<file_name>:<line_number>:<column_number>
On 2012/08/21 11:29:19, glider1 wrote:
> Does DWARF allow line ranges or column ranges for a certain location? Which
> line/column will you get?

No, it maps each machine instruction to a single file/line/column tuple. Of
course we _may_ hack debug info to make it contain different stuff, but don't
think this would happen any time soon.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:96: CHECK(input_fd_);
On 2012/08/21 10:52:55, dvyukov wrote:
> Invalid fd value is platform-dependent. On Unices it's -1. 0 is a correct fd.

Done.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:104: static const int kMaxBufferSize =
1024;
On 2012/08/21 10:52:55, dvyukov wrote:
> Please leave a TODO here.
> It may be not enough for some debug info. Then, you will end up with trash in
> the pipe that will read next time.

Done.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:111: return false;
On 2012/08/21 10:52:55, dvyukov wrote:
> Leave a TODO that we need to respawn the subprocess.

Resolved.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:117: // FIXME: shoule we handle cases
when function/filename is "<unknown>"
On 2012/08/21 11:29:19, glider1 wrote:
> This is another case where you may need to send some overhead to distinguish
> unknown addresses.
Nah, I think it's fine to deduce that if name is "??", then the name is unknown,
smth what addr2line does. Resolved this FIXME.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:127: if (read_len == 0 || read_len ==
(uptr)-1) {
On 2012/08/21 11:29:19, glider1 wrote:
> Isn't read_len==0 a valid situation when you've exhausted all the data?
We can't "exhaust" data, as symbolizer doesn't close its stdout. For now we can
make it print trailing '\0' to mark the end of output. Fixed this.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:156: uptr GetListOfModules(LoadedModule
*modules, uptr max_modules);
On 2012/08/21 11:29:19, glider1 wrote:
> Please move the prototypes of functions used in more than one module to some
> common header.

Done.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:170: if (external_symbolizer_ != 0) {
On 2012/08/21 11:29:19, glider1 wrote:
> Please remove the extra space.
which space do you mean?

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:171:
external_symbolizer_->getFileLineInfo(info->module,
On 2012/08/21 11:29:19, glider1 wrote:
> Please remove the space.
which space do you mean?

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:178: Report("WARNING: Sanitizer tries
to symbolize code, but external "
On 2012/08/21 11:29:19, glider1 wrote:
> Dunno if referring to the tool as "Sanitizer" is a good idea. Leaving this up
to
> you.

Done.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
File sanitizer_common/sanitizer_symbolizer_linux.cc (right):

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer_linux.cc:31: if (pipe(&outfd[0])) {
On 2012/08/21 11:29:19, glider1 wrote:
> A single call to pipe() might be insufficient: if the process closes
> std{in,out,error} before forking the symbolizer (which surprisingly happens in
> the wild), pipe() will reuse the closed descriptor. If you then close
> STDOUT_FILENO etc. the communication between the tool and the symbolizer will
be
> broken.
> Please see
> http://code.google.com/p/gperftools/source/browse/trunk/src/symbolize.cc#137
for
> an example how to deal with this problem.

Interesting. Thanks for the link! I've used some of that code here.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer_linux.cc:53: Exit(0);
On 2012/08/21 11:29:19, glider1 wrote:
> Perhaps this is Exit(1)

Done.

http://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer_linux.cc:100: info->dlpi_addr);
On 2012/08/21 11:29:19, glider1 wrote:
> Please fix the spacing.

Done.

[glider1, 2012-08-22 10:30:44]

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
File sanitizer_common/sanitizer_symbolizer.cc (right):

https://codereview.appspot.com/6448179/diff/2001/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:171:
external_symbolizer_->getFileLineInfo(info->module,
On 2012/08/21 15:15:45, samsonov wrote:
> On 2012/08/21 11:29:19, glider1 wrote:
> > Please remove the space.
> which space do you mean?

Ah, sorry. Looks like my browser has hidden the underscore. Never mind.

https://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_s...
File sanitizer_common/sanitizer_symbolizer.cc (right):

https://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer.cc:156: if (buffer[read_len + just_read -
1] == '\0') break;
Perhaps this is not a good idea to use \0 as a delimiter, especially if you're
going to make the output human-readable.
Text processing tools may also choke on this \0.

https://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_s...
File sanitizer_common/sanitizer_symbolizer_linux.cc (right):

https://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer_linux.cc:25: #include <sys/types.h>
Please fix the order of headers.

https://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer_linux.cc:43: if (socketpair(AF_UNIX,
SOCK_STREAM, 0, sock_pair[i]) == -1) {
I guess pipe() was also ok, but socketpair() is fine.

https://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_s...
sanitizer_common/sanitizer_symbolizer_linux.cc:93: 
Please add a comment that we're falling through to the parent process.

[samsonov, 2012-08-22 14:08:22]

http://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_sy...
File sanitizer_common/sanitizer_symbolizer.cc (right):

http://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer.cc:156: if (buffer[read_len + just_read -
1] == '\0') break;
On 2012/08/22 10:30:44, glider1 wrote:
> Perhaps this is not a good idea to use \0 as a delimiter, especially if you're
> going to make the output human-readable.
> Text processing tools may also choke on this \0.

Yeah, on a second though, empty line as an end of output is fine. (probably this
still should be polished in later CLs).

http://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_sy...
File sanitizer_common/sanitizer_symbolizer_linux.cc (right):

http://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer_linux.cc:25: #include <sys/types.h>
On 2012/08/22 10:30:44, glider1 wrote:
> Please fix the order of headers.

Done.

http://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer_linux.cc:43: if (socketpair(AF_UNIX,
SOCK_STREAM, 0, sock_pair[i]) == -1) {
On 2012/08/22 10:30:45, glider1 wrote:
> I guess pipe() was also ok, but socketpair() is fine.

Done.

http://codereview.appspot.com/6448179/diff/1008/sanitizer_common/sanitizer_sy...
sanitizer_common/sanitizer_symbolizer_linux.cc:93: 
On 2012/08/22 10:30:45, glider1 wrote:
> Please add a comment that we're falling through to the parent process.

Done.

[glider1, 2012-08-22 14:11:38]

LGTM

[samsonov, 2012-08-23 07:35:12]

On 2012/08/22 14:11:38, glider1 wrote:
> LGTM

r162437, thanks!