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

[arphaman]

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?

[bruno]

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.

[arphaman]

In D33357#761400, @bruno wrote:

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]

Remove the first redundant error check.

[bruno]

Makes sense. LGTM!

lib/Serialization/ASTReader.cpp

 static void updateModuleTimestamp(ModuleFile &MF) {
   // Overwrite the timestamp file contents so that file's mtime changes.
   std::string TimestampFilename = MF.getTimestampFilename();
   std::error_code EC;
   llvm::raw_fd_ostream OS(TimestampFilename, EC, llvm::sys::fs::F_Text);
   if (EC)
     return;
   OS << "Timestamp file\n";
+  OS.close();
+  OS.clear_error(); // Avoid triggering a fatal error.
 }
 
 /// \brief Given a cursor at the start of an AST file, scan ahead and drop the
 /// cursor into the start of the given block ID, returning false on success and
 /// true on failure.
 static bool SkipCursorToBlock(BitstreamCursor &Cursor, unsigned BlockID) {
   while (true) {
     llvm::BitstreamEntry Entry = Cursor.advance();