Page MenuHomePhabricator

Avoid calling report_fatal_error in the destructor of raw_fd_ostream when saving a module timestamp file
ClosedPublic

Authored by arphaman on May 19 2017, 7:54 AM.

Details

Summary

We should clear the error on the output stream to prevent the call to report_fatal_error from raw_fd_ostreams's destructor.

I couldn't think of a way to test this, do you think it's possible to have a test for this?

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.May 19 2017, 7:54 AM
bruno edited edge metadata.May 22 2017, 2:05 PM

Any idea why we're hitting this issue in the first place? The error that gets cleaned up is reported at some point before? Seems to me that we're going to fail to update the timestamp but continue as nothing happened, I wonder how many other issues this might trigger...

I couldn't think of a way to test this, do you think it's possible to have a test for this?

If you can come up with a testcase it would be awesome, but for this to trigger you'd have to reproduce the concurrent scenario in the testcase, which I don't see how.

Any idea why we're hitting this issue in the first place? The error that gets cleaned up is reported at some point before? Seems to me that we're going to fail to update the timestamp but continue as nothing happened, I wonder how many other issues this might trigger...

I don't know, I only have access to a crashlog without a reproducer.
Correct me if I'm wrong though, but judging by the way the timestamp is used, it seems to me that if we fail to update the timestamp nothing bad will happen, except the build will have to do more work as it will have to validate the system input files in that module for each CU until the first write to the module's timestamp succeeds.
I actually don't need the first change in the if it turns out, because that doesn't trigger a crash. The open file error is cleaned up and ignored, so I don't think that cleaning up an error on closure will break things.

I couldn't think of a way to test this, do you think it's possible to have a test for this?

If you can come up with a testcase it would be awesome, but for this to trigger you'd have to reproduce the concurrent scenario in the testcase, which I don't see how.

It seems that I can't really test a failure on write/close unfortunately.

arphaman updated this revision to Diff 99904.May 23 2017, 6:50 AM

Remove the first redundant error check.

bruno accepted this revision.May 24 2017, 3:56 PM

Makes sense. LGTM!

This revision is now accepted and ready to land.May 24 2017, 3:56 PM
This revision was automatically updated to reflect the committed changes.