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

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added a comment.

In D68164#1689164 <https://reviews.llvm.org/D68164#1689164>, @MaskRay wrote:

> In D68164#1689162 <https://reviews.llvm.org/D68164#1689162>, @ychen wrote:
>
> > In D68164#1689155 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

I agree with @ruiu that this test passes by coincidence and we should address it in some way besides D67847 <https://reviews.llvm.org/D67847>.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68164/new/

https://reviews.llvm.org/D68164