Re: [PATCH] D13128: Fix backend crash on multiple close of stdout.
sunfish added a comment. > In any case, that's not how clang deal with usage or even internal unexpected > errors. It asserts, print error messages but does not crash on purpose. > Having clang crash here does not seem like a good solution and will result in > this issue continue to surface again. When clang has bugs, it is common for it to crash unceremoniously. That said, if you know of a reasonable way to assert here before the crash, that'd be great. > The bad thing is that the same problem can be reproduced not only in > frontend, but also in opt tool. The test I wrote (test/Other/empty.ll) also > failed with the same message. This could also be interpreted as opt failing to check for incompatible command-line options. One of the command-line options in this case is a hidden option, so this isn't especially surprising. > And Yaron is right, I need to mix several outputs in stdout. What should I do > in this case? What's the context? It may be worth investigating whether there are other ways to address your need. If you want to use -rewrite-map-file and -info-output-file in a testcase at the same time, see the documentation for '%t' in the TestingGuide for creating multiple temporary output files. http://reviews.llvm.org/D13128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13128: Fix backend crash on multiple close of stdout.
ABataev added a comment. The bad thing is that the same problem can be reproduced not only in frontend, but also in opt tool. The test I wrote (test/Other/empty.ll) also failed with the same message. And Yaron is right, I need to mix several outputs in stdout. What should I do in this case? http://reviews.llvm.org/D13128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13128: Fix backend crash on multiple close of stdout.
yaron.keren added a comment. Hi Dan, it makes sense that output streams should not usually be mixed together, especially if one is binary as you write. This may or may not be a problem depending on what the user really wants. He may want to mix the outputs for whatever purposes or it may usually be a user error. In any case, that's not how clang deal with usage or even internal unexpected errors. It asserts, print error messages but does not crash on purpose. Having clang crash here does not seem like a good solution and will result in this issue continue to surface again. http://reviews.llvm.org/D13128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13128: Fix backend crash on multiple close of stdout.
sunfish added a comment. 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. http://reviews.llvm.org/D13128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13128: Fix backend crash on multiple close of stdout.
ABataev updated the summary for this revision. ABataev updated this revision to Diff 35698. ABataev added a comment. Reworked patch after some discussion http://reviews.llvm.org/D13128 Files: lib/Support/raw_ostream.cpp test/Other/empty.ll Index: lib/Support/raw_ostream.cpp === --- lib/Support/raw_ostream.cpp +++ lib/Support/raw_ostream.cpp @@ -490,8 +490,8 @@ static int getFD(StringRef Filename, std::error_code &EC, sys::fs::OpenFlags Flags) { // Handle "-" as stdout. Note that when we do this, we consider ourself - // the owner of stdout. This means that we can do things like close the - // file descriptor when we're done and set the "binary" flag globally. + // the owner of stdout. This means that we can do things like set the "binary" + // flag globally. if (Filename == "-") { EC = std::error_code(); // If user requested binary then put stdout into binary mode if @@ -523,6 +523,10 @@ return; } + // Do not close standard input/output streams. + if (FD <= STDERR_FILENO) +ShouldClose = false; + // Get the starting position. off_t loc = ::lseek(FD, 0, SEEK_CUR); #ifdef LLVM_ON_WIN32 Index: test/Other/empty.ll === --- test/Other/empty.ll +++ test/Other/empty.ll @@ -0,0 +1,2 @@ +; RUN: /export/bpart/_users/abataev/development/llvm_commit3/build/bin/opt -analyze -time-passes -rewrite-map-file=- -stats -info-output-file=- %s 2>&1 | FileCheck %s +; CHECK: Total Execution Time Index: lib/Support/raw_ostream.cpp === --- lib/Support/raw_ostream.cpp +++ lib/Support/raw_ostream.cpp @@ -490,8 +490,8 @@ static int getFD(StringRef Filename, std::error_code &EC, sys::fs::OpenFlags Flags) { // Handle "-" as stdout. Note that when we do this, we consider ourself - // the owner of stdout. This means that we can do things like close the - // file descriptor when we're done and set the "binary" flag globally. + // the owner of stdout. This means that we can do things like set the "binary" + // flag globally. if (Filename == "-") { EC = std::error_code(); // If user requested binary then put stdout into binary mode if @@ -523,6 +523,10 @@ return; } + // Do not close standard input/output streams. + if (FD <= STDERR_FILENO) +ShouldClose = false; + // Get the starting position. off_t loc = ::lseek(FD, 0, SEEK_CUR); #ifdef LLVM_ON_WIN32 Index: test/Other/empty.ll === --- test/Other/empty.ll +++ test/Other/empty.ll @@ -0,0 +1,2 @@ +; RUN: /export/bpart/_users/abataev/development/llvm_commit3/build/bin/opt -analyze -time-passes -rewrite-map-file=- -stats -info-output-file=- %s 2>&1 | FileCheck %s +; CHECK: Total Execution Time ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13128: Fix backend crash on multiple close of stdout.
yaron.keren added a subscriber: sunfish. yaron.keren added a comment. The original commit http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20100816/106268.html by Dan Ghoman says: "Make raw_fd_ostream consider itself the owner of STDOUT_FILENO when constructed with an output filename of "-". In particular, allow the file descriptor to be closed, and close the file descriptor in the destructor if it hasn't been explicitly closed already, to ensure that any write errors are detected." Closing stdout causes not only this problem in the 2nd close (which may be worked around) but potentially losing text output depending on who gets to close the stream first. Is it normal behaviour for console programs to close their stdout? http://reviews.llvm.org/D13128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13128: Fix backend crash on multiple close of stdout.
ABataev added a comment. In http://reviews.llvm.org/D13128#252630, @yaron.keren wrote: > When stdout goes elsewhere the console, the shell creates the the output file > (pipe) and will close it when clang terminates so so why clang should close > it at all ? it did not open it. > > Practically, we have been running locally > > Error(false), UseAtomicWrites(false) { > if (FD < 0 ) { > ShouldClose = false; > return; > } > if (FD <= STDERR_FILENO) > ShouldClose = false; > > > and passing regression tests on Windows 7 and Linux, maybe this is required > on other usage scenarios or OS. I thought about it. The problem is that few lines above there is a comment (in getFD()): // Handle "-" as stdout. Note that when we do this, we consider ourself // the owner of stdout. This means that we can do things like close the // file descriptor when we're done and set the "binary" flag globally. and all such stream are created with ShouldClose flag explicitly set to `true`. Should we remove all this stuff? http://reviews.llvm.org/D13128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D13128: Fix backend crash on multiple close of stdout.
yaron.keren added a subscriber: yaron.keren. yaron.keren added a comment. When stdout goes elsewhere the console, the shell creates the the output file (pipe) and will close it when clang terminates so so why clang should close it at all ? it did not open it. Practically, we have been running locally Error(false), UseAtomicWrites(false) { if (FD < 0 ) { ShouldClose = false; return; } if (FD <= STDERR_FILENO) ShouldClose = false; and passing regression tests on Windows 7 and Linux, maybe this is required on other usage scenarios or OS. http://reviews.llvm.org/D13128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D13128: Fix backend crash on multiple close of stdout.
ABataev created this revision. ABataev added a reviewer: chandlerc. ABataev added a subscriber: cfe-commits. If stdout is used as an output file for several outputs (like dep file, output file, etc.) , it causes a crash in llvm::raw_fd_ostream::~raw_fd_ostream(), when destructor tries to close file descriptor, which already is closed in another stream. Here is the part of the code where it happens: ``` raw_fd_ostream::~raw_fd_ostream() { if (FD >= 0) { flush(); if (ShouldClose && sys::Process::SafelyCloseFileDescriptor(FD)) error_detected(); } ... if (has_error()) report_fatal_error("IO failure on output stream.", /*GenCrashDiag=*/false); } ``` if stdout is already closed sys::Process::SafelyCloseFileDescriptor() cannot close stdout for the second time and an error flag is set for the stream. It makes the destructor to emit `IO failure on output stream.` fatal error message. Patch closes such streams manually and then clears (possibly set) error flags for the stream. http://reviews.llvm.org/D13128 Files: include/clang/Frontend/CompilerInstance.h lib/Frontend/CompilerInstance.cpp test/Frontend/empty.cpp Index: lib/Frontend/CompilerInstance.cpp === --- lib/Frontend/CompilerInstance.cpp +++ lib/Frontend/CompilerInstance.cpp @@ -531,6 +531,12 @@ void CompilerInstance::clearOutputFiles(bool EraseFiles) { for (OutputFile &OF : OutputFiles) { // Manually close the stream before we rename it. +if (OF.OS && OF.ShouldClose) { + // Close the stream and clear error state for stdout. + auto *Stream = static_cast(OF.OS.get()); + Stream->close(); + Stream->clear_error(); +} OF.OS.reset(); if (!OF.TempFilename.empty()) { @@ -555,6 +561,11 @@ } OutputFiles.clear(); + if (NonSeekStream) { +// Close the stream and clear error state for stdout. +NonSeekStream->close(); +NonSeekStream->clear_error(); + } NonSeekStream.reset(); } @@ -593,7 +604,8 @@ // Add the output file -- but don't try to remove "-", since this means we are // using stdin. addOutputFile(OutputFile((OutputPathName != "-") ? OutputPathName : "", - TempPathName, std::move(OS))); + TempPathName, std::move(OS), + OutputPathName == "-" && !Binary)); return Ret; } Index: include/clang/Frontend/CompilerInstance.h === --- include/clang/Frontend/CompilerInstance.h +++ include/clang/Frontend/CompilerInstance.h @@ -153,14 +153,17 @@ std::string Filename; std::string TempFilename; std::unique_ptr OS; +/// \brief true if the file stream should be closed before to be destroyed. +bool ShouldClose; OutputFile(std::string filename, std::string tempFilename, - std::unique_ptr OS) + std::unique_ptr OS, bool ShouldClose = false) : Filename(std::move(filename)), TempFilename(std::move(tempFilename)), - OS(std::move(OS)) {} + OS(std::move(OS)), ShouldClose(ShouldClose) {} OutputFile(OutputFile &&O) : Filename(std::move(O.Filename)), - TempFilename(std::move(O.TempFilename)), OS(std::move(O.OS)) {} + TempFilename(std::move(O.TempFilename)), OS(std::move(O.OS)), + ShouldClose(O.ShouldClose) {} }; /// If the output doesn't support seeking (terminal, pipe). we switch Index: test/Frontend/empty.cpp === --- test/Frontend/empty.cpp +++ test/Frontend/empty.cpp @@ -0,0 +1,6 @@ +// RUN: %clang_cc1 -verify %s -x c++ -triple x86_64-unknown-linux-gnu -dependency-file - -MT - -emit-llvm -o - | FileCheck %s +// Check that no additional error messages are generated. +int main() { + return; // expected-error {{non-void function 'main' should return a value}} +} +// CHECK: -: Index: lib/Frontend/CompilerInstance.cpp === --- lib/Frontend/CompilerInstance.cpp +++ lib/Frontend/CompilerInstance.cpp @@ -531,6 +531,12 @@ void CompilerInstance::clearOutputFiles(bool EraseFiles) { for (OutputFile &OF : OutputFiles) { // Manually close the stream before we rename it. +if (OF.OS && OF.ShouldClose) { + // Close the stream and clear error state for stdout. + auto *Stream = static_cast(OF.OS.get()); + Stream->close(); + Stream->clear_error(); +} OF.OS.reset(); if (!OF.TempFilename.empty()) { @@ -555,6 +561,11 @@ } OutputFiles.clear(); + if (NonSeekStream) { +// Close the stream and clear error state for stdout. +NonSeekStream->close(); +NonSeekStream->clear_error(); + } NonSeekStream.reset(); } @@ -593,7 +604,8 @@ // Add the output file -- but don't try to remove "-", since this means we are // using stdin. addOutputFile(OutputFile((