Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(on linux) the hardcoded driver names include the library version #1

Open
fwyzard opened this issue Mar 17, 2020 · 9 comments
Open

Comments

@fwyzard
Copy link

fwyzard commented Mar 17, 2020

On Linux, the hardcoded library names include the major and minor library version, i.e.

MAKE_LIBRARY_NAME( "ze_intel_gpu", "0.4")

results in libze_intel_gpu.so.0.4.

Being hardcoded, this breaks loading the driver when its major or minor version changes (currently it is libze_intel_gpu.so.0.8).

Until a proper mechanism for enumerating the drivers is introduced, would it make sense to drop the version number from the library name also on Linux, as it is already the case on Windows ?

@fwyzard
Copy link
Author

fwyzard commented Mar 17, 2020

See intel/compute-runtime#279.

@fwyzard
Copy link
Author

fwyzard commented Mar 17, 2020

Here is a patch:

diff --git a/source/inc/ze_util.h b/source/inc/ze_util.h
index 07a50c8..6aec3c8 100644
--- a/source/inc/ze_util.h
+++ b/source/inc/ze_util.h
@@ -21,6 +21,7 @@
 #  include <dlfcn.h>
 #  define HMODULE void*
 #  define MAKE_LIBRARY_NAME(NAME, VERSION)    "lib" NAME ".so." VERSION
+#  define MAKE_LIBRARY_NAMELINK(NAME)         "lib" NAME ".so"
 #  define MAKE_VALIDATION_LAYER_NAME(NAME)    "lib" NAME ".so." L0_VALIDATION_LAYER_SUPPORTED_VERSION
 #  define LOAD_DRIVER_LIBRARY(NAME) dlopen(NAME, RTLD_LAZY|RTLD_LOCAL)
 #  define FREE_DRIVER_LIBRARY(LIB)  if(LIB) dlclose(LIB)
@@ -28,6 +29,7 @@
 #elif defined(_WIN32)
 #  include <Windows.h>
 #  define MAKE_LIBRARY_NAME(NAME, VERSION)    NAME".dll"
+#  define MAKE_LIBRARY_NAMELINK(NAME)         NAME".dll"
 #  define MAKE_VALIDATION_LAYER_NAME(NAME)    NAME".dll"
 #  define LOAD_DRIVER_LIBRARY(NAME) LoadLibrary(NAME)
 #  define FREE_DRIVER_LIBRARY(LIB)  if(LIB) FreeLibrary(LIB)
diff --git a/source/loader/ze_loader.cpp b/source/loader/ze_loader.cpp
index 82e31df..6e5398a 100644
--- a/source/loader/ze_loader.cpp
+++ b/source/loader/ze_loader.cpp
@@ -11,7 +11,7 @@ namespace loader
 {
     ///////////////////////////////////////////////////////////////////////////////
     static const char* known_driver_names[] = {
-        MAKE_LIBRARY_NAME( "ze_intel_gpu", "0.4"),
+        MAKE_LIBRARY_NAMELINK( "ze_intel_gpu" ),
     };
 
     static const size_t num_known_driver_names =

Let me know if this makes sense, and if you would like a pull request.

@ravindra-ganapathi
Copy link

ravindra-ganapathi commented Mar 17, 2020

The loader's patches are behind driver's, but you should see soon the loader updated to work with level zero driver v0.8. Removing version can cause confusion while APIs are added or modified. We will improve our readme for pulling the right commit while building from source.

Thanks
Ravi

@rwmcguir
Copy link
Contributor

On Linux, the hardcoded library names include the major and minor library version, i.e.

MAKE_LIBRARY_NAME( "ze_intel_gpu", "0.4")

results in libze_intel_gpu.so.0.4.

Being hardcoded, this breaks loading the driver when its major or minor version changes (currently it is libze_intel_gpu.so.0.8).

Until a proper mechanism for enumerating the drivers is introduced, would it make sense to drop the version number from the library name also on Linux, as it is already the case on Windows ?

Both normally linked libraries and libraries that dlopen() should only use SONAME to ensure SEMVER style API/ABI compatibilty.

The SONAME currently exported from libze_intel_gpu.so contains both MAJOR and MINOR. This is a more precise variation on the SEMVER standard of only MAJOR. Since the ABI between the loader and driver has to be precisely matched, it is necessary to retain for the near term since the loader only supports a single version for the moment. Do please note this is vendor specific, should another vendor add a driver, they have their own control over versioning between loader and driver.

Additional background, today since the Level Zero specification is not at v1.0 and is pre-release, our initial implementation for SONAME is including the MINOR to avoid runtime issues that are fluxing to quickly. After v1.0 is released the SONAME will be switched for the intel-gpu driver to only using MAJOR and thus normal SEMVER will resume, hopefully very soon. So these breakages will become far less likely. Also community adoption of perhaps beyond simple SEMVER compatibility perhaps will be a thing.

If the community wants to implement backwards compatibility past normal SEMVER standards, that can be worked on, but will be complicated pending on how the Level Zero specification decides to handle their headers, functions, and structure names.

@fwyzard
Copy link
Author

fwyzard commented Mar 17, 2020

The SONAME currently exported from libze_intel_gpu.so contains both MAJOR and MINOR. This is a more precise variation on the SEMVER standard of only MAJOR. Since the ABI between the loader and driver has to be precisely matched, it is necessary to retain for the near term since the loader only supports a single version for the moment. Do please note this is vendor specific, should another vendor add a driver, they have their own control over versioning between loader and driver.

So, if I understand correctly, once v1.0 is define, a loader version v1.x should be compatible with a library version v1.y: the major version (v1) will identify the ABI compatibility, and the minor version (x vs y) will identify differences in features that do not impede such (backward? ) compatibility.

If the community wants to implement backwards compatibility past normal SEMVER standards, that can be worked on, but will be complicated pending on how the Level Zero specification decides to handle their headers, functions, and structure names.

These seem more like API changes, that would make incompatible use fail at build time rather than at run time ?

@rwmcguir
Copy link
Contributor

So, if I understand correctly, once v1.0 is define, a loader version v1.x should be compatible with a library version v1.y: the major version (v1) will identify the ABI compatibility, and the minor version (x vs y) will identify differences in features that do not impede such (backward? ) compatibility.

Correct, this is standard SEMVER: https://semver.org/
Which is the intention. v1.x+1 will work with v1.x support drivers, with perhaps a warning message if it can't find the symbol in the driver. Loader v1.x simply won't see new functions implemented by a v1.x+1 runtime library, but should still work fine.

These seem more like API changes, that would make incompatible use fail at build time rather than at run time ?
The difference I was pointing out is if the loader was asked to be compatible between v1.x and v2.x, specification then structures or similar API's may change in small or major ways with the same name. How does one #include both versions of the Level Zero Specification and not break compilation? This gets tricky, and while clever solutions are possible it may not be worth it. Opening a file with extension .so at runtime is non-deterministic for versions, so runtime may be forced start doing ABI/API checking per symbol. Not opening unversioned .so files and using SONAME alleviates the need for some or most of this. Attempting to avoid where application has to tell loader which major interface it wants and thus the logic in the loader to archive previous versions of all symbols.

While some parts of this will indeed need implemented just sticking with standard SEMVER will solve a lot of complexity and prevent bugs down the road. :-) But hey, this is community project, that is just my two cents.

@fwyzard
Copy link
Author

fwyzard commented Mar 18, 2020

I see, thanks for sharing your point of view.

@rwmcguir
Copy link
Contributor

Just FYI, the version has been updated with latest commit.
Things should align with the binaries in the libze_intel_gpu.so.0.8 now.

@fwyzard
Copy link
Author

fwyzard commented Mar 20, 2020

Thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants