[clang] dafb566 - [Support] Return `LockFileManager` errors right away (#130627)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 11 13:49:47 PDT 2025
Author: Jan Svoboda
Date: 2025-03-11T13:49:44-07:00
New Revision: dafb566710cd03b7fbb4b187a91f32be9452fd8c
URL: https://github.com/llvm/llvm-project/commit/dafb566710cd03b7fbb4b187a91f32be9452fd8c
DIFF: https://github.com/llvm/llvm-project/commit/dafb566710cd03b7fbb4b187a91f32be9452fd8c.diff
LOG: [Support] Return `LockFileManager` errors right away (#130627)
This patch removes some internal state out of `LockFileManager` by
moving the locking code from the constructor into new member function
`tryLock()` which returns the errors right away. This simplifies and
modernizes the interface.
Added:
Modified:
clang/lib/Frontend/CompilerInstance.cpp
clang/lib/Serialization/GlobalModuleIndex.cpp
llvm/include/llvm/Support/LockFileManager.h
llvm/lib/Support/LockFileManager.cpp
llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
llvm/unittests/Support/LockFileManagerTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 6098e2e30af9d..e9e96683ca51f 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1483,29 +1483,28 @@ static bool compileModuleAndReadASTBehindLock(
llvm::sys::fs::create_directories(Dir);
while (true) {
- llvm::LockFileManager Locked(ModuleFileName);
- switch (Locked) {
- case llvm::LockFileManager::LFS_Error:
+ llvm::LockFileManager Lock(ModuleFileName);
+ bool Owned;
+ if (llvm::Error Err = Lock.tryLock().moveInto(Owned)) {
// ModuleCache takes care of correctness and locks are only necessary for
// performance. Fallback to building the module in case of any lock
// related errors.
Diags.Report(ModuleNameLoc, diag::remark_module_lock_failure)
- << Module->Name << Locked.getErrorMessage();
+ << Module->Name << toString(std::move(Err));
// Clear out any potential leftover.
- Locked.unsafeRemoveLockFile();
- [[fallthrough]];
- case llvm::LockFileManager::LFS_Owned:
+ Lock.unsafeRemoveLockFile();
+ return compileModuleAndReadASTImpl(ImportingInstance, ImportLoc,
+ ModuleNameLoc, Module, ModuleFileName);
+ }
+ if (Owned) {
// We're responsible for building the module ourselves.
return compileModuleAndReadASTImpl(ImportingInstance, ImportLoc,
ModuleNameLoc, Module, ModuleFileName);
-
- case llvm::LockFileManager::LFS_Shared:
- break; // The interesting case.
}
// Someone else is responsible for building the module. Wait for them to
// finish.
- switch (Locked.waitForUnlock()) {
+ switch (Lock.waitForUnlock()) {
case llvm::LockFileManager::Res_Success:
break; // The interesting case.
case llvm::LockFileManager::Res_OwnerDied:
@@ -1517,7 +1516,7 @@ static bool compileModuleAndReadASTBehindLock(
Diags.Report(ModuleNameLoc, diag::remark_module_lock_timeout)
<< Module->Name;
// Clear the lock file so that future invocations can make progress.
- Locked.unsafeRemoveLockFile();
+ Lock.unsafeRemoveLockFile();
continue;
}
diff --git a/clang/lib/Serialization/GlobalModuleIndex.cpp b/clang/lib/Serialization/GlobalModuleIndex.cpp
index 4b920fccecac3..1e2272c48bd04 100644
--- a/clang/lib/Serialization/GlobalModuleIndex.cpp
+++ b/clang/lib/Serialization/GlobalModuleIndex.cpp
@@ -849,22 +849,21 @@ GlobalModuleIndex::writeIndex(FileManager &FileMgr,
// Coordinate building the global index file with other processes that might
// try to do the same.
- llvm::LockFileManager Locked(IndexPath);
- switch (Locked) {
- case llvm::LockFileManager::LFS_Error:
+ llvm::LockFileManager Lock(IndexPath);
+ bool Owned;
+ if (llvm::Error Err = Lock.tryLock().moveInto(Owned)) {
+ llvm::consumeError(std::move(Err));
return llvm::createStringError(std::errc::io_error, "LFS error");
-
- case llvm::LockFileManager::LFS_Owned:
- // We're responsible for building the index ourselves. Do so below.
- break;
-
- case llvm::LockFileManager::LFS_Shared:
+ }
+ if (!Owned) {
// Someone else is responsible for building the index. We don't care
// when they finish, so we're done.
return llvm::createStringError(std::errc::device_or_resource_busy,
"someone else is building the index");
}
+ // We're responsible for building the index ourselves.
+
// The module index builder.
GlobalModuleIndexBuilder Builder(FileMgr, PCHContainerRdr);
diff --git a/llvm/include/llvm/Support/LockFileManager.h b/llvm/include/llvm/Support/LockFileManager.h
index 4bb8919a0ab31..cf02b41a6f729 100644
--- a/llvm/include/llvm/Support/LockFileManager.h
+++ b/llvm/include/llvm/Support/LockFileManager.h
@@ -9,9 +9,11 @@
#define LLVM_SUPPORT_LOCKFILEMANAGER_H
#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Error.h"
#include <optional>
+#include <string>
#include <system_error>
-#include <utility> // for std::pair
+#include <variant>
namespace llvm {
class StringRef;
@@ -25,19 +27,6 @@ class StringRef;
/// finished the operation.
class LockFileManager {
public:
- /// Describes the state of a lock file.
- enum LockFileState {
- /// The lock file has been created and is owned by this instance
- /// of the object.
- LFS_Owned,
- /// The lock file already exists and is owned by some other
- /// instance.
- LFS_Shared,
- /// An error occurred while trying to create or find the lock
- /// file.
- LFS_Error
- };
-
/// Describes the result of waiting for the owner to release the lock.
enum WaitForUnlockResult {
/// The lock was released successfully.
@@ -53,27 +42,32 @@ class LockFileManager {
SmallString<128> LockFileName;
SmallString<128> UniqueLockFileName;
- std::optional<std::pair<std::string, int>> Owner;
- std::error_code ErrorCode;
- std::string ErrorDiagMsg;
+ struct OwnerUnknown {};
+ struct OwnedByUs {};
+ struct OwnedByAnother {
+ std::string OwnerHostName;
+ int OwnerPID;
+ };
+ std::variant<OwnerUnknown, OwnedByUs, OwnedByAnother> Owner;
LockFileManager(const LockFileManager &) = delete;
LockFileManager &operator=(const LockFileManager &) = delete;
- static std::optional<std::pair<std::string, int>>
- readLockFile(StringRef LockFileName);
+ static std::optional<OwnedByAnother> readLockFile(StringRef LockFileName);
static bool processStillExecuting(StringRef Hostname, int PID);
public:
-
+ /// Does not try to acquire the lock.
LockFileManager(StringRef FileName);
- ~LockFileManager();
- /// Determine the state of the lock file.
- LockFileState getState() const;
+ /// Unlocks the lock if previously acquired by \c tryLock().
+ ~LockFileManager();
- operator LockFileState() const { return getState(); }
+ /// Tries to acquire the lock without blocking.
+ /// \returns true if the lock was successfully acquired, false if the lock is
+ /// already held by someone else, or \c Error in case of unexpected failure.
+ Expected<bool> tryLock();
/// For a shared lock, wait until the owner releases the lock.
/// Total timeout for the file to appear is ~1.5 minutes.
@@ -83,15 +77,6 @@ class LockFileManager {
/// Remove the lock file. This may delete a
diff erent lock file than
/// the one previously read if there is a race.
std::error_code unsafeRemoveLockFile();
-
- /// Get error message, or "" if there is no error.
- std::string getErrorMessage() const;
-
- /// Set error and error message
- void setError(const std::error_code &EC, StringRef ErrorMsg = "") {
- ErrorCode = EC;
- ErrorDiagMsg = ErrorMsg.str();
- }
};
} // end namespace llvm
diff --git a/llvm/lib/Support/LockFileManager.cpp b/llvm/lib/Support/LockFileManager.cpp
index ac26646525666..7cf9db379974f 100644
--- a/llvm/lib/Support/LockFileManager.cpp
+++ b/llvm/lib/Support/LockFileManager.cpp
@@ -51,7 +51,7 @@ using namespace llvm;
/// \param LockFileName The name of the lock file to read.
///
/// \returns The process ID of the process that owns this lock file
-std::optional<std::pair<std::string, int>>
+std::optional<LockFileManager::OwnedByAnother>
LockFileManager::readLockFile(StringRef LockFileName) {
// Read the owning host and PID out of the lock file. If it appears that the
// owning process is dead, the lock file is invalid.
@@ -69,8 +69,10 @@ LockFileManager::readLockFile(StringRef LockFileName) {
PIDStr = PIDStr.substr(PIDStr.find_first_not_of(' '));
int PID;
if (!PIDStr.getAsInteger(10, PID)) {
- auto Owner = std::make_pair(std::string(Hostname), PID);
- if (processStillExecuting(Owner.first, Owner.second))
+ OwnedByAnother Owner;
+ Owner.OwnerHostName = Hostname;
+ Owner.OwnerPID = PID;
+ if (processStillExecuting(Owner.OwnerHostName, Owner.OwnerPID))
return Owner;
}
@@ -158,41 +160,40 @@ class RemoveUniqueLockFileOnSignal {
} // end anonymous namespace
LockFileManager::LockFileManager(StringRef FileName)
-{
- this->FileName = FileName;
- if (std::error_code EC = sys::fs::make_absolute(this->FileName)) {
- std::string S("failed to obtain absolute path for ");
- S.append(std::string(this->FileName));
- setError(EC, S);
- return;
- }
- LockFileName = this->FileName;
+ : FileName(FileName), Owner(OwnerUnknown{}) {}
+
+Expected<bool> LockFileManager::tryLock() {
+ assert(std::holds_alternative<OwnerUnknown>(Owner) &&
+ "lock has already been attempted");
+
+ SmallString<128> AbsoluteFileName(FileName);
+ if (std::error_code EC = sys::fs::make_absolute(AbsoluteFileName))
+ return createStringError(EC, "failed to obtain absolute path for " +
+ AbsoluteFileName);
+ LockFileName = AbsoluteFileName;
LockFileName += ".lock";
// If the lock file already exists, don't bother to try to create our own
// lock file; it won't work anyway. Just figure out who owns this lock file.
- if ((Owner = readLockFile(LockFileName)))
- return;
+ if (auto LockFileOwner = readLockFile(LockFileName)) {
+ Owner = std::move(*LockFileOwner);
+ return false;
+ }
// Create a lock file that is unique to this instance.
UniqueLockFileName = LockFileName;
UniqueLockFileName += "-%%%%%%%%";
int UniqueLockFileID;
if (std::error_code EC = sys::fs::createUniqueFile(
- UniqueLockFileName, UniqueLockFileID, UniqueLockFileName)) {
- std::string S("failed to create unique file ");
- S.append(std::string(UniqueLockFileName));
- setError(EC, S);
- return;
- }
+ UniqueLockFileName, UniqueLockFileID, UniqueLockFileName))
+ return createStringError(EC, "failed to create unique file " +
+ UniqueLockFileName);
// Write our process ID to our unique lock file.
{
SmallString<256> HostID;
- if (auto EC = getHostID(HostID)) {
- setError(EC, "failed to get host id");
- return;
- }
+ if (auto EC = getHostID(HostID))
+ return createStringError(EC, "failed to get host id");
raw_fd_ostream Out(UniqueLockFileID, /*shouldClose=*/true);
Out << HostID << ' ' << sys::Process::getProcessId();
@@ -201,13 +202,12 @@ LockFileManager::LockFileManager(StringRef FileName)
if (Out.has_error()) {
// We failed to write out PID, so report the error, remove the
// unique lock file, and fail.
- std::string S("failed to write to ");
- S.append(std::string(UniqueLockFileName));
- setError(Out.error(), S);
+ Error Err = createStringError(Out.error(),
+ "failed to write to " + UniqueLockFileName);
sys::fs::remove(UniqueLockFileName);
// Don't call report_fatal_error.
Out.clear_error();
- return;
+ return std::move(Err);
}
}
@@ -221,23 +221,21 @@ LockFileManager::LockFileManager(StringRef FileName)
sys::fs::create_link(UniqueLockFileName, LockFileName);
if (!EC) {
RemoveUniqueFile.lockAcquired();
- return;
+ Owner = OwnedByUs{};
+ return true;
}
- if (EC != errc::file_exists) {
- std::string S("failed to create link ");
- raw_string_ostream OSS(S);
- OSS << LockFileName.str() << " to " << UniqueLockFileName.str();
- setError(EC, S);
- return;
- }
+ if (EC != errc::file_exists)
+ return createStringError(EC, "failed to create link " + LockFileName +
+ " to " + UniqueLockFileName);
// Someone else managed to create the lock file first. Read the process ID
// from the lock file.
- if ((Owner = readLockFile(LockFileName))) {
+ if (auto LockFileOwner = readLockFile(LockFileName)) {
// Wipe out our unique lock file (it's useless now)
sys::fs::remove(UniqueLockFileName);
- return;
+ Owner = std::move(*LockFileOwner);
+ return false;
}
if (!sys::fs::exists(LockFileName)) {
@@ -248,39 +246,14 @@ LockFileManager::LockFileManager(StringRef FileName)
// There is a lock file that nobody owns; try to clean it up and get
// ownership.
- if ((EC = sys::fs::remove(LockFileName))) {
- std::string S("failed to remove lockfile ");
- S.append(std::string(UniqueLockFileName));
- setError(EC, S);
- return;
- }
- }
-}
-
-LockFileManager::LockFileState LockFileManager::getState() const {
- if (Owner)
- return LFS_Shared;
-
- if (ErrorCode)
- return LFS_Error;
-
- return LFS_Owned;
-}
-
-std::string LockFileManager::getErrorMessage() const {
- if (ErrorCode) {
- std::string Str(ErrorDiagMsg);
- std::string ErrCodeMsg = ErrorCode.message();
- raw_string_ostream OSS(Str);
- if (!ErrCodeMsg.empty())
- OSS << ": " << ErrCodeMsg;
- return Str;
+ if ((EC = sys::fs::remove(LockFileName)))
+ return createStringError(EC, "failed to remove lockfile " +
+ UniqueLockFileName);
}
- return "";
}
LockFileManager::~LockFileManager() {
- if (getState() != LFS_Owned)
+ if (!std::holds_alternative<OwnedByUs>(Owner))
return;
// Since we own the lock, remove the lock file and our own unique lock file.
@@ -293,8 +266,9 @@ LockFileManager::~LockFileManager() {
LockFileManager::WaitForUnlockResult
LockFileManager::waitForUnlock(const unsigned MaxSeconds) {
- if (getState() != LFS_Shared)
- return Res_Success;
+ auto *LockFileOwner = std::get_if<OwnedByAnother>(&Owner);
+ assert(LockFileOwner &&
+ "waiting for lock to be unlocked without knowing the owner");
// Since we don't yet have an event-based method to wait for the lock file,
// use randomized exponential backoff, similar to Ethernet collision
@@ -311,7 +285,8 @@ LockFileManager::waitForUnlock(const unsigned MaxSeconds) {
return Res_Success;
// If the process owning the lock died without cleaning up, just bail out.
- if (!processStillExecuting((*Owner).first, (*Owner).second))
+ if (!processStillExecuting(LockFileOwner->OwnerHostName,
+ LockFileOwner->OwnerPID))
return Res_OwnerDied;
}
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
index 65aee2f70104b..65cbb13628301 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
@@ -1545,18 +1545,16 @@ PreservedAnalyses AMDGPUSplitModulePass::run(Module &M,
<< "'\n");
while (true) {
- llvm::LockFileManager Locked(LockFilePath.str());
- switch (Locked) {
- case LockFileManager::LFS_Error:
+ llvm::LockFileManager Lock(LockFilePath.str());
+ bool Owned;
+ if (Error Err = Lock.tryLock().moveInto(Owned)) {
+ consumeError(std::move(Err));
LLVM_DEBUG(
dbgs() << "[amdgpu-split-module] unable to acquire lockfile, debug "
"output may be mangled by other processes\n");
- Locked.unsafeRemoveLockFile();
- break;
- case LockFileManager::LFS_Owned:
- break;
- case LockFileManager::LFS_Shared: {
- switch (Locked.waitForUnlock()) {
+ Lock.unsafeRemoveLockFile();
+ } else if (!Owned) {
+ switch (Lock.waitForUnlock()) {
case LockFileManager::Res_Success:
break;
case LockFileManager::Res_OwnerDied:
@@ -1566,11 +1564,9 @@ PreservedAnalyses AMDGPUSplitModulePass::run(Module &M,
dbgs()
<< "[amdgpu-split-module] unable to acquire lockfile, debug "
"output may be mangled by other processes\n");
- Locked.unsafeRemoveLockFile();
+ Lock.unsafeRemoveLockFile();
break; // give up
}
- break;
- }
}
splitAMDGPUModule(TTIGetter, M, N, ModuleCallback);
diff --git a/llvm/unittests/Support/LockFileManagerTest.cpp b/llvm/unittests/Support/LockFileManagerTest.cpp
index 552053d46e843..627b2daef650c 100644
--- a/llvm/unittests/Support/LockFileManagerTest.cpp
+++ b/llvm/unittests/Support/LockFileManagerTest.cpp
@@ -9,6 +9,7 @@
#include "llvm/Support/LockFileManager.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
+#include "llvm/Testing/Support/Error.h"
#include "llvm/Testing/Support/SupportHelpers.h"
#include "gtest/gtest.h"
#include <memory>
@@ -27,12 +28,12 @@ TEST(LockFileManagerTest, Basic) {
{
// The lock file should not exist, so we should successfully acquire it.
LockFileManager Locked1(LockedFile);
- EXPECT_EQ(LockFileManager::LFS_Owned, Locked1.getState());
+ EXPECT_THAT_EXPECTED(Locked1.tryLock(), HasValue(true));
// Attempting to reacquire the lock should fail. Waiting on it would cause
// deadlock, so don't try that.
LockFileManager Locked2(LockedFile);
- EXPECT_NE(LockFileManager::LFS_Owned, Locked2.getState());
+ EXPECT_THAT_EXPECTED(Locked2.tryLock(), HasValue(false));
}
// Now that the lock is out of scope, the file should be gone.
@@ -68,7 +69,7 @@ TEST(LockFileManagerTest, LinkLockExists) {
// The lock file doesn't point to a real file, so we should successfully
// acquire it.
LockFileManager Locked(LockedFile);
- EXPECT_EQ(LockFileManager::LFS_Owned, Locked.getState());
+ EXPECT_THAT_EXPECTED(Locked.tryLock(), HasValue(true));
}
// Now that the lock is out of scope, the file should be gone.
@@ -93,7 +94,7 @@ TEST(LockFileManagerTest, RelativePath) {
{
// The lock file should not exist, so we should successfully acquire it.
LockFileManager Locked(LockedFile);
- EXPECT_EQ(LockFileManager::LFS_Owned, Locked.getState());
+ EXPECT_THAT_EXPECTED(Locked.tryLock(), HasValue(true));
EXPECT_TRUE(sys::fs::exists(FileLock.str()));
}
More information about the cfe-commits
mailing list