Skip to content

fix: handle missing APIs on every static loader call - #555

Open
againull wants to merge 1 commit into
oneapi-src:masterfrom
againull:fix/loader-missing-api
Open

againull wants to merge 1 commit into
oneapi-src:masterfrom
againull:fix/loader-missing-api

Conversation

@againull

Copy link
Copy Markdown
Contributor

The static API wrappers cache function pointers, but only set a local error status during the first cache initialization. If an API is absent from the selected shared loader, the first call returns unsupported and subsequent calls invoke the cached null pointer causing a crash.

Check the cached pointer on each invocation instead of capturing a per-call result in its initializer. Regenerate the core, Sysman, Tools, and runtime wrappers from the updated template.

The static API wrappers cache function pointers, but only set a local error status during the first cache initialization. If an API is absent from the selected shared loader, the first call returns unsupported and subsequent calls invoke the cached null pointer.

Check the cached pointer on each invocation instead of capturing a per-call result in its initializer. Regenerate the core, Sysman, Tools, and runtime wrappers from the updated template.

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>
@github-actions

Copy link
Copy Markdown

Level Zero loader ABI check

last-release (v1.35.0): ✅ no ABI problems. Report

Creating compatibility report ...
Binary compatibility: 100%
Source compatibility: 100%
Total binary compatibility problems: 0, warnings: 0
Total source compatibility problems: 0, warnings: 0

master (master): ✅ no ABI problems. Report

Creating compatibility report ...
Binary compatibility: 100%
Source compatibility: 100%
Total binary compatibility problems: 0, warnings: 0
Total source compatibility problems: 0, warnings: 0

Workflow run

If an ABI change is intended, a reviewer with write access can accept it: leave an approval review on the PR head commit with the keyword abi-failure-approved in the review body. That re-runs the check.

@rwmcguir

Copy link
Copy Markdown
Contributor

The concept seems solid and I am not seeing additional cost in normal good calls, so this is acceptable..

Any chance you may be able to add a unit test for this into the ./test/ ctests?

@rwmcguir

Copy link
Copy Markdown
Contributor

Here is a suggested unit test for this fix. It nulls one entry in the static DDI table after zeInitDrivers to emulate a shared loader that lacks the API, then calls it twice.

Verified locally (static build):

  • master: segfault on the second call
  • this PR: passes
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 6559267..9dcb3b8 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -1051,6 +1051,11 @@ else()
   set_property(TEST init_driver_unit_tests PROPERTY ENVIRONMENT "ZE_ENABLE_LOADER_DEBUG_TRACE=1;ZE_ENABLE_NULL_DRIVER=1;")
 endif()
 
+if(BUILD_STATIC)
+  add_test(NAME tests_static_missing_api_called_twice COMMAND tests --gtest_filter=*StaticLoaderMissingApi*GivenApiMissingFromSharedLoaderWhenCallingApiTwiceThenUnsupportedFeatureIsReturnedBothTimes*)
+  set_property(TEST tests_static_missing_api_called_twice PROPERTY ENVIRONMENT "ZE_ENABLE_LOADER_DEBUG_TRACE=1;ZE_ENABLE_NULL_DRIVER=1")
+endif()
+
 
 
 # These tests are currently not supported on Windows. The reason is that the std::cerr is not being redirected to a pipe in Windows to be then checked against the expected output.
diff --git a/test/loader_api.cpp b/test/loader_api.cpp
index c04c41b..b51e232 100644
--- a/test/loader_api.cpp
+++ b/test/loader_api.cpp
@@ -4059,4 +4059,33 @@ TEST_F(DriverOrderingTest,
     EXPECT_EQ(0, strcmp(errorDesc, "ERROR UNSUPPORTED FEATURE"));
   }
 
+#ifdef L0_STATIC_LOADER_BUILD
+  // The static loader caches each API's function pointer on first call. When the
+  // shared loader does not provide an API, every call must keep returning
+  // ZE_RESULT_ERROR_UNSUPPORTED_FEATURE instead of calling the cached nullptr.
+  // Must run in its own process so zeContextSystemBarrier is not already cached.
+  TEST(
+      StaticLoaderMissingApi,
+      GivenApiMissingFromSharedLoaderWhenCallingApiTwiceThenUnsupportedFeatureIsReturnedBothTimes)
+  {
+    uint32_t pInitDriversCount = 0;
+    ze_init_driver_type_desc_t desc = {ZE_STRUCTURE_TYPE_INIT_DRIVER_TYPE_DESC};
+    desc.flags = UINT32_MAX;
+    desc.pNext = nullptr;
+    EXPECT_EQ(ZE_RESULT_SUCCESS, zeInitDrivers(&pInitDriversCount, nullptr, &desc));
+    EXPECT_GT(pInitDriversCount, 0);
+
+    // Emulate an older shared loader that does not export this API.
+    ze_dditable_t *ddiTable = ze_lib::context->zeDdiTable.load();
+    ASSERT_NE(nullptr, ddiTable);
+    auto savedPfn = ddiTable->Context.pfnSystemBarrier;
+    ddiTable->Context.pfnSystemBarrier = nullptr;
+
+    EXPECT_EQ(ZE_RESULT_ERROR_UNSUPPORTED_FEATURE, zeContextSystemBarrier(nullptr, nullptr));
+    EXPECT_EQ(ZE_RESULT_ERROR_UNSUPPORTED_FEATURE, zeContextSystemBarrier(nullptr, nullptr));
+
+    ddiTable->Context.pfnSystemBarrier = savedPfn;
+  }
+#endif // L0_STATIC_LOADER_BUILD
+
 } // namespace

This branch has not been deployed

No deployments
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

Successfully merging this pull request may close these issues.

2 participants