Skip to content

[llvm][Support] Call clear_error in LockFileManager to avoid report_fatal_error #83655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 2, 2024

Conversation

Bigcheese
Copy link
Contributor

As per the comment in raw_fd_ostream's destructor, you must call clear_error() to prevent a call to report_fatal_error(). There's not really a way to test this, but we did encounter it in the wild.

rdar://117347895

…atal_error

As per the comment in `raw_fd_ostream`'s destructor, you must call
`clear_error()` to prevent a call to `report_fatal_error()`. There's
not really a way to test this, but we did encounter it in the wild.

rdar://117347895
@llvmbot
Copy link
Member

llvmbot commented Mar 2, 2024

@llvm/pr-subscribers-llvm-support

Author: Michael Spencer (Bigcheese)

Changes

As per the comment in raw_fd_ostream's destructor, you must call clear_error() to prevent a call to report_fatal_error(). There's not really a way to test this, but we did encounter it in the wild.

rdar://117347895


Full diff: https://github.com/llvm/llvm-project/pull/83655.diff

1 Files Affected:

  • (modified) llvm/lib/Support/LockFileManager.cpp (+2)
diff --git a/llvm/lib/Support/LockFileManager.cpp b/llvm/lib/Support/LockFileManager.cpp
index 34c7a16b24be41..facdc5a0d7d421 100644
--- a/llvm/lib/Support/LockFileManager.cpp
+++ b/llvm/lib/Support/LockFileManager.cpp
@@ -205,6 +205,8 @@ LockFileManager::LockFileManager(StringRef FileName)
       S.append(std::string(UniqueLockFileName));
       setError(Out.error(), S);
       sys::fs::remove(UniqueLockFileName);
+      // Don't call report_fatal_error.
+      Out.clear_error();
       return;
     }
   }

@Bigcheese Bigcheese merged commit c4f5993 into llvm:main Mar 2, 2024
@dwblaikie
Copy link
Collaborator

Hmm - this was sent for review but committed without review?
If you didn't mean for this to require pre-commit review, please add the tag skip-precommit-approval when creating the PR (per: https://llvm.org/docs/CodeReview.html#code-review-workflow - though I need to fix the link in the note there... )

As for the change itself - if this code was invoked by a unit test, would it fail the unit test due to report_fatal_error? (or would the unit test exit quietly? I'd think it'd fail) - so maybe could be tested that way? Or is it that it's hard to create the error condition that'd lead to the report fatal error in the first place?

@Bigcheese
Copy link
Contributor Author

Yeah, this was just for CI, I'll add the tag in the future.

Getting into a situation that would have an error here is the hard part. I don't actually know how it happened, all I have is a stack trace from the call to abort after report_fatal_error, but not the actual error it failed with. To get here you have to successfully create and open the lock file with the correct permissions, but then fail to write to it. It's possible this happened because of a network failure or some other weird situation.

Bigcheese added a commit to Bigcheese/llvm-project that referenced this pull request Mar 4, 2024
…atal_error (llvm#83655)

As per the comment in `raw_fd_ostream`'s destructor, you must call
`clear_error()` to prevent a call to `report_fatal_error()`. There's not
really a way to test this, but we did encounter it in the wild.

rdar://117347895
(cherry picked from commit c4f5993)
Bigcheese added a commit to swiftlang/llvm-project that referenced this pull request Mar 5, 2024
…atal_error (llvm#83655)

As per the comment in `raw_fd_ostream`'s destructor, you must call
`clear_error()` to prevent a call to `report_fatal_error()`. There's not
really a way to test this, but we did encounter it in the wild.

rdar://117347895
(cherry picked from commit c4f5993)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants