We've had this argument before. IMO LLVM should not be in the business of closing stdout, and no code in lib/ should print to stdout because users may expect output there (-o -). On Fri, Jan 23, 2015 at 10:20 AM, Duncan P. N. Exon Smith < dexonsmith at apple.com> wrote:> > > On 2015-Jan-23, at 09:52, mats petersson <mats at planetcatfish.com> wrote: > > > > I was just fixing a bug that was caused by `stdout` being closed > > before the runtime has done `fflush(stdout)` [or however this is > > implemented in the runtime]. > > > > The cause of this seems to be that `outs()` returns a static object > > created from `raw_fd_stream(STDOUT_FILENO, true)` - the `true` being > > the `shouldClose` parameter. > > > > Surely LLVM is not supposed to close `stdout` as part of its operations? > > Looks like this was added in r111643: > > commit 5d56d9df928c48571980efe8d4205de8ab557b7c > Author: Dan Gohman <gohman at apple.com> > Date: Fri Aug 20 16:44:56 2010 +0000 > > Make outs() close its file when its stream is destructed, so that > pending output errors are detected. > > > git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk at 111643 > 91177308-0d34-0410-b5e6-96231b3b80d8 > > Certainly looks intentional. Comment in the code is: > > // Delete the file descriptor when the program exits, forcing error > // detection. If you don't want this behavior, don't use outs(). > > I don't really have an opinion on whether this is a good idea, but it > does suggest that any use of `outs()` in `lib/` is a bug. > > $ git grep -l 'outs()' -- lib/ > lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp > lib/IR/GCOV.cpp > lib/Support/CommandLine.cpp > lib/Support/FormattedStream.cpp > lib/Support/TargetRegistry.cpp > lib/Support/raw_ostream.cpp > lib/Target/CppBackend/CPPBackend.cpp > > All of these cases seem to be for outputting help text of some sort. But > probably they should be passed a reference to a `raw_ostream`, so that the > caller gets to choose whether or not to use `outs()`. > > Were you hitting one of these? > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150123/f918db20/attachment.html>
Both of those SGTM.> On 2015-Jan-23, at 10:32, Reid Kleckner <rnk at google.com> wrote: > > We've had this argument before. IMO LLVM should not be in the business of closing stdout, and no code in lib/ should print to stdout because users may expect output there (-o -). > > On Fri, Jan 23, 2015 at 10:20 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote: > > > On 2015-Jan-23, at 09:52, mats petersson <mats at planetcatfish.com> wrote: > > > > I was just fixing a bug that was caused by `stdout` being closed > > before the runtime has done `fflush(stdout)` [or however this is > > implemented in the runtime]. > > > > The cause of this seems to be that `outs()` returns a static object > > created from `raw_fd_stream(STDOUT_FILENO, true)` - the `true` being > > the `shouldClose` parameter. > > > > Surely LLVM is not supposed to close `stdout` as part of its operations? > > Looks like this was added in r111643: > > commit 5d56d9df928c48571980efe8d4205de8ab557b7c > Author: Dan Gohman <gohman at apple.com> > Date: Fri Aug 20 16:44:56 2010 +0000 > > Make outs() close its file when its stream is destructed, so that > pending output errors are detected. > > > git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk at 111643 91177308-0d34-0410-b5e6-96231b3b80d8 > > Certainly looks intentional. Comment in the code is: > > // Delete the file descriptor when the program exits, forcing error > // detection. If you don't want this behavior, don't use outs(). > > I don't really have an opinion on whether this is a good idea, but it > does suggest that any use of `outs()` in `lib/` is a bug. > > $ git grep -l 'outs()' -- lib/ > lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp > lib/IR/GCOV.cpp > lib/Support/CommandLine.cpp > lib/Support/FormattedStream.cpp > lib/Support/TargetRegistry.cpp > lib/Support/raw_ostream.cpp > lib/Target/CppBackend/CPPBackend.cpp > > All of these cases seem to be for outputting help text of some sort. But > probably they should be passed a reference to a `raw_ostream`, so that the > caller gets to choose whether or not to use `outs()`. > > Were you hitting one of these? > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >
I too tried replicating this, by running gdb on a unchanged [but quite old] clang (-cc1 and stuff), with a breakpoint on `outs`, and it is never called called. So, I will have to investigate if something in our code is calling llvm::outs(), without actually writing anything. (By the way, at some point in the past, someone [1] had altered this code to have shouldClose to false, I changed it back to true as that is how upstream is, and then found out "the hard way" that this is not a good change - it passed all sorts of local testing and regression tests, only some time later someone discovered that some benchmarks were no longer producing output) [1] the history is murky due to changes in our process&version control system, so can't say who, and it's quite possibly someone that is no longer working here anyway, so can't really ask "why is it this way". -- Mats On 23 January 2015 at 19:12, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:> Both of those SGTM. > >> On 2015-Jan-23, at 10:32, Reid Kleckner <rnk at google.com> wrote: >> >> We've had this argument before. IMO LLVM should not be in the business of closing stdout, and no code in lib/ should print to stdout because users may expect output there (-o -). >> >> On Fri, Jan 23, 2015 at 10:20 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote: >> >> > On 2015-Jan-23, at 09:52, mats petersson <mats at planetcatfish.com> wrote: >> > >> > I was just fixing a bug that was caused by `stdout` being closed >> > before the runtime has done `fflush(stdout)` [or however this is >> > implemented in the runtime]. >> > >> > The cause of this seems to be that `outs()` returns a static object >> > created from `raw_fd_stream(STDOUT_FILENO, true)` - the `true` being >> > the `shouldClose` parameter. >> > >> > Surely LLVM is not supposed to close `stdout` as part of its operations? >> >> Looks like this was added in r111643: >> >> commit 5d56d9df928c48571980efe8d4205de8ab557b7c >> Author: Dan Gohman <gohman at apple.com> >> Date: Fri Aug 20 16:44:56 2010 +0000 >> >> Make outs() close its file when its stream is destructed, so that >> pending output errors are detected. >> >> >> git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk at 111643 91177308-0d34-0410-b5e6-96231b3b80d8 >> >> Certainly looks intentional. Comment in the code is: >> >> // Delete the file descriptor when the program exits, forcing error >> // detection. If you don't want this behavior, don't use outs(). >> >> I don't really have an opinion on whether this is a good idea, but it >> does suggest that any use of `outs()` in `lib/` is a bug. >> >> $ git grep -l 'outs()' -- lib/ >> lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp >> lib/IR/GCOV.cpp >> lib/Support/CommandLine.cpp >> lib/Support/FormattedStream.cpp >> lib/Support/TargetRegistry.cpp >> lib/Support/raw_ostream.cpp >> lib/Target/CppBackend/CPPBackend.cpp >> >> All of these cases seem to be for outputting help text of some sort. But >> probably they should be passed a reference to a `raw_ostream`, so that the >> caller gets to choose whether or not to use `outs()`. >> >> Were you hitting one of these? >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> >
I've fixed (ie, removed) the uses of outs() in lib/IR/GCOV.cpp in r226961. "Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:> Both of those SGTM. > >> On 2015-Jan-23, at 10:32, Reid Kleckner <rnk at google.com> wrote: >> >> We've had this argument before. IMO LLVM should not be in the >> business of closing stdout, and no code in lib/ should print to >> stdout because users may expect output there (-o -). >> >> On Fri, Jan 23, 2015 at 10:20 AM, Duncan P. N. Exon Smith >> <dexonsmith at apple.com> wrote: >> >> > On 2015-Jan-23, at 09:52, mats petersson <mats at planetcatfish.com> wrote: >> > >> > I was just fixing a bug that was caused by `stdout` being closed >> > before the runtime has done `fflush(stdout)` [or however this is >> > implemented in the runtime]. >> > >> > The cause of this seems to be that `outs()` returns a static object >> > created from `raw_fd_stream(STDOUT_FILENO, true)` - the `true` being >> > the `shouldClose` parameter. >> > >> > Surely LLVM is not supposed to close `stdout` as part of its operations? >> >> Looks like this was added in r111643: >> >> commit 5d56d9df928c48571980efe8d4205de8ab557b7c >> Author: Dan Gohman <gohman at apple.com> >> Date: Fri Aug 20 16:44:56 2010 +0000 >> >> Make outs() close its file when its stream is destructed, so that >> pending output errors are detected. >> >> >> git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk at 111643 >> 91177308-0d34-0410-b5e6-96231b3b80d8 >> >> Certainly looks intentional. Comment in the code is: >> >> // Delete the file descriptor when the program exits, forcing error >> // detection. If you don't want this behavior, don't use outs(). >> >> I don't really have an opinion on whether this is a good idea, but it >> does suggest that any use of `outs()` in `lib/` is a bug. >> >> $ git grep -l 'outs()' -- lib/ >> lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp >> lib/IR/GCOV.cpp >> lib/Support/CommandLine.cpp >> lib/Support/FormattedStream.cpp >> lib/Support/TargetRegistry.cpp >> lib/Support/raw_ostream.cpp >> lib/Target/CppBackend/CPPBackend.cpp >> >> All of these cases seem to be for outputting help text of some sort. But >> probably they should be passed a reference to a `raw_ostream`, so that the >> caller gets to choose whether or not to use `outs()`. >> >> Were you hitting one of these? >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev