[llvm] r297661 - Bring back r297624.

[Yaron Keren via llvm-commits]

stdout is special since getFD (pre-commit) always returns the same file
descriptor, STDOUT_FILENO (1) whereas for regular files openFileForWrite
will return new (unique) file descriptor and then there is no problem of
closing one of them.

What will happen when you open the same file with two file descriptors and
write there? probably intermixed output, depending on file exclusivity
flags and buffers? dono. With stdout it works intermixed which is actually
ok for many cases, as long you don't depend on order.

I'll update the patch.




‫בתאריך יום ה׳, 30 במרץ 2017 ב-17:34 מאת ‪Rafael Espíndola‬‏ <‪
rafael.espindola at gmail.com‬‏>:‬

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170330/264c68a1/attachment.html>