Page MenuHomePhabricator

Fix and simplify PrepareCommandsForSourcing
ClosedPublic

Authored by amccarth on Apr 2 2019, 2:12 PM.

Details

Summary

Spotted some problems in the Driver's PrepareCommandsForSourcing while
helping a colleague track another problem.

  1. One error case was not handled because there was no else clause.

Fixed by switching to llvm's early-out style instead of nested
if (success) { } else { } cases. This keeps error handling close
to the actual error.

  1. One call-site failed to call the clean-up function. I solved this

by simplifying the API. PrepareCommandsForSourcing no longer requires
the caller to provide a buffer for the pipe's file descriptors and to
call a separate clean-up function later. PrepareCommandsForSourcing
now ensures the file descriptors are handled before returning.
(The read end of the pipe is held open by the returned FILE * as
before.)

I also eliminated an unnecessary local, shorted the lifetime of another,
and tried to improve the comments.

I wrapped the call to open the pipe to get the #ifdefs out of the
mainline. I replaced the close/_close calls with a platform-neutral
helper from llvm::sys for the same reason. Per discussion on the
review, I'm leaving the fdopen call to use the spelling that Windows
has officially deprecated because it still works it avoids more #ifdefs.

Diff Detail

Repository
rL LLVM

Event Timeline

amccarth created this revision.Apr 2 2019, 2:12 PM
amccarth added reviewers: zturner, labath, rnk.
labath accepted this revision.Apr 3 2019, 12:04 AM

Nice cleanup, thanks. I have one suggestion inline you can implement if you think it's a good idea.

FTR, I believe using pipes here is wrong altogether because it can lead to deadlock. The size of the pipe buffer is implementation-defined, and since there's noone reading from it while we write it, we can block indefinitely if we encounter a particularly long sequence of -o commands. I guess the fact that we haven't hit this in practice means that the implementation-defined size is sufficiently big on all OSs we care about.

lldb/tools/driver/Driver.cpp
492–496 ↗(On Diff #193364)

Do you think it would be possible to use llvm::Process::SafelyCloseFileDescriptor here? It looks like that still uses close (without the _) on windows, which we are trying hard to avoid here, but I'm not sure why. I know close is technically deprecated on windows, but AFAIK the only thing deprecated about it is the name, and otherwise it works correctly.

(If that works, I'd even consider removing this function entirely, as the only thing it does extra is clear the fd, but that does not seem necessary since we're now calling it immediately before we return (and the fds go out of scope anyway)).

This revision is now accepted and ready to land.Apr 3 2019, 12:04 AM
amccarth marked an inline comment as done.Apr 3 2019, 8:49 AM

Thanks for the review.

FTR, I believe using pipes here is wrong altogether because it can lead to deadlock. The size of the pipe buffer is implementation-defined, and since there's noone reading from it while we write it, we can block indefinitely if we encounter a particularly long sequence of -o commands. I guess the fact that we haven't hit this in practice means that the implementation-defined size is sufficiently big on all OSs we care about.

Agreed, pipes probably aren't the right way, but this was just a drive-by cleanup. On Windows, the necessary size is passed to the _pipe call, so if we have an unusually large buffer of commands it should either work or fail gracefully rather than deadlocking.

lldb/tools/driver/Driver.cpp
492–496 ↗(On Diff #193364)

I'm happy to use llvm::sys::Process::SafelyCloseFileDescriptor and delete my little wrapper. I agree that resetting the df to -1 right before it goes out of scope isn't useful. I'll update the patch momentarily.

If we're not that worried about the deprecated names on Windows, should I also just use fdopen and forget about wrapping that as well?

Either way, I'm going to keep the OpenPipe wrapper since the Windows version takes additional parameters.

amccarth updated this revision to Diff 193515.Apr 3 2019, 9:06 AM
amccarth edited the summary of this revision. (Show Details)

Further simplification per labath's feedback.

Specifically:

  1. Use a utility function from llvm to avoid making own wrapper for closing a file descriptor.
  2. Don't bother resetting fd numbers right before they go out of scope.
  3. Live with the Windows-deprecated spelling of fdopen in favor of more #ifdefs.
  4. Updated summary to reflect these changes.
labath accepted this revision.Apr 3 2019, 12:11 PM
labath added inline comments.
lldb/tools/driver/Driver.cpp
492–496 ↗(On Diff #193364)

I think using the non-underscore fdopen is fine, but I am not a windows guy. (The nice thing about SafelyCloseFileDescriptor is that it already provides an abstraction and so we can add the underscore there centrally, should it ever become necessary).

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2019, 12:48 PM
zturner added inline comments.Apr 3 2019, 12:50 PM
lldb/tools/driver/Driver.cpp
492–496 ↗(On Diff #193364)

I dug into this a little bit to find out what the actual difference is, and there isn't a difference at all. close and _close, fdopen and _fdopen, and all other pairs of underscore and non underscore names both resolve to the exact same symbol in the object file (it links in an object file that has a symbol for close that aliases it to _close. So there is literally no difference.

ted added a subscriber: ted.EditedApr 4 2019, 11:37 AM

llvm/tools/lldb/tools/driver/Driver.cpp:495:12: error: comparison of integers of different signs: 'ssize_t' (aka 'long') and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]

if (nrwr != commands_size) {
    ~~~~ ^  ~~~~~~~~~~~~~

1 error generated.