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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

It's a pitty about flock&solaris. It's a much saner api, and a lot closer to the windows implementation.

I guess we have to stick to fcntl then. The new version looks good. I just think it would be good to acknowledge the weirdness of the flock api in the method documentation. Maybe something like "Care should be taken when using this function in a multithreaded context, as it may not prevent other threads in the same process from obtaining a lock on the same file, even if they are using a different file descriptor."



================
Comment at: llvm/lib/Support/Unix/Path.inc:1066
+      return std::error_code(Error, std::generic_category());
+    usleep(1000);
+  } while (std::chrono::steady_clock::now() < End);
----------------
MaskRay wrote:
> I feel uneasy with usleep(1000). Why is it needed?
That's because the os provides no mechanism to wait for a lock to become available for a given amount of time (well.... one could use `SIGALRM`s to achieve that, but those come with problems of their own..)

Using exponential backoff for this might be a good idea, but I also think that can wait until this becomes a problem.


================
Comment at: llvm/unittests/Support/ProgramTest.cpp:369
+  SmallString<128> EnvStr;
+  Twine EnvVar = "LLVM_PROGRAM_TEST_LOCKED_FILE=" + LockedFile.str();
+  addEnvVar(EnvVar.toStringRef(EnvStr));
----------------
Although this will work in this particular case, using Twines in this way is very dangerous (this is _almost_ the same as the one that <http://llvm.org/docs/ProgrammersManual.html#llvm-adt-twine-h> warns about). As performance is definitely not relevant here, it would be better to just use strings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896