Details
Diff Detail
Event Timeline
lib/sanitizer_common/sanitizer_common.h | ||
---|---|---|
283 | Why do you check if they are negative, instead of checking if they are specified (i.e. not kInvalidFd)? | |
286 | It's unclear that program should be absolute path. Either add a comment, or rename variable (e.g. "filename"). | |
764 | Why is fn_ public? | |
767 | Why do you need this? | |
773 | You can add a AutoFileCloser as well (takes fd in a constructor, and close file in dtor if fd is valid). | |
lib/sanitizer_common/sanitizer_linux_libcdep.cc | ||
550 | This function must work on all POSIX platforms, as SymbolizerProcess::StartSymbolizerSubprocess from sanitizer_symbolizer_posix_libcdep.cc is used there. You might need to move this function (and IsProcessRunning below) | |
lib/sanitizer_common/sanitizer_mac.cc | ||
673 | See above: delete this. | |
lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc | ||
142 | Looks like it makes more sense to do const_cast in the StartSubprocess function. | |
146–147 | Shouldn't this be outfd[1]? (outfd[0] will be closed by StartSubprocess). | |
lib/sanitizer_common/sanitizer_win.cc | ||
780 | Please note that this should be implemented based on SymbolizerProcess::StartSymbolizerSubprocess from lib/sanitizer_common/sanitizer_symbolizer_win.cc. |
Thanks for reviewing. PTAL.
lib/sanitizer_common/sanitizer_common.h | ||
---|---|---|
283 | Why not? Valid descriptors are >=0 and I really don't like special values. | |
286 | It shouldn't. It could be relative to current dir which is perfectly fine. Renamed to "filename" though I can't say I like it. | |
767 | Because otherwise it is impossible to use AutoRunner with lambdas. | |
773 | Code size overhead was Kostya's primary concern. I don't think AutoFileCloser is generic enough. Plus I'd hate checking for kInvalidFd in it. | |
lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc | ||
142 | I'd actually like a little advice here. Here's how execve is defined in standard: int execv(const char *path, char *const argv[]); So argv points to mutable strings. I don't actually understand why. Is it a standard oversight? WDYT? I'd be happy to define argv in StartSubprocess as "const char *const argv[];" if you thinks it is ok to cast const off later. |
lib/sanitizer_common/sanitizer_common.h | ||
---|---|---|
283 | But you *are* using "special values", harcoding the assumption that valid file descriptors are positive, and InvalidFd is negative. This is not always so, e.g. on Windows fd_t is void*, so ">=0" check would always pass. It's better to use a named constant kInvalidFd than literal 0. | |
286 | ... but you call FileExists in the implementation? So, e.g. this wouldn't work: StartSubprocess("llvm-symbolizer", argv, ....); | |
767 | Ack. | |
773 | ... but you check for kInvalidFd three times in the implementation, though. Anyway, I don't have a strong opinion here. | |
lib/sanitizer_common/sanitizer_posix_libcdep.cc | ||
324 ↗ | (On Diff #45922) | Minor nit: we tend to catenate these to stdin, stdout etc. |
lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc | ||
142 | Yeah, let's do the latter for now. |
Thanks for working on this.
lib/sanitizer_common/sanitizer_common.h | ||
---|---|---|
284 | Nit: s/would/will/g | |
285 | We should probably add that all fd after STDERR_FILENO get closed. | |
290 | Should this argument (and StartSubprocess' return value) be something else, since, as Alexey said, "on Windows fd_t is void*"? | |
761 | I'd prefer a more descriptive name (RunOnDestruction or something), even if it's longer. It seems it's mostly used in utility functions (at_scope_exit), which also makes a longer name a smaller problem. | |
lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc | ||
142 | Nit: I think we usually have the comment before the parameter. |
LGTM after addressing existing comments. Thanks!
lib/sanitizer_common/sanitizer_common.h | ||
---|---|---|
284 | "will be redirected" | |
290 | process id is not fd_t. But I agree that uptr might be more appropriate. We use it for ReportFile::fd_pid, for example. | |
lib/sanitizer_common/sanitizer_posix_libcdep.cc | ||
324 ↗ | (On Diff #45936) | Please match arg names to function declaration. Also (sorry for not being clear) I meant stdin_fd, stdout_fd, stderr_fd to avoid confusing with predefined values of the same name in headers. |
All done. Thanks a lot for review. Running check-all and submitting.
lib/sanitizer_common/sanitizer_common.h | ||
---|---|---|
290 | Introduced pid_t typedef. |
Why do you check if they are negative, instead of checking if they are specified (i.e. not kInvalidFd)?