This is an archive of the discontinued LLVM Phabricator instance.

[Driver] check for exit code from SIGPIPE
ClosedPublic

Authored by nickdesaulniers on Oct 8 2018, 3:16 PM.

Details

Summary

D53000 adds a special exit code for SIGPIPE (writing to a closed
reader), and rather than print a fatal warning, skips printing the
error. This can be seen commonly from piping into head, tee, or
split.

Fixes PR25349, rdar://problem/14285346, b/77310947.

Diff Detail

Event Timeline

nickdesaulniers created this revision.Oct 8 2018, 3:16 PM
nickdesaulniers edited the summary of this revision. (Show Details)Oct 8 2018, 3:19 PM
nickdesaulniers retitled this revision from [DRIVER] check for exit code from SIGPIPE to [Driver] check for exit code from SIGPIPE.Oct 8 2018, 3:22 PM
jfb added a comment.Oct 8 2018, 3:41 PM

What's the return code of the driver when the pipe is broken that way?

lib/Driver/Driver.cpp
1406

Ditto on magical number in a header.

1406

I think you want to create a new diagnostic here for "broken pipe" before the continue.

nickdesaulniers added inline comments.Oct 8 2018, 3:56 PM
lib/Driver/Driver.cpp
1406

Won't the diagnostic always be printed then? I thought we were trying to silence all related warnings?

jfb added inline comments.Oct 8 2018, 3:58 PM
lib/Driver/Driver.cpp
1406

It is an error that I think we want to report, but we don't want to ask for a bug report. So I think we want to avoid printing the long bug report error message, but we want to say "Broken pipe" which seems to be the standard Unix thing to do.

Not my area of expertise though, maybe someone has a different opinion?

  • prefer EX_IOERR from sysexits.h
nickdesaulniers marked an inline comment as done.Oct 8 2018, 4:15 PM
jfb added a comment.Oct 8 2018, 4:16 PM

Just to be sure: what's the exit code from the driver? If we don't diagnose I'm fine with it... but the exit code still needs to reflect the failure!

  • return error code 74 (EX_IOERR) if observed
jfb accepted this revision.Oct 8 2018, 4:58 PM

Digging through tools/driver/driver.cpp this seems to be the right thing. Maybe leave it open for a bit so others can chime in (if say they have other drivers or whatever?). Thanks for fixing!

This revision is now accepted and ready to land.Oct 8 2018, 4:58 PM
This revision was automatically updated to reflect the committed changes.

@rnk has reverted this due to breaking the windows build (sysexits.h) is not available there.

nickdesaulniers added inline comments.
cfe/trunk/lib/Driver/Driver.cpp
81–85 ↗(On Diff #169456)

@rnk || @majnemer thoughts on making this:

#if LLVM_ON_UNIX
​#include <unistd.h> // getpid
#include <sysexits.h> // EX_IOERR
#else
#define EX_IOERR 74
​#endif

vs also guarding the check I added on L1409 with #if LLVM_ON_UNIX?

nickdesaulniers marked an inline comment as done.Oct 12 2018, 12:39 PM
nickdesaulniers added inline comments.
cfe/trunk/lib/Driver/Driver.cpp
81–85 ↗(On Diff #169456)

posted to: https://reviews.llvm.org/D53210
(let's do code review there).