Skip manifests already found when scanning search paths - #2033
Wint3rNight wants to merge 1 commit into
Conversation
Duplicate elimination runs on the search path strings, so it cannot catch the case where one path names a directory and another names a file inside that directory. The two strings differ, but the directory scan and the explicit file path resolve to the same manifest, so the driver is loaded twice and its physical devices are reported twice. Both call sites in add_data_files resolve to a full path before reaching add_if_manifest_file, so comparing the resolved name is enough to spot the repeat. The duplication is only visible with Vulkan 1.1 or newer, because GPDP2 gates the Linux physical device sort, which is why the existing tests at 1.0 did not catch it.
|
Author Wint3rNight not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author Wint3rNight not on autobuild list. Waiting for curator authorization before starting CI build. |
|
CI Vulkan-Loader build queued with queue ID 119687. |
|
CI Vulkan-Loader build # 3766 running. |
|
CI Vulkan-Loader build # 3766 passed. |
charles-lunarg
left a comment
charles-lunarg
left a comment
There was a problem hiding this comment.
This change doesn't modify prepend_if_manifest_file() which also adds to the list, so for paths added by the loader settings file, duplicates would sail on through. That function will be more involved because 'prepending' should take precedence - so if there is a duplicate, we should still prepend while also removing the duplicate.
While I understand marking the issue as 'fixed' is to link it with #1874, I am going to remove it because the issue isn't solely about fixing duplicates, but expanding testing to cover many more cases where those duplicates may occur (due to GPDP2).
| // Skip a manifest that is already in the list. The same file is reached twice whenever a search path names a | ||
| // directory and another names a file inside it, which duplicate elimination on the search paths cannot catch | ||
| // because those two strings differ. Both call sites resolve to a full path before getting here, so comparing | ||
| // the resolved name is enough to spot it. |
There was a problem hiding this comment.
This comment is way to long for what it is.
It also has an un-truth. While loader_get_fullpath() does attempt to create (and check) the paths given to it, it doesn't try to validate already existing paths. Which to be clear is likely an issue in its own right but not important. By claiming paths 'resolve to a full path before getting here', it tries to claim a truth about something that isn't.
3 participants
Duplicate elimination runs on the search path strings, so it does not catch the case where one path names a directory and another names a file inside that directory. The two strings differ, but the directory scan and the explicit file path resolve to the same manifest, so the driver is loaded twice and its physical devices are reported twice.
With
VK_DRIVER_FILESset to a folder plus two manifests inside that folder,vkEnumeratePhysicalDevicesreports 9 devices where 5 exist, matching the log in the issue:Both call sites in
add_data_filesresolve to a full path before callingadd_if_manifest_file, so comparing the resolved name catches the repeat. The existingcopy_str_to_string_list_if_uniquehelper already does exactly this.This is only visible with Vulkan 1.1 or newer, because GPDP2 gates the Linux physical device sort. The existing env var tests run at 1.0, which is why CI never caught it.
Added
EnvVarICDOverrideSetup.DirAndFileOverlapDoesNotDuplicate, which fails on main with 9 devices and passes with the fix. Full suite is 703/703.Two notes:
add_data_filesalso serves layer discovery, so layers get the same protection. No existing test changes behaviour.loader_get_fullpathdoes not canonicalise symlinks or..segments, so two different spellings of the same file are still not detected. That is outside what this issue reports and I have left it alone.The issue also suggests running these env var cases at both 1.0 and 1.1, since much loader logic depends on GPDP2. That is a broader testing change and I have kept it out of this PR, but happy to follow up if useful.