Thanks for the great comments. In patch set 3, I fixed everything except the
fwrite bit. According to the fwrite man page, fwrite only writes shorter
chunks
if there is an error (not to be confused with the low-level 'write', which
can
be indeed interrupted by a signal). Also, according to the man page,
fwrite's
return type is size_t (unlike 'write', which is indeed ssize_t).
On 2013/11/13 23:40:51, bnoordhuis wrote:
Left some comments. I really like this patch. Tested it with node.js
master
and it works great (as long as V8 can write to /tmp :-))
https://codereview.chromium.org/70013002/diff/40001/src/log.cc
File src/log.cc (right):
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode289
src/log.cc:289: perf_output_handle_ = OS::FOpen(perf_dump_name.start(),
OS::LogFileOpenMode);
This should check for NULL, shouldn't it? Calling setvbuf() or fclose()
on a
NULL stream will segfault.
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode307
src/log.cc:307: (uint64_t)code->instruction_start(),
static_cast? (Here and in a few other places.)
https://codereview.chromium.org/70013002/diff/40001/src/log.cc#newcode452
src/log.cc:452: ASSERT(static_cast<size_t>(size) == rv);
I don't think that's a safe assumption to make. fwrite() can (and will)
sometimes write fewer bytes than requested, e.g. when the write() system
call
got interrupted by a signal.
Minor nit: you probably meant `ssize_t` one line up.
https://codereview.chromium.org/70013002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
