This is an archive of the discontinued LLVM Phabricator instance.

[bugpoint] Revert r318459
ClosedPublic

Authored by hintonda on Sep 8 2018, 8:11 PM.

Details

Summary

Revert r318459 which introduced a TempFile scoping bug.

Diff Detail

Event Timeline

hintonda created this revision.Sep 8 2018, 8:11 PM

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

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 marked an inline comment as done.Sep 9 2018, 6:39 AM

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

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

Meinersbur added inline comments.Sep 10 2018, 8:56 AM
tools/bugpoint/ExecutionDriver.cpp
439

[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 added inline comments.Sep 10 2018, 10:26 AM
tools/bugpoint/ExecutionDriver.cpp
439

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 added inline comments.Sep 10 2018, 12:26 PM
tools/bugpoint/ExecutionDriver.cpp
439

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

hintonda added inline comments.Sep 10 2018, 1:07 PM
tools/bugpoint/ExecutionDriver.cpp
439

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

hintonda updated this revision to Diff 164743.Sep 10 2018, 1:36 PM
  • Rollback last change.
  • Revert r318459.
hintonda retitled this revision from [bugpoint] Fix TempFile scoping bug to [bugpoint] Revert r318459.Sep 10 2018, 1:37 PM
hintonda edited the summary of this revision. (Show Details)
Meinersbur added inline comments.Sep 10 2018, 1:41 PM
tools/bugpoint/ExecutionDriver.cpp
439

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 added inline comments.Sep 10 2018, 1:50 PM
tools/bugpoint/ExecutionDriver.cpp
439

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.

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 accepted this revision.Sep 14 2018, 11:11 PM
Meinersbur added a subscriber: hfinkel.

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

This revision is now accepted and ready to land.Sep 14 2018, 11:11 PM

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.

This revision was automatically updated to reflect the committed changes.