Page MenuHomePhabricator

[sanitizers] extracted process management functions
ClosedPublic

Authored by aizatsky on Jan 25 2016, 11:58 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

aizatsky updated this revision to Diff 45894.Jan 25 2016, 11:58 AM
aizatsky retitled this revision from to [sanitizers] extracted process management functions.
aizatsky updated this object.
aizatsky added a project: Restricted Project.
aizatsky added a subscriber: llvm-commits.
aizatsky updated this revision to Diff 45912.Jan 25 2016, 1:52 PM

Updated test.

samsonov added inline comments.Jan 25 2016, 2:30 PM
lib/sanitizer_common/sanitizer_common.h
283 ↗(On Diff #45912)

Why do you check if they are negative, instead of checking if they are specified (i.e. not kInvalidFd)?

286 ↗(On Diff #45912)

It's unclear that program should be absolute path. Either add a comment, or rename variable (e.g. "filename").

764 ↗(On Diff #45912)

Why is fn_ public?

767 ↗(On Diff #45912)

Why do you need this?

773 ↗(On Diff #45912)

You can add a AutoFileCloser as well (takes fd in a constructor, and close file in dtor if fd is valid).
FTR there is ScopedHandle in lib/sanitizer_common/sanitizer_symbolizer_win.cc you might want to generalize on.

lib/sanitizer_common/sanitizer_linux_libcdep.cc
550 ↗(On Diff #45912)

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)
to sanitizer_posix_libcdep.cc instead.

lib/sanitizer_common/sanitizer_mac.cc
673 ↗(On Diff #45912)

See above: delete this.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
142 ↗(On Diff #45912)

Looks like it makes more sense to do const_cast in the StartSubprocess function.

146 ↗(On Diff #45912)

Shouldn't this be outfd[1]? (outfd[0] will be closed by StartSubprocess).

lib/sanitizer_common/sanitizer_win.cc
780 ↗(On Diff #45912)

Please note that this should be implemented based on SymbolizerProcess::StartSymbolizerSubprocess from lib/sanitizer_common/sanitizer_symbolizer_win.cc.

aizatsky updated this revision to Diff 45922.Jan 25 2016, 3:29 PM
aizatsky marked 4 inline comments as done.

review

aizatsky marked an inline comment as done.Jan 25 2016, 3:30 PM

Thanks for reviewing. PTAL.

lib/sanitizer_common/sanitizer_common.h
283 ↗(On Diff #45912)

Why not? Valid descriptors are >=0 and I really don't like special values.

286 ↗(On Diff #45912)

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 ↗(On Diff #45912)

Because otherwise it is impossible to use AutoRunner with lambdas.

773 ↗(On Diff #45912)

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 ↗(On Diff #45912)

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.

samsonov added inline comments.Jan 25 2016, 4:47 PM
lib/sanitizer_common/sanitizer_common.h
283 ↗(On Diff #45922)

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 ↗(On Diff #45922)

... but you call FileExists in the implementation? So, e.g. this wouldn't work: StartSubprocess("llvm-symbolizer", argv, ....);

768 ↗(On Diff #45922)

Ack.

774 ↗(On Diff #45922)

... 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 ↗(On Diff #45922)

Yeah, let's do the latter for now.

aizatsky updated this revision to Diff 45936.Jan 25 2016, 5:10 PM
aizatsky marked 2 inline comments as done.

review

All done. PTAL.

lib/sanitizer_common/sanitizer_common.h
283 ↗(On Diff #45922)

I didn't know that. Using kInvalidFd.

286 ↗(On Diff #45922)

Thought about this. Removed FileExists. (I used it to debug things).

774 ↗(On Diff #45922)

Let's see how this one would be used. If we end up using it only for files - I'll create another utility.

filcab added a subscriber: filcab.Jan 26 2016, 5:58 AM

Thanks for working on this.

lib/sanitizer_common/sanitizer_common.h
284 ↗(On Diff #45936)

Nit: s/would/will/g

285 ↗(On Diff #45936)

We should probably add that all fd after STDERR_FILENO get closed.

290 ↗(On Diff #45936)

Should this argument (and StartSubprocess' return value) be something else, since, as Alexey said, "on Windows fd_t is void*"?
Probably either a typedef or just a uptr?

761 ↗(On Diff #45936)

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 ↗(On Diff #45936)

Nit: I think we usually have the comment before the parameter.

samsonov accepted this revision.Jan 26 2016, 10:43 AM
samsonov edited edge metadata.

LGTM after addressing existing comments. Thanks!

lib/sanitizer_common/sanitizer_common.h
284 ↗(On Diff #45936)

"will be redirected"

290 ↗(On Diff #45936)

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.

This revision is now accepted and ready to land.Jan 26 2016, 10:43 AM
aizatsky updated this revision to Diff 46027.Jan 26 2016, 12:08 PM
aizatsky marked 8 inline comments as done.
aizatsky edited edge metadata.

review

All done. Thanks a lot for review. Running check-all and submitting.

lib/sanitizer_common/sanitizer_common.h
290 ↗(On Diff #45936)

Introduced pid_t typedef.

This revision was automatically updated to reflect the committed changes.