[llvm] r297661 - Bring back r297624.

[Rafael Espíndola via llvm-commits]

On 30 March 2017 at 10:11, Yaron Keren <yaron.keren at gmail.com> wrote:
> Dan's point was that outputing mixed two streams to - is always an error so
> the new testcase
>
>    llc %s -o=- -pass-remarks-output=- -filetype=asm
>
> should fail by design. This restriction was enforced by
> SafelyCloseFileDescriptor() and error_detected().  Adding dup() circumvented
> the check, as the file descriptors are different now so it's no longer an
> error to mix streams.

But it should also be an error if the two file streams refer to the
same regular file, no? Why is stdout a special case?

> In addition, before this commit, STDOUT_FILENO was closed upon exit but now
> a side effect of the dups is that only its dups are closed and STDOUT_FILENO
> is closed by the OS upon process termination.

But that was a crash, not a real user error. Is there a way to detect
if we have two fds that refer to the same underlying file and error on
that?

> Achieving both changes, allowing mixed two streams into and not closing
> STDOUT_FILENO could be done by never closing STDOUT_FILENO as suggested in
> https://reviews.llvm.org/D13128. This is simpler and saves duping and
> closing dups.

I see. That is the point I was missing.  Sorry, yes, the current
change in a convoluted way of just not closing the original file
descriptor. Would you mind posting that patch on top of current trunk
as a cleanup?

The more I think about it the more I feel that we should support
having two streams to the same file. The output is intermixed, but the
complication of always producing a good error is probably not worth
it. Mixing two streams might also be harmless in some cases. For
example, a FileCheck test using CHECK-DAG.

Cheers,
Rafael