square / pylink

Python Library for device debugging/programming via J-Link
https://pylink.readthedocs.io/en/latest/
Other
351 stars 128 forks source link

pylink.Library does not honor LD_LIBRARY_PATH on Linux #131

Closed dottspina closed 2 years ago

dottspina commented 2 years ago

Thanks for maintaining this library.

The demonstrable problem assumes:

pylink will then fail to load the library, even when LD_LIBRARY_PATH includes the actual JLink installation directory.

For e.g. with pyOCD (v0.33.1):

$ export LD_LIBRARY_PATH=$HOME/arbitrary/path/to/JLink
$ pyocd list                                   
No available debug probes are connected

Preferable behavior: pylink honors LD_LIBRARY_PATH on Linux. If I understand correctly it's also the expected behavior.

OTOH, in the ctypes_util.find_library() reference documentation we read:

name is the library name without any prefix like lib, suffix like .so, .dylib or version number

Which would not match libjlinkarm (aka Library._sdk, aka JLINK_SDK_NAME), but rather jlinkarm.

And indeed, ctypes.cdll.LoadLibrary(ctypes_util.find_library('jlinkarm')) correctly honors LD_LIBRARY_PATH.

I understand we still have to find the library absolute path to be able to make a temp copy (work around for multiple J-Links within the same process): I've implemented a PoC, with some contortion (see attached patch bellow).

On Linux, pylink will then:

$ export LD_LIBRARY_PATH=$HOME/arbitrary/path/to/JLink
$ pyocd list                                   
  #   Probe                                    Unique ID  
----------------------------------------------------------
  0   Segger J-Link OB-SAM3U128-V2-NordicSem   683xxxxxx  

This patch is really a PoC, and probably not the right approach: IMHO path = ctypes_util.find_library(self._sdk) does not have the intended semantic to begin with (at least on Linux, and probably on MacOS), and a proper fix may imply a bit more rework to the function pylink.Library.load_default().

Thanks.

-- chris

I'm not sure we can actually attach a patch file to a GitHub issue: I'm willing to setup a PR if helpful.

---
 pylink/library.py | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/pylink/library.py b/pylink/library.py
index 44a3e8d..63fceb5 100644
--- a/pylink/library.py
+++ b/pylink/library.py
@@ -21,6 +21,57 @@ import sys
 import tempfile

+class DlInfo:
+    """
+    Helper used to find absolute path of sofile for a given dlname
+    (library name without any prefix like lib, suffix like .so).
+
+    For e.g.:
+    - LD_LIBRARY_PATH='/mnt/platform/segger/JLink
+    - DlInfo('jlinkarm').path -> '/mnt/platform/segger/JLink/libjlinkarm.so.7'
+
+    This is done by:
+    - loading the library using the dlname
+    - calling native dlinfo() to retrieve the file path
+
+    Implementation adapted from @cloudflightio: https://github.com/cloudflightio/python-dlinfo/
+    """
+
+    RTLD_DI_LINKMAP = 2
+
+    class LinkMap(ctypes.Structure):
+        _fields_ = [
+            ('l_addr', ctypes.c_void_p),
+            ('l_name', ctypes.c_char_p),
+            ('l_ld', ctypes.c_void_p),
+            ('l_next', ctypes.c_void_p),
+            ('l_previous', ctypes.c_void_p),
+        ]
+
+    def __init__(self, libname :str):
+        self._so_path = None
+
+        self._libdl = ctypes_util.find_library('dl')
+        if self._libdl is not None:
+            self._linkmap = ctypes.c_void_p()
+            self._LIBDL = ctypes.cdll.LoadLibrary(self._libdl)
+            self._DLINFO = self._LIBDL.dlinfo
+            self._DLINFO.argtypes = ctypes.c_void_p, ctypes.c_int, ctypes.c_void_p
+            self._DLINFO.restype = ctypes.c_int
+
+            so_name = ctypes_util.find_library(libname)
+            if so_name is not None:
+                tmp_lib = ctypes.cdll.LoadLibrary(so_name)
+                if self._DLINFO(tmp_lib._handle, self.RTLD_DI_LINKMAP, ctypes.byref(self._linkmap)) == 0:
+                    self._linkmap = ctypes.cast(self._linkmap, ctypes.POINTER(DlInfo.LinkMap))
+                    self._so_path = self._linkmap.contents.l_name.decode(sys.getdefaultencoding())
+                tmp_lib = None
+
+    @property
+    def path(self) -> str:
+        return self._so_path
+
+
 class Library(object):
     """Wrapper to provide easy access to loading the J-Link SDK DLL.

@@ -87,6 +138,10 @@ class Library(object):
     WINDOWS_32_JLINK_SDK_NAME = 'JLinkARM'
     WINDOWS_64_JLINK_SDK_NAME = 'JLink_x64'

+    # Poor man's singleton: we'll instantiate this only once
+    #
+    _dlinfo = None
+
     @classmethod
     def get_appropriate_windows_sdk_name(cls):
         """Returns the appropriate JLink SDK library name on Windows depending
@@ -254,6 +309,8 @@ class Library(object):
             self._sdk = self.get_appropriate_windows_sdk_name()
         else:
             self._sdk = self.JLINK_SDK_NAME
+            # Library name without prefix and suffix (as in gcc -l<name>)
+            self._dlname = "jlinkarm"

         if dllpath is not None:
             self.load(dllpath)
@@ -283,6 +340,20 @@ class Library(object):
         Returns:
           ``True`` if the DLL was loaded, otherwise ``False``.
         """
+
+        # Honor LD_LIBRARY_PATH on Linux
+        if sys.platform.startswith('linux'):
+            if self.load_default_linux():
+                return True
+
+        # According to the ctypes API documentation,
+        # in find_library(<name>), <name> is the library name
+        # without any prefix like lib, suffix like .so, .dylib
+        #
+        # ctypes_util.find_library('libjlinkarm') does indeed fail on Linux
+        # even when LD_LIBRARY_PATH is set,
+        # and might as well fail on MacOS even when DYLD_LIBRARY_PATH is set.
+        #
         path = ctypes_util.find_library(self._sdk)
         if path is None:
             # Couldn't find it the standard way.  Fallback to the non-standard
@@ -300,6 +371,17 @@ class Library(object):

         return False

+    def load_default_linux(self):
+        """
+        """
+        if Library._dlinfo is None:
+            Library._dlinfo = DlInfo(self._dlname)
+        path = Library._dlinfo.path
+
+        if path is not None:
+            return self.load(path)
+        return False
+
     def load(self, path=None):
         """Loads the specified DLL, if any, otherwise re-loads the current DLL.

-- 
2.34.1
dottspina commented 2 years ago

I'm quite confident that path = ctypes_util.find_library(self._sdk) is not always correct:

OS self._sdk find_library()
Windows 'JLink{ARM,_x64}' should answer the expected file path
MacOS 'libjlinkarm' will fail (the library name should be 'jlinkarm')
Linux 'libjlinkarm' will fail, and would not return the full file path anyway

I've written a new patch, going forward my former comment, that completely removes the useless find_library('libjlinkarm') call, and makes more fluent we:

I agree we are few who install the JLink Sofware pack not in its default location, but there are valid use cases: for e.g. this issue prevents to write a developer guide, involving pyOCD, without assuming the reader has the permission to write in the /opt directory.

I've created a PR (#132 ), which, if I'm correct, should preserve the code path and semantic on Windows, and honor both LD_LIBRARY_PATH on Linux and DYLD_LIBRARY_PATH on MacOS. It should not mess with the whole gymnastic pylink implements to "work around a limitation of the J-Link DLL in which multiple J-Links cannot be accessed from the same process".

Thanks.

-- chris

hkpeprah commented 2 years ago

Should be resolved with #132 (which you added 😛), and available in v0.14.0. Thanks for the patch.

dottspina commented 2 years ago

@hkpeprah, thanks for the prompt PyPI update.

Seems to work fine here:

$ pyocd list
No available debug probes are connected
$ pip install -U pylink-square
[...]
Successfully uninstalled pylink-square-0.13.0
Successfully installed pylink-square-0.14.1
$ pyocd list                 
  #   Probe/Board                              Unique ID   Target  
-------------------------------------------------------------------
  0   Segger J-Link OB-SAM3U128-V2-NordicSem   683529xxx   n/a     

For me we can close this issue as fixed.

Thanks.

-- chris

hkpeprah commented 2 years ago

Thanks for the confirmation!