[Commandline] Clear error on raw_ostream when there is parsing error.

[ychen]

Summary

Otherwise report_fatal_error will be triggered in
raw_fd_ostream::~raw_fd_ostream for llvm::errs since it is a static object.

Exposed by llvm/test/Other/close-stderr.ll where 2>&- is used.

[ruiu]

Can you add a testcase to demonstrate the issue?

[MaskRay]

POSIX has stated that file descriptors 0, 1 or 2 should not be closed. The is stated in many parts, e.g.

Applications should not execute programs with file descriptor 0 not open for reading or with file descriptor 1 or 2 not open for writing, as this might cause the executed program to misbehave. In order not to pass on these file descriptors to an executed program, applications should not just close them but should reopen them on, for example, /dev/null.

The change is justified if you can give an example of a different file descriptor.

[ruiu]

In D68164#1687414, @MaskRay wrote:

POSIX has stated that file descriptors 0, 1 or 2 should not be closed. The is stated in many parts, e.g.

Applications should not execute programs with file descriptor 0 not open for reading or with file descriptor 1 or 2 not open for writing, as this might cause the executed program to misbehave. In order not to pass on these file descriptors to an executed program, applications should not just close them but should reopen them on, for example, /dev/null.

The change is justified if you can give an example of a different file descriptor.

I believe that sentence is talking about stdin/stdout/stderr and forking a new process. As long as you don't create a new process, I think you are free to close 0, 1 and 2.

That said I agree with you that we shouldn't close 0, 1 and 2 on exit. I'm not sure if we are doing this at the moment, but if we do, I think we should stop doing that.

[ychen]

In D68164#1687442, @ruiu wrote:

In D68164#1687414, @MaskRay wrote:

POSIX has stated that file descriptors 0, 1 or 2 should not be closed. The is stated in many parts, e.g.

Applications should not execute programs with file descriptor 0 not open for reading or with file descriptor 1 or 2 not open for writing, as this might cause the executed program to misbehave. In order not to pass on these file descriptors to an executed program, applications should not just close them but should reopen them on, for example, /dev/null.

The change is justified if you can give an example of a different file descriptor.

I believe that sentence is talking about stdin/stdout/stderr and forking a new process. As long as you don't create a new process, I think you are free to close 0, 1 and 2.

That said I agree with you that we shouldn't close 0, 1 and 2 on exit. I'm not sure if we are doing this at the moment, but if we do, I think we should stop doing that.

Comments in close-stderr.ll says "Test that the error handling when writing to stderr fails exits the program cleanly rather than aborting." We shouldn't close it and I believe we're not doing it although users could do it at the commandline if they wish and we should exit cleanly.

[ychen]

In D68164#1687403, @ruiu wrote:

Can you add a testcase to demonstrate the issue?

The exit is called twice and somehow the test exit cleanly. I don't know why but it does not look right. If D67847 is landed without this change, the test would fail because of abort.

➜  llvm-project git:(master) gdb --args /home/ychen2/Bld/llvm-clang-lld-x86_64-scei-ps4-ubuntu-Debug-with-gcc/bin/opt --reject-this-option 2>&-
(gdb) b exit
Breakpoint 1 at 0x1c55ac0
(gdb) r
Starting program: /home/ychen2/Bld/llvm-clang-lld-x86_64-scei-ps4-ubuntu-Debug-with-gcc/bin/opt --reject-this-option
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, __GI_exit (status=1) at exit.c:139
(gdb) bt
#0  __GI_exit (status=1) at exit.c:139
#1  0x0000555559b20f76 in (anonymous namespace)::CommandLineParser::ParseCommandLineOptions (this=0x55555ecb6490, argc=2, argv=0x7fffffffcff0,
    Overview="llvm .bc -> .bc modular optimizer and analysis printer\n", Errs=0x55555ec8d420 <llvm::errs()::S>, LongOptionsUseDoubleDash=false)
    at /home/ychen2/Src/llvm-project/llvm/lib/Support/CommandLine.cpp:1588
#2  0x0000555559b1f36c in llvm::cl::ParseCommandLineOptions (argc=2, argv=0x7fffffffd978, Overview="llvm .bc -> .bc modular optimizer and analysis printer\n", Errs=0x0, EnvVar=0x0,
    LongOptionsUseDoubleDash=false) at /home/ychen2/Src/llvm-project/llvm/lib/Support/CommandLine.cpp:1243
#3  0x00005555571e6652 in main (argc=2, argv=0x7fffffffd978) at /home/ychen2/Src/llvm-project/llvm/tools/opt/opt.cpp:543
(gdb) c
Continuing.

Breakpoint 1, __GI_exit (status=1) at exit.c:139
139     in exit.c
(gdb) bt
#0  __GI_exit (status=1) at exit.c:139
#1  0x0000555559b4030a in llvm::report_fatal_error (Reason=, GenCrashDiag=false) at /home/ychen2/Src/llvm-project/llvm/lib/Support/ErrorHandling.cpp:125
#2  0x0000555559b40107 in llvm::report_fatal_error (Reason="IO failure on output stream: Bad file descriptor", GenCrashDiag=false) at /home/ychen2/Src/llvm-project/llvm/lib/Support/ErrorHandling.cpp:86
#3  0x0000555559ba6636 in llvm::raw_fd_ostream::~raw_fd_ostream (this=0x55555ec8d420 <llvm::errs()::S>, __in_chrg=<optimized out>) at /home/ychen2/Src/llvm-project/llvm/lib/Support/raw_ostream.cpp:633
#4  0x00007ffff687e041 in __run_exit_handlers (status=1, listp=0x7ffff6c26718 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at exit.c:108
#5  0x00007ffff687e13a in __GI_exit (status=<optimized out>) at exit.c:139
#6  0x0000555559b20f76 in (anonymous namespace)::CommandLineParser::ParseCommandLineOptions (this=0x55555ecb6490, argc=2, argv=0x7fffffffcff0,
    Overview="llvm .bc -> .bc modular optimizer and analysis printer\n", Errs=0x55555ec8d420 <llvm::errs()::S>, LongOptionsUseDoubleDash=false)
    at /home/ychen2/Src/llvm-project/llvm/lib/Support/CommandLine.cpp:1588
#7  0x0000555559b1f36c in llvm::cl::ParseCommandLineOptions (argc=2, argv=0x7fffffffd978, Overview="llvm .bc -> .bc modular optimizer and analysis printer\n", Errs=0x0, EnvVar=0x0,
    LongOptionsUseDoubleDash=false) at /home/ychen2/Src/llvm-project/llvm/lib/Support/CommandLine.cpp:1243
#8  0x00005555571e6652 in main (argc=2, argv=0x7fffffffd978) at /home/ychen2/Src/llvm-project/llvm/tools/opt/opt.cpp:543
(gdb) c
Continuing.
[Inferior 1 (process 28650) exited with code 01]

[ruiu]

So, could you add a testcase to demonstrate the issue?

If an error occurs, I believe this piece of code will try to call report_fatal_error and that will fail too for the same reason (because if the code fails, it is likely that we can't write anything to stderr), and the process will exit with exit code 1. So, is the new behavior implemented in this patch distinguishable from the existing behavior? If a command behaves differently with this patch, please add a test to demonstrate it.

[ychen]

In D68164#1689148, @ruiu wrote:

So, could you add a testcase to demonstrate the issue?

If an error occurs, I believe this piece of code will try to call report_fatal_error and that will fail too for the same reason (because if the code fails, it is likely that we can't write anything to stderr), and the process will exit with exit code 1. So, is the new behavior implemented in this patch distinguishable from the existing behavior? If a command behaves differently with this patch, please add a test to demonstrate it.

The new behavior is not distinguishable from the existing behavior from the user's perspective. The existing behavior does exit(1)-> report_fatal_error -> exit(1) and new behavior just exit(1) cleanly.

[ychen]

In D68164#1689152, @ychen wrote:

In D68164#1689148, @ruiu wrote:

So, could you add a testcase to demonstrate the issue?

If an error occurs, I believe this piece of code will try to call report_fatal_error and that will fail too for the same reason (because if the code fails, it is likely that we can't write anything to stderr), and the process will exit with exit code 1. So, is the new behavior implemented in this patch distinguishable from the existing behavior? If a command behaves differently with this patch, please add a test to demonstrate it.

The new behavior is not distinguishable from the existing behavior from the user's perspective. The existing behavior does exit(1)-> report_fatal_error -> exit(1) and new behavior just exit(1) cleanly.

Another perspective is that the report_fatal_error in raw_fd_ostream is to make sure errors are not silently ignored before the destructor is called, which is exactly what this patch is for.

[ruiu]

The result of calling exit() more than once is an undefined behavior, so I agree with you that we should avoid that even if that's currently not observable.

Another way of fixing this issue is handling 0, 1 and 2 as special FDs, as they are actually special. If FD is equal or less than 2, we can avoid calling exit even if reading/writing it results in an error. That's perhaps more general way of fixing this issue than clearing an error condition at this particular place, I guess?

[MaskRay]

The exit is called twice and somehow the test exit cleanly.

The current behavior is undefined. POSIX says "If exit() is called more than once, the behavior is undefined." Changing exit to abort in D67847 will fix the issue. Thanks.

The test ; RUN: sh -c 'opt --reject-this-option 2>&-; echo $?; opt -o /dev/null /dev/null 2>&-; echo $?;' is testing an undefined behavior, but I can see 2>&- is a simple alternative to a full-fledged test that makes write operations fail with some other errno. My feeling is that you can just update close-stderr.ll to (not --crash opt --reject-this-option 2>&-) in D67847 and be prepared to update it if build bots on some platforms have different behaviors.

[ychen]

In D68164#1689155, @MaskRay wrote:

The exit is called twice and somehow the test exit cleanly.

The current behavior is undefined. POSIX says "If exit() is called more than once, the behavior is undefined." Changing exit to abort in D67847 will fix the issue. Thanks.

The test ; RUN: sh -c 'opt --reject-this-option 2>&-; echo $?; opt -o /dev/null /dev/null 2>&-; echo $?;' is testing an undefined behavior, but I can see 2>&- is a simple alternative to a full-fledged test that makes write operations fail with some other errno. My feeling is that you can just update close-stderr.ll to (not --crash opt --reject-this-option 2>&1) in D67847 and be prepared to update it if build bots on some platforms have different behaviors.

sorry, I'm really confused now. Based on the comment in close-stderr.ll, the test is to make sure abort does not happen? Why is it checking the undefined behavior?

[ruiu]

In D68164#1689162, @ychen wrote:

In D68164#1689155, @MaskRay wrote:

The exit is called twice and somehow the test exit cleanly.

The current behavior is undefined. POSIX says "If exit() is called more than once, the behavior is undefined." Changing exit to abort in D67847 will fix the issue. Thanks.

The test ; RUN: sh -c 'opt --reject-this-option 2>&-; echo $?; opt -o /dev/null /dev/null 2>&-; echo $?;' is testing an undefined behavior, but I can see 2>&- is a simple alternative to a full-fledged test that makes write operations fail with some other errno. My feeling is that you can just update close-stderr.ll to (not --crash opt --reject-this-option 2>&1) in D67847 and be prepared to update it if build bots on some platforms have different behaviors.

sorry, I'm really confused now. Based on the comment in close-stderr.ll, the test is to make sure abort does not happen? Why is it checking the undefined behavior?

I think the current implementation is wrong in the sense that it calls exit more than once, but it happens to work just fine on most platforms, so the test is not failing at the moment. But in theory the test could hang instead of existing with 1, for example.

[MaskRay]

In D68164#1689162, @ychen wrote:

In D68164#1689155, @MaskRay wrote:

The exit is called twice and somehow the test exit cleanly.

The current behavior is undefined. POSIX says "If exit() is called more than once, the behavior is undefined." Changing exit to abort in D67847 will fix the issue. Thanks.

The test ; RUN: sh -c 'opt --reject-this-option 2>&-; echo $?; opt -o /dev/null /dev/null 2>&-; echo $?;' is testing an undefined behavior, but I can see 2>&- is a simple alternative to a full-fledged test that makes write operations fail with some other errno. My feeling is that you can just update close-stderr.ll to (not --crash opt --reject-this-option 2>&-) in D67847 and be prepared to update it if build bots on some platforms have different behaviors.

sorry, I'm really confused now. Based on the comment in close-stderr.ll, the test is to make sure abort does not happen? Why is it checking the undefined behavior?

close-stderr.ll was added to test a particular behavior of an undefined behavior. There was some churn to mark it as fail on Windows or under valgrind. This is a signal that it really should not test the particular behavior. As this patch suggests, to maintain that particular behavior, we would have to make the code complex. Adding a virtual function for this tricky case is something I really do not want to happen... We probably should just use not --crash opt --reject-this-option 2>&-.

[ychen]

In D68164#1689164, @MaskRay wrote:

In D68164#1689162, @ychen wrote:

In D68164#1689155, @MaskRay wrote:

The exit is called twice and somehow the test exit cleanly.

The current behavior is undefined. POSIX says "If exit() is called more than once, the behavior is undefined." Changing exit to abort in D67847 will fix the issue. Thanks.

The test ; RUN: sh -c 'opt --reject-this-option 2>&-; echo $?; opt -o /dev/null /dev/null 2>&-; echo $?;' is testing an undefined behavior, but I can see 2>&- is a simple alternative to a full-fledged test that makes write operations fail with some other errno. My feeling is that you can just update close-stderr.ll to (not --crash opt --reject-this-option 2>&-) in D67847 and be prepared to update it if build bots on some platforms have different behaviors.

sorry, I'm really confused now. Based on the comment in close-stderr.ll, the test is to make sure abort does not happen? Why is it checking the undefined behavior?

close-stderr.ll was added to test a particular behavior of an undefined behavior.

By undefined I don't know what kind of consistent behavior could be checked.

There was some churn to mark it as fail on Windows or under valgrind. This is a signal that it really should not test the particular behavior.

I suspend some of this are related to double exit. The rest are related to platform / software specific handling of exit / crash.

As this patch suggests, to maintain that particular behavior, we would have to make the code complex. Adding a virtual function for this tricky case is something I really do not want to happen...

I see what you’re saying but the vtable is already there and error handling is a slow path.

We probably should just use not --crash opt --reject-this-option 2>&-.

This is against user experience and the test could be removed if we just let it crash in this case. I think it totally possible that the user doesn't really want to see anything from stderr and managed to give a wrong option. They could use &> /dev/null too but...

I agree with @ruiu that this test passes by coincidence and we should address it in some way besides D67847.

[ychen]

llvm/test/Other/close-stderr.ll is removed in rG4ad768525844 which makes this patch unnecessary.

llvm/include/llvm/Support/raw_ostream.h

public:
 
   /// This function determines if this stream is displayed and supports colors.
   virtual bool has_colors() const { return is_displayed(); }
 
   // Enable or disable colors. Once disable_colors() is called,
   // changeColor() has no effect until enable_colors() is called.
   virtual void enable_colors(bool /*enable*/) {}
 
+  virtual void clear_error() {}
+
   //===--------------------------------------------------------------------===//
   // Subclass Interface
   //===--------------------------------------------------------------------===//
 
 private:
   /// The is the piece of the class that is implemented by subclasses.  This
   /// writes the \p Size bytes starting at
   /// \p Ptr to the underlying stream.
public:
   /// time when this raw_ostream's destructor is called, report_fatal_error is
   /// called to report the error. Use clear_error() after handling the error to
   /// avoid this behavior.
   ///
   ///   "Errors should never pass silently.
   ///    Unless explicitly silenced."
   ///      - from The Zen of Python, by Tim Peters
   ///
-  void clear_error() { EC = std::error_code(); }
+  void clear_error() override { EC = std::error_code(); }
 };
 
 /// This returns a reference to a raw_ostream for standard output. Use it like:
 /// outs() << "foo" << "bar";
 raw_ostream &outs();
 
 /// This returns a reference to a raw_ostream for standard error. Use it like:
 /// errs() << "foo" << "bar";

llvm/lib/Support/CommandLine.cpp

bool CommandLineParser::ParseCommandLineOptions(int argc,
   LLVM_DEBUG(dbgs() << "Args: ";
              for (int i = 0; i < argc; ++i) dbgs() << argv[i] << ' ';
              dbgs() << '\n';);
 
   // Free all of the memory allocated to the map.  Command line options may only
   // be processed once!
   MoreHelp.clear();
 
-  // If we had an error processing our arguments, don't let the program execute
   if (ErrorParsing) {
-    if (!IgnoreErrors)
+    if (!IgnoreErrors) {
+      Errs->clear_error();
       exit(1);
+    }
     return false;
   }
   return true;
 }
 
 //===----------------------------------------------------------------------===//
 // Option Base class implementation
 //