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())); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
