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

[Frederich Munch via Phabricator via llvm-commits]

marsupial 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;
----------------
vsk wrote:
> 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?
I'm of the opinion that search will probably be fast enough and in the cases where it would't be using twice the memory is a worse penalty.


================
Comment at: lib/Support/DynamicLibrary.cpp:57
+  std::vector<void*> Handles;
+  bool KeepFront;
+public:
----------------
vsk wrote:
> 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?
Seems reasonable. Hopefully I haven't already tried that and there is a reason not...


================
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:
> 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.
Generally I agree, but the cost of one compare that will most likely succeed to deal with an obscure event that would be hard to debug seems prudent?


================
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.
----------------
vsk wrote:
> 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?
Yes there is a lot that can be reduced...just didn't want the patch to keep growing.
If this is the direction, I would probably add **Unix/DynamicLibrary.inc** and both the DynamicLibrary.inc files would be the implementation of HandleSet.

Leaving DynamicLibrary.cpp the common implementation of the public class.


https://reviews.llvm.org/D30107