[PATCH] D71967: [opt] Fix run-twice crash and detection problem

[Kókai Péter via Phabricator via llvm-commits]

Kokan added a comment.

In D71967#1798176 <https://reviews.llvm.org/D71967#1798176>, @MaskRay wrote:

> In D71967#1798108 <https://reviews.llvm.org/D71967#1798108>, @Kokan wrote:
>
> > In D71967#1798089 <https://reviews.llvm.org/D71967#1798089>, @xbolva00 wrote:
> >
> > > Tests?
> >
> >
> > Hello, @xbolva00
> >
> > Thanks for checkout out my patch, and sorry for the lack of test or explanation beforehand.
> >
> > Regarding the tests I seek some support. I was thinking about the following test scenarios, but failed to find any of the proper here:
> >
> > 1. unittests: With proper refactoring of the `opt` that should be doable, although that would be out of the scope of this current fix.
> > 2. test-suit[1]: The issue only happens when the output file is a TTY, a bitcode format is requested and `-run-twice` is used. I could not trigger the failure with llvm-lit test. Please advice.
> > 3. Creating a negative test for `-run-twice`, as every Pass should behave the same way after executing multiple time, there is not test that confirms that `-run-twice` actually could catch different results (just like it does not work now with bitcode format), there could be a dummy Pass created that in fact does reproduce different results with each execution in order to test this.
> >
> >   Could you or anyone please help with it ?
> >
> > [1] I am referring to those tests that are executed via llvm-lit
>
>
> Due to
>
>   if (!Force && !NoOutput && !AnalyzeOnly && !OutputAssembly)
>     if (CheckBitcodeOutputToConsole(Out->os(), !Quiet))
>       NoOutput = true;
>   
>
> This is difficult to test. The bug relies on stdout being a pty.
>
> I think it may be worth defining a variable for `!NoOutput && !AnalyzeOnly`, because the pattern occurs multiple times.


Sure thing, but that does not help much. While checking the issue I spent some time to see the originalish version of opt. The lack of proper abstraction lead to this condition jungle that is very much error prone. IMHO a proper refactor is needed here, depending on my time I plan to do it in a separate patchset.


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

https://reviews.llvm.org/D71967