[llvm] r297661 - Bring back r297624.

Thu Mar 30 07:11:10 PDT 2017

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.

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.

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.

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

I can see the point of just making it an error, any idea what is the
best way of implementing that?

If we are going to support it, I think we do have to close it and use
dup.  Just leaving the file open seems too different from the regular
behavior.

Cheers,
Rafael

On 30 March 2017 at 09:13, Yaron Keren <yaron.keren at gmail.com> wrote:
> Hi Rafael,
>
> In previous discussion https://reviews.llvm.org/D13128
>
> "This code has been questioned in this way before, and every time it comes
> up, it turns out that there's a bug somewhere else that really ought to be
> fixed anyway.
>
> In previous iterations of this discussion, when clang is found attempting
to
> print something like a dep file to the same stream as the normal compiler
> output file, this is not actually desirable behavior because it mixes two
> different kinds of output. Besides the fact that this typically isn't
what a
> user actually wants, it can cause serious problems if one of the stream
> needs to use "binary" mode and the other doesn't, for example.
>
> The resolution has been to have clang issue an error if the dep file and
the
> output file are both using stdout, which is nice to do, because if a user
is
> asking for this it's likely not intentional. With this done, there is no
> longer a way to crash the compiler, and there's no need to change how
> raw_fd_ostream works."
>
> If we do want to support this usage case (thus avoid closing
STDOUT_FILENO)
> how about simply avoid closing it (as suggested in D13128) instead of
going
> around, duping STDOUT_FILENO and then closing the dups?
>
> Yaron
>
>
>
>
>
>
> ‫בתאריך יום ב׳, 13 במרץ 2017 ב-22:12 מאת ‪Rafael Espindola via
> llvm-commits‬‏ <‪llvm-commits at lists.llvm.org‬‏>:‬
>
> Author: rafael
> Date: Mon Mar 13 15:00:25 2017
> New Revision: 297661
>
> URL: http://llvm.org/viewvc/llvm-project?rev=297661&view=rev
> Log:
> Bring back r297624.
>
> The issues was just a missing REQUIRES in the test.
>
> Added:
> llvm/trunk/test/Other/writing-to-stdout.ll
> Modified:
> llvm/trunk/lib/Support/raw_ostream.cpp
> llvm/trunk/unittests/Support/raw_ostream_test.cpp
>
> Modified: llvm/trunk/lib/Support/raw_ostream.cpp
> URL:
>
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/raw_ostream.cpp?rev=297661&r1=297660&r2=297661&view=diff
>
==============================================================================
> --- llvm/trunk/lib/Support/raw_ostream.cpp (original)
> +++ llvm/trunk/lib/Support/raw_ostream.cpp Mon Mar 13 15:00:25 2017
> @@ -473,7 +473,7 @@ static int getFD(StringRef Filename, std
> // possible.
> if (!(Flags & sys::fs::F_Text))
> sys::ChangeStdoutToBinary();
> - return STDOUT_FILENO;
> + return dup(STDOUT_FILENO);
> }
>
> int FD;
>
> Added: llvm/trunk/test/Other/writing-to-stdout.ll
> URL:
>
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/writing-to-stdout.ll?rev=297661&view=auto
>
==============================================================================
> --- llvm/trunk/test/Other/writing-to-stdout.ll (added)
> +++ llvm/trunk/test/Other/writing-to-stdout.ll Mon Mar 13 15:00:25 2017
> @@ -0,0 +1,16 @@
> +; REQUIRES: default_triple
> +
> +; Often LLVM tools use "-" to indicate that output should be written to
> stdout
> +; instead of a file. This behaviour is implemented by the raw_fd_ostream
> class.
> +; This test verifies that when doing so multiple times we don't try to
> access a
> +; closed STDOUT_FILENO. The exact options used in this test are
> unimportant, as
> +; long as they write to stdout using raw_fd_ostream.
> +; RUN: llc %s -o=- -pass-remarks-output=- -filetype=asm | FileCheck %s
> +; foobar should appear as a function somewhere in the assembly file.
> +; CHECK: foobar
> +; !Analysis appears at the start of pass-remarks-output.
> +; CHECK: !Analysis
> +
> +define void @foobar() {
> + ret void
> +}
>
> Modified: llvm/trunk/unittests/Support/raw_ostream_test.cpp
> URL:
>
http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/raw_ostream_test.cpp?rev=297661&r1=297660&r2=297661&view=diff
>
==============================================================================
> --- llvm/trunk/unittests/Support/raw_ostream_test.cpp (original)
> +++ llvm/trunk/unittests/Support/raw_ostream_test.cpp Mon Mar 13 15:00:25
> 2017
> @@ -9,6 +9,7 @@
>
> #include "gtest/gtest.h"
> #include "llvm/ADT/SmallString.h"
> +#include "llvm/Support/FileSystem.h"
> #include "llvm/Support/Format.h"
> #include "llvm/Support/raw_ostream.h"
>
> @@ -330,4 +331,11 @@ TEST(raw_ostreamTest, FormattedHexBytes)
> "0007: 68 69 6a 6b 6c |hijkl|",
> format_bytes_with_ascii_str(B.take_front(12), 0, 7, 1));
> }
> +
> +TEST(raw_fd_ostreamTest, multiple_raw_fd_ostream_to_stdout) {
> + std::error_code EC;
> +
> + { raw_fd_ostream("-", EC, sys::fs::OpenFlags::F_None); }
> + { raw_fd_ostream("-", EC, sys::fs::OpenFlags::F_None); }
> +}
> }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170330/cf6b97b9/attachment.html>