<div dir="rtl"><div dir="rtl" class="gmail_msg"><div dir="ltr" class="gmail_msg">Dan's point was that outputing mixed two streams to - is always an error so the new testcase</div><div dir="ltr" class="gmail_msg"><br></div><div dir="ltr" class="gmail_msg">   llc %s -o=- -pass-remarks-output=- -filetype=asm  </div><div dir="ltr" class="gmail_msg"><br></div><div dir="ltr" class="gmail_msg">should fail by design. This restriction was enforced by SafelyCloseFileDescriptor()<span class="inbox-inbox-inbox-inbox-inbox-inbox-Apple-converted-space"> and error_detected(). </span> Adding dup() circumvented the check, as the file descriptors are different now so it's no longer an error to mix streams.</div><div dir="ltr" class="gmail_msg"><br></div><div dir="ltr" class="gmail_msg">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.</div><div dir="ltr" class="gmail_msg"><br></div><div dir="ltr" class="gmail_msg"><span class="inbox-inbox-Apple-converted-space">Achieving both changes, allowing mixed two streams into and </span>not closing STDOUT_FILENO<span class="inbox-inbox-inbox-inbox-Apple-converted-space"> could be done by never </span>closing STDOUT_FILENO<span class="inbox-inbox-inbox-inbox-Apple-converted-space"> as suggested in </span><a href="https://reviews.llvm.org/D13128">https://reviews.llvm.org/D13128</a>. This is simpler and saves duping and closing dups.</div><div dir="ltr" class="gmail_msg"><br></div></div><div style="direction:ltr"><br></div><div class="gmail_quote gmail_msg"><div dir="rtl" class="gmail_msg" style="direction:ltr">‫בתאריך יום ה׳, 30 במרץ 2017 ב-16:24 מאת ‪Rafael Espíndola‬‏ <‪<a href="mailto:rafael.espindola@gmail.com" class="gmail_msg" target="_blank">rafael.espindola@gmail.com</a>‬‏>:‬<br class="gmail_msg"></div></div><div class="gmail_quote gmail_msg"><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="direction:ltr">I can see the point of just making it an error, any idea what is the</div><div style="direction:ltr">best way of implementing that?</div>
<div style="direction:ltr"><br></div><div style="direction:ltr">If we are going to support it, I think we do have to close it and use</div><div style="direction:ltr">dup.  Just leaving the file open seems too different from the regular</div><div style="direction:ltr">behavior.</div>
<div style="direction:ltr"><br></div><div style="direction:ltr">Cheers,</div><div style="direction:ltr">Rafael</div>
<div style="direction:ltr"><br></div>
<div style="direction:ltr"><br></div><div style="direction:ltr">On 30 March 2017 at 09:13, Yaron Keren <<a href="mailto:yaron.keren@gmail.com" class="gmail_msg" target="_blank">yaron.keren@gmail.com</a>> wrote:</div><div style="direction:ltr">> Hi Rafael,</div><div style="direction:ltr">></div><div style="direction:ltr">> In previous discussion <a href="https://reviews.llvm.org/D13128" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D13128</a></div><div style="direction:ltr">></div><div style="direction:ltr">> "This code has been questioned in this way before, and every time it comes</div><div style="direction:ltr">> up, it turns out that there's a bug somewhere else that really ought to be</div><div style="direction:ltr">> fixed anyway.</div><div style="direction:ltr">></div><div style="direction:ltr">> In previous iterations of this discussion, when clang is found attempting to</div><div style="direction:ltr">> print something like a dep file to the same stream as the normal compiler</div><div style="direction:ltr">> output file, this is not actually desirable behavior because it mixes two</div><div style="direction:ltr">> different kinds of output. Besides the fact that this typically isn't what a</div><div style="direction:ltr">> user actually wants, it can cause serious problems if one of the stream</div><div style="direction:ltr">> needs to use "binary" mode and the other doesn't, for example.</div><div style="direction:ltr">></div><div style="direction:ltr">> The resolution has been to have clang issue an error if the dep file and the</div><div style="direction:ltr">> output file are both using stdout, which is nice to do, because if a user is</div><div style="direction:ltr">> asking for this it's likely not intentional. With this done, there is no</div><div style="direction:ltr">> longer a way to crash the compiler, and there's no need to change how</div><div style="direction:ltr">> raw_fd_ostream works."</div><div style="direction:ltr">></div><div style="direction:ltr">> If we do want to support this usage case (thus avoid closing STDOUT_FILENO)</div><div style="direction:ltr">> how about simply avoid closing it (as suggested in D13128) instead of going</div><div style="direction:ltr">> around, duping STDOUT_FILENO and then closing the dups?</div><div style="direction:ltr">></div><div style="direction:ltr">> Yaron</div><div style="direction:ltr">></div><div style="direction:ltr">></div><div style="direction:ltr">></div><div style="direction:ltr">></div><div style="direction:ltr">></div><div style="direction:ltr">></div><div style="direction:ltr">> ‫בתאריך יום ב׳, 13 במרץ 2017 ב-22:12 מאת ‪Rafael Espindola via</div><div style="direction:ltr">> llvm-commits‬‏ <‪<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a>‬‏>:‬</div><div style="direction:ltr">></div><div style="direction:ltr">> Author: rafael</div><div style="direction:ltr">> Date: Mon Mar 13 15:00:25 2017</div><div style="direction:ltr">> New Revision: 297661</div><div style="direction:ltr">></div><div style="direction:ltr">> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=297661&view=rev" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project?rev=297661&view=rev</a></div><div style="direction:ltr">> Log:</div><div style="direction:ltr">> Bring back r297624.</div><div style="direction:ltr">></div><div style="direction:ltr">> The issues was just a missing REQUIRES in the test.</div><div style="direction:ltr">></div><div style="direction:ltr">> Added:</div><div style="direction:ltr">> llvm/trunk/test/Other/writing-to-stdout.ll</div><div style="direction:ltr">> Modified:</div><div style="direction:ltr">> llvm/trunk/lib/Support/raw_ostream.cpp</div><div style="direction:ltr">> llvm/trunk/unittests/Support/raw_ostream_test.cpp</div><div style="direction:ltr">></div><div style="direction:ltr">> Modified: llvm/trunk/lib/Support/raw_ostream.cpp</div><div style="direction:ltr">> URL:</div><div style="direction:ltr">> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/raw_ostream.cpp?rev=297661&r1=297660&r2=297661&view=diff" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/raw_ostream.cpp?rev=297661&r1=297660&r2=297661&view=diff</a></div><div style="direction:ltr">> ==============================================================================</div><div style="direction:ltr">> --- llvm/trunk/lib/Support/raw_ostream.cpp (original)</div><div style="direction:ltr">> +++ llvm/trunk/lib/Support/raw_ostream.cpp Mon Mar 13 15:00:25 2017</div><div style="direction:ltr">> @@ -473,7 +473,7 @@ static int getFD(StringRef Filename, std</div><div style="direction:ltr">> // possible.</div><div style="direction:ltr">> if (!(Flags & sys::fs::F_Text))</div><div style="direction:ltr">> sys::ChangeStdoutToBinary();</div><div style="direction:ltr">> - return STDOUT_FILENO;</div><div style="direction:ltr">> + return dup(STDOUT_FILENO);</div><div style="direction:ltr">> }</div><div style="direction:ltr">></div><div style="direction:ltr">> int FD;</div><div style="direction:ltr">></div><div style="direction:ltr">> Added: llvm/trunk/test/Other/writing-to-stdout.ll</div><div style="direction:ltr">> URL:</div><div style="direction:ltr">> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/writing-to-stdout.ll?rev=297661&view=auto" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/writing-to-stdout.ll?rev=297661&view=auto</a></div><div style="direction:ltr">> ==============================================================================</div><div style="direction:ltr">> --- llvm/trunk/test/Other/writing-to-stdout.ll (added)</div><div style="direction:ltr">> +++ llvm/trunk/test/Other/writing-to-stdout.ll Mon Mar 13 15:00:25 2017</div><div style="direction:ltr">> @@ -0,0 +1,16 @@</div><div style="direction:ltr">> +; REQUIRES: default_triple</div><div style="direction:ltr">> +</div><div style="direction:ltr">> +; Often LLVM tools use "-" to indicate that output should be written to</div><div style="direction:ltr">> stdout</div><div style="direction:ltr">> +; instead of a file. This behaviour is implemented by the raw_fd_ostream</div><div style="direction:ltr">> class.</div><div style="direction:ltr">> +; This test verifies that when doing so multiple times we don't try to</div><div style="direction:ltr">> access a</div><div style="direction:ltr">> +; closed STDOUT_FILENO. The exact options used in this test are</div><div style="direction:ltr">> unimportant, as</div><div style="direction:ltr">> +; long as they write to stdout using raw_fd_ostream.</div><div style="direction:ltr">> +; RUN: llc %s -o=- -pass-remarks-output=- -filetype=asm | FileCheck %s</div><div style="direction:ltr">> +; foobar should appear as a function somewhere in the assembly file.</div><div style="direction:ltr">> +; CHECK: foobar</div><div style="direction:ltr">> +; !Analysis appears at the start of pass-remarks-output.</div><div style="direction:ltr">> +; CHECK: !Analysis</div><div style="direction:ltr">> +</div><div style="direction:ltr">> +define void @foobar() {</div><div style="direction:ltr">> + ret void</div><div style="direction:ltr">> +}</div><div style="direction:ltr">></div><div style="direction:ltr">> Modified: llvm/trunk/unittests/Support/raw_ostream_test.cpp</div><div style="direction:ltr">> URL:</div><div style="direction:ltr">> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/raw_ostream_test.cpp?rev=297661&r1=297660&r2=297661&view=diff" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/raw_ostream_test.cpp?rev=297661&r1=297660&r2=297661&view=diff</a></div><div style="direction:ltr">> ==============================================================================</div><div style="direction:ltr">> --- llvm/trunk/unittests/Support/raw_ostream_test.cpp (original)</div><div style="direction:ltr">> +++ llvm/trunk/unittests/Support/raw_ostream_test.cpp Mon Mar 13 15:00:25</div><div style="direction:ltr">> 2017</div><div style="direction:ltr">> @@ -9,6 +9,7 @@</div><div style="direction:ltr">></div><div style="direction:ltr">> #include "gtest/gtest.h"</div><div style="direction:ltr">> #include "llvm/ADT/SmallString.h"</div><div style="direction:ltr">> +#include "llvm/Support/FileSystem.h"</div><div style="direction:ltr">> #include "llvm/Support/Format.h"</div><div style="direction:ltr">> #include "llvm/Support/raw_ostream.h"</div><div style="direction:ltr">></div><div style="direction:ltr">> @@ -330,4 +331,11 @@ TEST(raw_ostreamTest, FormattedHexBytes)</div><div style="direction:ltr">> "0007: 68 69 6a 6b 6c |hijkl|",</div><div style="direction:ltr">> format_bytes_with_ascii_str(B.take_front(12), 0, 7, 1));</div><div style="direction:ltr">> }</div><div style="direction:ltr">> +</div><div style="direction:ltr">> +TEST(raw_fd_ostreamTest, multiple_raw_fd_ostream_to_stdout) {</div><div style="direction:ltr">> + std::error_code EC;</div><div style="direction:ltr">> +</div><div style="direction:ltr">> + { raw_fd_ostream("-", EC, sys::fs::OpenFlags::F_None); }</div><div style="direction:ltr">> + { raw_fd_ostream("-", EC, sys::fs::OpenFlags::F_None); }</div><div style="direction:ltr">> +}</div><div style="direction:ltr">> }</div><div style="direction:ltr">></div><div style="direction:ltr">></div><div style="direction:ltr">> _______________________________________________</div><div style="direction:ltr">> llvm-commits mailing list</div><div style="direction:ltr">> <a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a></div><div style="direction:ltr">> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a></div>
</blockquote></div></div>