[PATCH] D78896: [Support] Add file lock/unlock functions

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1136
+///
+/// This function implements advisory locks on entire file. If it returns
+/// <em>errc::success</em>, the process may read or write the file assuming no
----------------
Is the "advisory" part really true on windows? My impression is that is not true (at least not in the sense that posix uses this term). [[ https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-writefile | WriteFile documentation ]] says:
```
If part of the file is locked by another process and the write operation overlaps the locked portion, WriteFile fails.
```


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1138
+/// <em>errc::success</em>, the process may read or write the file assuming no
+/// other processes may change it. Any other process that calls this function
+/// would get <em>errc::no_lock_available</em> until the process owning the lock
----------------
I find this "may change it" part very ambiguous. Did you mean that the process may assume that no other process changes that file (because the operating system guarantees that) or that the process must assume that no other process will change it (because there is no mechanism in the OS to prevent it).

Note that if my thoughts on WriteFile above are true, then neither of the two interpretations are correct, and all we can say is something like. "Attempts to lock the file by other processes will fail/block, but the caller should not assume that the file cannot be modified by uncooperating processes who access it without locking."


================
Comment at: llvm/unittests/Support/Path.cpp:2054-2109
+  std::mutex Start2M;
+  std::condition_variable Start2CV;
+  bool Start2 = false;
+  std::mutex LockM;
+  std::condition_variable LockCV;
+  bool Locked = false;
+
----------------
I get the impression this code is much more complicated then needed. There's a lot of synchronization going on but it still does not guarantee the that the file is unlocked while the other thread is inside the `tryLock` call (my goal was to get coverage for the while loop). How about something like:
```
EC = fs::tryLockFile(FD1);
ASSERT_NO_ERROR(EC);
EC = fs::tryLockFile(FD2);
ASSERT_ERROR(EC);
std::thread LockThread([&] { EC2 = fs::tryLockFile(FD2, std::chrono::minutes(1)); });
std::this_thread::sleep_for(std::chrono::seconds(1));
EC = fs::unlockFile(FD1);
ASSERT_NO_ERROR(EC);
LockThread.join();
ASSERT_NO_ERROR(EC2);
EC = fs::unlockFile(FD2);
ASSERT_NO_ERROR(EC);
```
It still does not guarantee that the other thread is inside the `tryLockFile` call, but it comes as close as we can get, and it avoids all the condition_variable overhead.


================
Comment at: llvm/unittests/Support/Path.cpp:2063
+  bool NoLock = true;
+  auto Thread1 = std::thread([&]() {
+    ECT1a = fs::tryLockFile(FD1);
----------------
I don't think this is a good use of auto per the coding standards <http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable>.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78896/new/

https://reviews.llvm.org/D78896