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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Support/DynamicLibrary.cpp:55
+class HandleSet {
+  // Cannot use an llvm::SetVector as no access to internal vector there.
+  std::vector<void*> Handles;
----------------
Maybe vector is the wrong container to use here, because it forces us to do O(n) work in one case. How about using a SmallPtrSet to do uniqueness testing, and a std::forward_list to preserve order?


================
Comment at: lib/Support/DynamicLibrary.cpp:57
+  std::vector<void*> Handles;
+  bool KeepFront;
+public:
----------------
Instead of having a 'KeepFront' switch, do you think it might be more convenient to store the RTLD_DEFAULT/IsProcess handle separately from the rest of the handles?


================
Comment at: lib/Support/DynamicLibrary.cpp:107
 
-  // If we've already loaded this library, dlclose() the handle in order to
-  // keep the internal refcount at +1.
-  if (!OpenedHandles->insert(handle).second)
-    dlclose(handle);
+  OpenedHandles->AddLibrary(handle, filename==nullptr);
 
----------------
The convention is to write e.g '/*IsProcess=*/filename==nullptr', so it's clear which parameter you're passing in.


================
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");
----------------
marsupial wrote:
> 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.
If there isn't a guarantee that, e.g a third thread C couldn't load another dylib after this loop terminates, I don't think we should have the loop.


================
Comment at: lib/Support/Windows/DynamicLibrary.inc:83
 
-static BOOL CALLBACK
-ELM_Callback(PCSTR ModuleName, DWORD64 ModuleBase,
-             ULONG ModuleSize, PVOID UserContext) {
-  OpenedHandles->insert((HMODULE)ModuleBase);
-  return TRUE;
+  void AddLibrary(HMODULE Lib, bool CanFree = true) {
+    // Check if we've already loaded this library.
----------------
I'm hoping there's a way to reduce code duplication between the windows/unix logic. Have you tried factoring out some of the generic logic in HandleSet s.t it can call into platform-specific stubs?


https://reviews.llvm.org/D30107