[bugpoint] Revert r318459

[hintonda]

Summary

Revert r318459 which introduced a TempFile scoping bug.

[Meinersbur]

Sorry, this does not fix bug http://llvm.org/PR38390 for me. I still get:

$ bugpoint polly-timeout-grtestutils.ll -verify -opt-command=C:\Users\meinersbur\build\llvm\release\bin\opt.exe
Read input file      : 'polly-timeout-grtestutils.ll'
*** All input ok
Running selected passes on program to test for crash: C:\Users\meinersbur\build\llvm\release\bin\opt.exe: bugpoint-input-18caf6b.bc: error: Could not open input file: permission denied
Exited with error code '1'

*** Debugging optimizer crash!
Checking to see if these passes crash: -verify: C:\Users\meinersbur\build\llvm\release\bin\opt.exe: bugpoint-input-75e7dbd.bc: error: Could not open input file: permission denied
Exited with error code '1'

*** Found crashing pass: -verify
Emitted bitcode to 'bugpoint-passes.bc'

*** You can reproduce the problem with: opt bugpoint-passes.bc -verify
Checking for crash with changing conditionals to always jump to true: call1.i.noexc call1.i.noexc.split invoke.cont invoke.cont6 for.cond lpad lpad1 invoke.cont10 invoke.cont17 call.i.i.noexc... <33 total>: C:\Users\meinersbur\build\llvm\release\bin\opt.exe: bugpoint-input-6f44ec7.bc: error: Could not open input file: permission denied
verify failed!

compared to when using r334629 (74928bc090c2626a3d4c3749a0d025922a25b13b), the commit before ("LTO: Keep file handles open for memory mapped files")

$ bugpoint polly-timeout-grtestutils.ll -verify -opt-command=C:\Users\meinersbur\build\llvm\release\bin\opt.exe
Read input file      : 'polly-timeout-grtestutils.ll'
*** All input ok
Running selected passes on program to test for crash: Success!
Initializing execution environment: Found lli: C:\Users\meinersbur\build\llvm\release\bin\lli.exe
Sorry, I can't automatically select a safe interpreter!

Exiting.

However, the early discard seems an obvious error. Do you happen to have a test case that shows that the code is used at all?

tools/bugpoint/ExecutionDriver.cpp

320 ↗

(On Diff #164581)

Is the error you suspect that ~DiscardTemp of Discard is called here, before any work is done?

For http://llvm.org/PR38390, the file accessed is called bugpoint-input-b43aded.bc (BugDriver::runPasses creates file name with this pattern). The file deleted here would be called bugpoint-test-program-....bc.

[hintonda]

This is a separate bug from the one you reported.

I have an idea on how to fix that one too, but since I don't have windows, I can't attempt a fix and test it. However, I'll add a comment to PR38390 shortly.

tools/bugpoint/ExecutionDriver.cpp

320 ↗

(On Diff #164581)

Yes, if -save-temps is not passed, the file is deleted when Discard goes out of scope.

[Meinersbur]

tools/bugpoint/ExecutionDriver.cpp

439 ↗

(On Diff #164581)

[serious] Here is another call to executeProgram. BitcodeFile can be empty as well (e.g. BugDriver.cpp:218) which will fail if only executeProgramSafely handles creating the bitcode file.

[hintonda]

tools/bugpoint/ExecutionDriver.cpp

439 ↗

(On Diff #164581)

Yes, you're correct. Looks like just rolling back r318459 with a note that you can't use TempFile in this case. Does that sound like the correct fix?

[Meinersbur]

tools/bugpoint/ExecutionDriver.cpp

439 ↗

(On Diff #164581)

Have you considered wrapping executeProgram inside a function that creates (and discards) the bitfile (instead of hoisting that code into executeProgramSafely)?

[hintonda]

tools/bugpoint/ExecutionDriver.cpp

439 ↗

(On Diff #164581)

I could do that, but we should revert r318459 first -- that change introduced a bug.

[hintonda]

• Rollback last change.
• Revert r318459.

[Meinersbur]

tools/bugpoint/ExecutionDriver.cpp

439 ↗

(On Diff #164581)

What purpose does the revert serve? We know how to fix the problem immediately, as shown here.

If you want to revert r318459 to fix the bug, why did you even upload this differential?

[hintonda]

tools/bugpoint/ExecutionDriver.cpp

439 ↗

(On Diff #164581)

r318459 introduced a bug 10 months ago by an author who no longer works on the project.

If it had been noticed earlier, I'd still consult with the author before reverting. In this case, I initially wanted to just fix it, but as you pointed out, there were multiple code paths that would have needed the same fix. So, in retrospect, reverting the commit the caused the problem seems like a more reasonable course.

If I was wrong to submit a diff for this, I'll just revert the change.

[Meinersbur]

Can you add a test case? Since I never contributed any code to bugpoint, I am not comfortable with just accepting it (usually one asks the original author, who, as you mentioned, is not active anymore). But it would obvious if there was a test case that illustrates the problem/ensure the bug does not reappear.

[Meinersbur]

I talked with @hfinkel about what to do in such cases. He's ok with just reverting. So go ahead and commit the revert.

[hintonda]

Okay, sounds good. I'll revert later today.

Btw, you make a good point about the lack of tests. I plan to look into that as well, but will make that a separate patch.

llvm/trunk/tools/bugpoint/ExecutionDriver.cpp

 Expected<std::string> BugDriver::executeProgram(const Module &Program,
                                                 std::string OutputFile,
                                                 std::string BitcodeFile,
                                                 const std::string &SharedObj,
                                                 AbstractInterpreter *AI) const {
   if (!AI)
     AI = Interpreter;
   assert(AI && "Interpreter should have been created already!");
+  bool CreatedBitcode = false;
   if (BitcodeFile.empty()) {
     // Emit the program to a bitcode file...
-    auto File =
-        sys::fs::TempFile::create(OutputPrefix + "-test-program-%%%%%%%.bc");
-    if (!File) {
-      errs() << ToolName
-             << ": Error making unique filename: " << toString(File.takeError())
+    SmallString<128> UniqueFilename;
+    int UniqueFD;
+    std::error_code EC = sys::fs::createUniqueFile(
+        OutputPrefix + "-test-program-%%%%%%%.bc", UniqueFD, UniqueFilename);
+    if (EC) {
+      errs() << ToolName << ": Error making unique filename: " << EC.message()
              << "!\n";
       exit(1);
     }
-    DiscardTemp Discard{*File};
-    BitcodeFile = File->TmpName;
+    BitcodeFile = UniqueFilename.str();
 
-    if (writeProgramToFile(File->FD, Program)) {
+    if (writeProgramToFile(BitcodeFile, UniqueFD, Program)) {
       errs() << ToolName << ": Error emitting bitcode to file '" << BitcodeFile
              << "'!\n";
       exit(1);
     }
+    CreatedBitcode = true;
   }
 
+  // Remove the temporary bitcode file when we are done.
+  std::string BitcodePath(BitcodeFile);
+  FileRemover BitcodeFileRemover(BitcodePath, CreatedBitcode && !SaveTemps);
+
   if (OutputFile.empty())
     OutputFile = OutputPrefix + "-execution-output-%%%%%%%";
 
   // Check to see if this is a valid output filename...
   SmallString<128> UniqueFile;
   std::error_code EC = sys::fs::createUniqueFile(OutputFile, UniqueFile);
   if (EC) {
     errs() << ToolName << ": Error making unique filename: " << EC.message()