[opt] Fix run-twice crash and detection problem

[Kokan]

Summary

   
1. As reported in the original issue[1], simply execute the `opt -run-twice` with
   any LLVM IR and it is going to crash. For details see the isssue.
2. The `-run-twice` compares the produced output that it saves into two buffer.
   When outputing the result is disabled, that produces two empty string thus
   they are going to be equal all the time resulting false-positive results.

The proposed solution is to generate the results even if the output will not be
emitted, as that is required for the comparision.

[1] https://bugs.llvm.org/show_bug.cgi?id=44382

[xbolva00]

Tests?

[Kokan]

In 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

[MaskRay]

In D71967#1798108, @Kokan wrote:

In 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.

[Kokan]

In D71967#1798176, @MaskRay wrote:

In D71967#1798108, @Kokan wrote:

In 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.

[MaskRay]

I have verified the logic is correct.

[Kokan]

I do not have commit rights to the repository, someone please commit this change.

[MaskRay]

In D71967#1798579, @Kokan wrote:

I do not have commit rights to the repository, someone please commit this change.

@Kokan git commit --amend --author='what-is-your-<email@address>' ?

[Kokan]

In D71967#1798593, @MaskRay wrote:

In D71967#1798579, @Kokan wrote:

I do not have commit rights to the repository, someone please commit this change.

@Kokan git commit --amend --author='what-is-your-<email@address>' ?

It is Peter Kokai <[e-mail]>

llvm/tools/opt/opt.cpp

#endif
   // equivalent if we run the pass manager again, so setup two buffers and
   // a stream to write to them. Note that llc does something similar and it
   // may be worth to abstract this out in the future.
   SmallVector<char, 0> Buffer;
   SmallVector<char, 0> FirstRunBuffer;
   std::unique_ptr<raw_svector_ostream> BOS;
   raw_ostream *OS = nullptr;
 
+  const bool ShouldEmitOutput = !NoOutput && !AnalyzeOnly;
+
   // Write bitcode or assembly to the output as the last step...
-  if (!NoOutput && !AnalyzeOnly) {
+  if (ShouldEmitOutput || RunTwice) {
     assert(Out);
     OS = &Out->os();
     if (RunTwice) {
       BOS = std::make_unique<raw_svector_ostream>(Buffer);
       OS = BOS.get();
     }
     if (OutputAssembly) {
       if (EmitSummaryIndex)
if (!RunTwice) {
     assert(Out);
     if (Buffer.size() != FirstRunBuffer.size() ||
         (memcmp(Buffer.data(), FirstRunBuffer.data(), Buffer.size()) != 0)) {
       errs()
           << "Running the pass manager twice changed the output.\n"
              "Writing the result of the second run to the specified output.\n"
              "To generate the one-run comparison binary, just run without\n"
              "the compile-twice option\n";
+      if (ShouldEmitOutput) {
         Out->os() << BOS->str();
         Out->keep();
+      }
       if (RemarksFile)
         RemarksFile->keep();
       return 1;
     }
+    if (ShouldEmitOutput)
       Out->os() << BOS->str();
   }
 
   if (DebugifyEach && !DebugifyExport.empty())
     exportDebugifyStats(DebugifyExport, Passes.getDebugifyStatsMap());
 
   // Declare success.
   if (!NoOutput || PrintBreakpoints)
     Out->keep();