Add platform discovery search paths #473

kswiecicki · 2023-04-26T14:46:30Z

This patch makes the loader search additional directories when searching for hardcoded adapter libraries.
Additional search paths can be added via UR_ADAPTERS_SEARCH_PATH environmental variable that holds a comma-separated list of directory paths (e.g. UR_ADAPTERS_SEARCH_PATH=/home/kswiecic/dev/sycl_workspace/libs/).
The loader now also searches the directory it was linked from.

Related issue: #128

bmyates · 2023-04-26T15:04:57Z

source/loader/ur_adapter_registry.hpp

-        for (const auto &path : knownPlatformNames) {
-            discovered_adapters.emplace_back(path);
+        auto searchPaths = getEnvAdapterSearchPaths();
+        auto loaderPath = getLoaderLibDirectory();


Would this result in adapter libraries in loader lib directory to take precedence over libraries discovered in LD_LIBRARY_PATH?

As a user I would expect LD_LIBRARY_PATH to have highest search precedence

I've set the highest search precedence to UR_ADAPTERS_SEARCH_PATH and made LD_LIBRARY_PATH a higher priority than the current loader lib directory.

source/loader/ur_adapter_registry.hpp

source/loader/windows/loader_location.cpp

source/loader/ur_adapter_registry.hpp

igchor · 2023-05-02T19:22:31Z

source/loader/linux/loader_location.cpp

+static std::optional<std::vector<fs::path>> getEnvOSSearchPaths() {
+    std::optional<std::string> pathStringsOpt;
+    try {
+        pathStringsOpt = ur_getenv("LD_LIBRARY_PATH");


I looked at the documentation for ld.so.8 (https://man7.org/linux/man-pages/man8/ld.so.8.html) and I'm actually not sure that interpreting LD_LIBRARY_PATH within UR is a good idea. There are a few special cases that we might not cover by interpreting it ourselves:

"This variable is ignored in secure-execution mode"

"Within the pathnames specified in LD_LIBRARY_PATH, the
dynamic linker expands the tokens $ORIGIN, $LIB, and
$PLATFORM (or the versions using curly braces around the
names) as described above in Dynamic string tokens."

The directory lists can also be separated by using semicolons

I think it would be safer to let the linker handle LD_LIBRARY_PATH.

There might be something similiar on Windows as well.

Since that was the case, I've changed the adapter registry to store a sorted list of paths that should be searched when looking for each hardcoded adapter. Eg. 1. Paths from UR_ADAPTERS_SEARCH_PATH. 2. Hardcoded library name (in that case the OS searches for this library). 3. UR Loader directory (current binary/library directory).

source/common/ur_util.hpp

PatKamin · 2023-05-05T07:38:13Z

source/loader/linux/loader_location.cpp

+ */
+
+#include <dlfcn.h>
+#include <filesystem>


See PR #440 for handling an alternative include:

#if __has_include(<filesystem>) #include <filesystem> namespace filesystem = std::filesystem; #else #include <experimental/filesystem> namespace filesystem = std::experimental::filesystem; #endif

PatKamin · 2023-05-05T08:10:17Z

source/loader/ur_adapter_registry.hpp

+                      std::back_inserter(searchPaths));
+        }
+
+        for (const auto &platformName : knownPlatformNames) {


nit: platformName -> adapterName

PatKamin · 2023-05-05T08:14:03Z

test/loader/adapter_registry/CMakeLists.txt

+# Copyright (C) 2023 Intel Corporation
+# SPDX-License-Identifier: MIT
+
+set(DUMMY_LIB_DIR ${CMAKE_BINARY_DIR}/bin/${CMAKE_BUILD_TYPE}/adapter-reg)


This has no effect as this variable is set again below.

I don't need this variable anymore.

PatKamin · 2023-05-05T08:15:36Z

test/loader/adapter_registry/CMakeLists.txt

+    set(DUMMY_LIB_PATH ${DUMMY_LIB_DIR}/libur_adapter_level_zero.so.0)
+endif()
+
+# Create a dummy adapter library in adapter the runtime output directory


in the adapter runtime output directory?

I've removed this part.

PatKamin · 2023-05-05T08:42:04Z

test/loader/adapter_registry/CMakeLists.txt

+set(DUMMY_LIB_DIR ${CMAKE_BINARY_DIR}/bin/${CMAKE_BUILD_TYPE}/adapter-reg)
+
+if(MSVC)
+    set(DUMMY_LIB_DIR ${CMAKE_BINARY_DIR}/bin/${CMAKE_BUILD_TYPE}/adapter-reg)


Please, check if both MSVC and non-MSVC paths could be resolved with CMAKE_CURRENT_BINARY_DIR:

${CMAKE_CURRENT_BINARY_DIR}/adapter-reg

It's no longer relevant.

PatKamin · 2023-05-05T08:43:18Z

test/loader/adapter_registry/CMakeLists.txt

+endif()
+
+# Create a dummy adapter library in adapter the runtime output directory
+file(WRITE ${DUMMY_LIB_PATH} "")


Since this will be created in the UR bin folder, consider removing it after tests finish. You might need to create a cmake script for that.

It's no longer relevant.

bratpiorka · 2023-05-08T15:32:00Z

source/loader/windows/loader_location.cpp

+
+#include "ur_loader.hpp"
+
+#define MAX_PATH_LEN_WIN 1024


why 1024? MAX_PATH is 256 (default) or 32K (need changes in registry)

That was an arbitrary number that I was going to change in the future but forgot.
I've made the buffer bigger, equal to the maximum possible path length according to https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry.

test/loader/adapter_registry/fixtures.hpp

source/loader/ur_adapter_registry.hpp

pbalcer · 2023-05-09T09:47:16Z

source/loader/ur_adapter_registry.hpp

+                           [](const std::string &s) { return fs::path(s); });
+            paths.erase(std::remove_if(paths.begin(), paths.end(),
+                                       [](const fs::path &path) {
+                                           return !fs::exists(path);


warn("incorrect path...") ?

pbalcer · 2023-05-09T09:48:22Z

scripts/core/INTRO.rst

@@ -260,6 +260,15 @@ Specific environment variables can be set to control the behavior of unified run

    This environment variable should be used for development and debugging only.


Add here that the search path is not considered when force load is used.

Done. I've also added this information to the UR_ADAPTERS_SEARCH_PATH notes.

pbalcer · 2023-05-09T09:57:22Z

source/loader/ur_adapter_registry.hpp

+            std::vector<fs::path> paths;
+            auto pathStrings = pathStringsOpt.value();
+
+            std::transform(pathStrings.cbegin(), pathStrings.cend(),


pbalcer · 2023-05-09T09:59:00Z

source/loader/ur_adapter_registry.hpp

+            // 3. Loader library directory.
+            if (searchPathsEnvOpt.has_value()) {
+                auto searchPathsEnv = searchPathsEnvOpt.value();
+                std::transform(searchPathsEnv.cbegin(), searchPathsEnv.cend(),


if (searchPathsEnvOpt) { for (const auto& p : searchPathsEnvOpt.value()) { loadPaths.emplace_back(p / adapterName); } }

pbalcer · 2023-05-09T09:59:55Z

source/loader/ur_adapter_registry.hpp

    }

  private:
-    std::vector<std::string> discovered_adapters;
+    std::vector<std::vector<fs::path>> adaptersLoadPaths;


Add a comment that each entry in an outer vector is an adapter, and the inner vector contains candidate paths to load.

Adapter registry now checks the UR_ADAPTERS_SEARCH_PATH env variable for search paths and takes them into the account when looking for hardcoded adapter libraries. It also searches the location of the loader library.

getenv_to_vec now accepts values with colons ':' and semicolons ';' if the values are inside either single ' ' or double " " quotation marks.

bmyates reviewed Apr 26, 2023

View reviewed changes

igchor reviewed Apr 26, 2023

View reviewed changes

source/loader/ur_adapter_registry.hpp Outdated Show resolved Hide resolved

kswiecicki force-pushed the platform-discovery-win branch from 8784f6c to 48bad61 Compare April 28, 2023 14:51

igchor reviewed Apr 28, 2023

View reviewed changes

source/loader/windows/loader_location.cpp Outdated Show resolved Hide resolved

source/loader/ur_adapter_registry.hpp Outdated Show resolved Hide resolved

kswiecicki force-pushed the platform-discovery-win branch 2 times, most recently from aaf899c to f484512 Compare May 2, 2023 16:03

igchor reviewed May 2, 2023

View reviewed changes

PatKamin reviewed May 5, 2023

View reviewed changes

kswiecicki force-pushed the platform-discovery-win branch 5 times, most recently from 3a9d616 to db1d49d Compare May 8, 2023 14:54

bratpiorka reviewed May 8, 2023

View reviewed changes

kswiecicki force-pushed the platform-discovery-win branch 2 times, most recently from 708efa9 to afb16a1 Compare May 8, 2023 15:46

igchor approved these changes May 8, 2023

View reviewed changes

test/loader/adapter_registry/fixtures.hpp Show resolved Hide resolved

source/loader/ur_adapter_registry.hpp Show resolved Hide resolved

igchor approved these changes May 8, 2023

View reviewed changes

bratpiorka approved these changes May 9, 2023

View reviewed changes

pbalcer reviewed May 9, 2023

View reviewed changes

kswiecicki added 5 commits May 9, 2023 14:09

Add new adapter search paths

59f3041

Adapter registry now checks the UR_ADAPTERS_SEARCH_PATH env variable for search paths and takes them into the account when looking for hardcoded adapter libraries. It also searches the location of the loader library.

Add adapter registry search path tests

b4f766c

Document UR_ADAPTERS_SEARCH_PATH usage

ae0c108

Refactor getenv_to_vec

0e477ad

getenv_to_vec now accepts values with colons ':' and semicolons ';' if the values are inside either single ' ' or double " " quotation marks.

Add quotes to the UR_ADAPTERS_FORCE_LOAD values

378293c

kswiecicki force-pushed the platform-discovery-win branch from afb16a1 to 378293c Compare May 9, 2023 13:25

Move filesystem include to a separate header

58a039a

pbalcer approved these changes May 9, 2023

View reviewed changes

pbalcer merged commit abdd25c into oneapi-src:main May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add platform discovery search paths #473

Add platform discovery search paths #473

kswiecicki commented Apr 26, 2023 •

edited

Loading

bmyates Apr 26, 2023

kswiecicki May 2, 2023

igchor May 2, 2023

igchor May 2, 2023

kswiecicki May 8, 2023

PatKamin May 5, 2023

kswiecicki May 8, 2023

PatKamin May 5, 2023

kswiecicki May 8, 2023

PatKamin May 5, 2023

kswiecicki May 8, 2023

PatKamin May 5, 2023

kswiecicki May 8, 2023

PatKamin May 5, 2023

kswiecicki May 8, 2023

PatKamin May 5, 2023

kswiecicki May 8, 2023

bratpiorka May 8, 2023

kswiecicki May 8, 2023

pbalcer May 9, 2023

kswiecicki May 9, 2023

pbalcer May 9, 2023

kswiecicki May 9, 2023

pbalcer May 9, 2023

kswiecicki May 9, 2023

pbalcer May 9, 2023

kswiecicki May 9, 2023

pbalcer May 9, 2023

kswiecicki May 9, 2023

		@@ -260,6 +260,15 @@ Specific environment variables can be set to control the behavior of unified run

		This environment variable should be used for development and debugging only.

Add platform discovery search paths #473

Add platform discovery search paths #473

Conversation

kswiecicki commented Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kswiecicki commented Apr 26, 2023 •

edited

Loading