[PATCH] D30107: Make DynamicLibrary::getPermanentLibrary on Windows have a defined ordering.

[Frederich Munch via Phabricator via llvm-commits]

marsupial added inline comments.


================
Comment at: lib/Support/Windows/DynamicLibrary.inc:38
+static std::unique_ptr<ModuleHandles> OpenedHandles;
+static bool KeepFront;
 
----------------
vsk wrote:
> Please use ManagedStatic.
Are you sure about that?
This code already has a lock guarding it.
What does ManagedStatic add?


================
Comment at: lib/Support/Windows/DynamicLibrary.inc:66
+      Current.resize(Bytes / sizeof(HMODULE));
+      if (!EnumProcessModulesEx(Self, Current.data(), Bytes, &Bytes, Flags)) {
+        MakeErrMsg(errMsg, "EnumProcessModulesEx failure");
----------------
vsk wrote:
> Could you explain with some more detail why EnumProcessModulesEx needs to be repeated until its results don't change? Is there any guarantee that its results won't change again after this loop terminates?
> 
> Apologies if my Windows ignorance is showing here...
There is a small chance of thread A calling EnumProcessModulesEx to get the size of the list and in between that and EnumProcessModulesEx to get the actual list that thread B does something to explicitly or implicitly load another dll.


================
Comment at: lib/Support/Windows/DynamicLibrary.inc:73
+#ifndef NDEBUG
+    if (OpenedHandles) {
+      auto Begin = Current.begin(), End = Current.end();
----------------
vsk wrote:
> At this point, I'd really expect OpenedHandles to be initialized.
Current has yet to be swapped into OpenedHandles there (line 90).


================
Comment at: lib/Support/Windows/DynamicLibrary.inc:106
   }
-  
+
   HMODULE a_handle = LoadLibraryW(filenameUnicode.data());
----------------
vsk wrote:
> Try to avoid unrelated whitespace changes.
Ok, but there was a spurious indent there.


https://reviews.llvm.org/D30107