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.
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.
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
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: 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 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 ,
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 ,
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
