[Support][Cygwin] Fix handling of Process symbol lookup.

[jeremyd2019]

In Unix/DynamicLibrary.inc, it was already known that Cygwin required use of RTLD_DEFAULT as the Handle parameter to DLSym to search all modules for a symbol. Unfortunately, RTLD_DEFAULT is defined as NULL, so the existing checks of the Process handle meant DLSym would never be called on Cygwin. Use the existing &Invalid sentinel instead of nullptr for the Process handle.

[llvmbot]

@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-llvm-support

Author: None (jeremyd2019)

Changes

In Unix/DynamicLibrary.inc, it was already known that Cygwin required use of RTLD_DEFAULT as the Handle parameter to DLSym to search all modules for a symbol. Unfortunately, RTLD_DEFAULT is defined as NULL, so the existing checks of the Process handle meant DLSym would never be called on Cygwin. Add a bool member to indicate whether the Process handle was set instead.

---

Full diff: https://github.com/llvm/llvm-project/pull/143072.diff

1 Files Affected:

• (modified) llvm/lib/Support/DynamicLibrary.cpp (+4-2)

diff --git a/llvm/lib/Support/DynamicLibrary.cpp b/llvm/lib/Support/DynamicLibrary.cpp
index 531c035ab9266..34c2dc2e56227 100644
--- a/llvm/lib/Support/DynamicLibrary.cpp
+++ b/llvm/lib/Support/DynamicLibrary.cpp
@@ -26,6 +26,7 @@ class DynamicLibrary::HandleSet {
   typedef std::vector<void *> HandleList;
   HandleList Handles;
   void *Process = nullptr;
+  bool ProcessAdded = false;
 
 public:
   static void *DLOpen(const char *Filename, std::string *Err);
@@ -66,6 +67,7 @@ class DynamicLibrary::HandleSet {
       }
 #endif
       Process = Handle;
+      ProcessAdded = true;
     }
     return true;
   }
@@ -97,11 +99,11 @@ class DynamicLibrary::HandleSet {
     assert(!((Order & SO_LoadedFirst) && (Order & SO_LoadedLast)) &&
            "Invalid Ordering");
 
-    if (!Process || (Order & SO_LoadedFirst)) {
+    if (!ProcessAdded || (Order & SO_LoadedFirst)) {
       if (void *Ptr = LibLookup(Symbol, Order))
         return Ptr;
     }
-    if (Process) {
+    if (ProcessAdded) {
       // Use OS facilities to search the current binary and all loaded libs.
       if (void *Ptr = DLSym(Process, Symbol))
         return Ptr;

[jeremyd2019]

cc @mstorsjo

this seemed less invasive than trying to use this as the Process handle, as seems to be done on the Windows/DynamicLibrary.inc side of things (since we don't have to do any additional special handling in the Unix/DynamicLibrary.inc side beyond using RTLD_DEFAULT instead of the executable's handle for Process).

This brought llvm test failures on Cygwin down from 110 to 50.

[mstorsjo]

LGTM

[kikairoya]

llvm::DynamicLibrary already has a symbol representing invalid state. How about to use this?

llvm-project/llvm/include/llvm/Support/DynamicLibrary.h

Line 38 in 95b3fd6

LLVM_ABI static char Invalid;

[jeremyd2019]

So instead of having Process initialized to nullptr, initialize it to &Invalid ? That could work I guess

[kikairoya]

LGTM (but I don't have access to approve)

[kikairoya]

Could #143246 be the cause of the CI failure?

[jeremyd2019]

I think it was probably the missing checks in the .inc files, which I've now added

[jeremyd2019]

LGTM (but I don't have access to approve)

I'm still waiting on mine, or I'd be looking harder at reviewing and approving your PRs. Hopefully @mstorsjo can take care of this chore in the meantime.

[mstorsjo]

LGTM