llvm dev - Jan 2015 - [LLVMdev] Behaviour of outs()?

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?

--
Mats

> 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?

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>

So, I'm only "fixing the compiler code" (by setting `shouldClose
false`), but in essence the code we have is something like this (much
simplified):

 int main(...)
 {
      printf("Some stuff\n");
      ...
      use_clang_to_compile_opencl();
      run_opencl();
      ..
      printf("Some more stuff\n");
 }

and then `myprog > file.txt`

In this case, clang and llvm do not output anything (as far as I'm
aware), but the "Some stuff" and "Some more stuff" does not
appear in
file.txt. Using strace, my colleague found the problem as `close(1)`
followed by `write(1, "...") = EBADF`.


[The ACTUAL code is a benchmark that is part of our test-code, and the
test is failing]

I'm fairly sure that whether llvm actually uses `outs` or not, it will
be constructed and destroyed based on it's existance. I will have a
look at this in more detail, but the general question still remains:

Should LLVM close `STDOUT_FILENO` in a destructor? I don't think so. If so,
why?

--
Mats

On 23 January 2015 at 18:20, 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?