bneradt commented on code in PR #13936:
URL: https://github.com/apache/trafficserver/pull/13936#discussion_r4415994848


##########
cmake/Findunwind.cmake:
##########
@@ -28,10 +28,21 @@
 #     unwind::unwind
 #
 
-find_library(unwind_LIBRARY NAMES unwind)
 find_library(unwind_ptrace_LIBRARY NAMES unwind-ptrace)
 find_library(unwind_generic_LIBRARY NAMES unwind-generic)
-find_path(unwind_INCLUDE_DIR NAMES libunwind.h libunwind/libunwind.h)
+# LLVM's unwinder also installs a libunwind.so and libunwind.h, which may be
+# earlier in the search path but lack the remote unwinding API. Take the ones
+# installed alongside libunwind-ptrace.
+if(unwind_ptrace_LIBRARY)
+  get_filename_component(_unwind_library_dir "${unwind_ptrace_LIBRARY}" 
DIRECTORY)
+  find_library(
+    unwind_LIBRARY
+    NAMES unwind
+    PATHS "${_unwind_library_dir}"
+    NO_DEFAULT_PATH
+  )
+endif()
+find_path(unwind_INCLUDE_DIR NAMES libunwind-ptrace.h 
libunwind/libunwind-ptrace.h)

Review Comment:
   The minimal fix is to invalidate only incompatible cached selections before 
the existing searches:
   
   ```cmake
   # Immediately after computing _unwind_library_dir:
   if(unwind_LIBRARY)
     get_filename_component(_unwind_cached_dir "${unwind_LIBRARY}" DIRECTORY)
     if(NOT _unwind_cached_dir STREQUAL _unwind_library_dir)
       unset(unwind_LIBRARY CACHE)
     endif()
   endif()
   
   # Immediately before find_path(unwind_INCLUDE_DIR ...):
   if(unwind_INCLUDE_DIR
      AND NOT EXISTS "${unwind_INCLUDE_DIR}/libunwind-ptrace.h"
      AND NOT EXISTS "${unwind_INCLUDE_DIR}/libunwind/libunwind-ptrace.h"
   )
     unset(unwind_INCLUDE_DIR CACHE)
   endif()
   ```
   
   I see that 8801856a09b5 now implements exactly this approach. I checked the 
updated finder with a minimal CMake fixture containing competing LLVM and 
remote-unwinding installations: it repairs caches with both paths stale, only 
the library stale, or only the include directory stale. A fresh configure also 
selects the intended installation, and a subsequent configure leaves the valid 
cache unchanged. This addresses my finding; no further patch is needed for it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to