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.

Sign up for GitHub

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

Prevent ICD from loading outdated amdocl binary 1.Keep track of libra… #181

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Prevent ICD from loading outdated amdocl binary 1.Keep track of libra… #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions loader/icd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,17 @@ void khrIcdVendorAdd(const char *libraryName)
goto Done;
}

// get the library's file name
const char *libName = libraryName;
const char *c;
for (c = libraryName; *c; ++c)
{
if (*c == DIRECTORY_SYMBOL)
{
libName = c + 1;
}
}
Comment on lines +85 to +91
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a non-issue, but so we go in with eyes open: As I understand this change strips all path information from the library name and only loads the first library with a given name. While this will prevent unintentional scenarios where the same library name would be loaded multiple times, it also means that library names must be unique, even if they are in different directories, or the ICD loader will filter them out. Is this the behavior we want?

I'm not aware of any implementors doing this, but a scenario I've been thinking of where two libraries could legitimately have the same name is if one device generation is supported by one software package and installed into one directory and another device generation is supported by a different package and installed into a different directory, but both device generations have the same library file name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is the behavior we want.
See my previous comment. ICD's rule on duplicate libraries has always been like this.

There is no such scenario at least for us. (Support for multiple device generations is not implemented in this way.)
Unlike loading OpenCL libraries form different vendors, loading multiple libraries from the same vendor is undefined behavior.


// ensure that we haven't already loaded this vendor
for (vendorIterator = khrIcdVendors; vendorIterator; vendorIterator = vendorIterator->next)
{
Expand All @@ -87,6 +98,11 @@ void khrIcdVendorAdd(const char *libraryName)
KHR_ICD_TRACE("already loaded vendor %s, nothing to do here\n", libraryName);
goto Done;
}
if (!strcmp(vendorIterator->libName, libName))
{
KHR_ICD_TRACE("already loaded library %s, nothing to do here\n", libName);
goto Done;
}
}

// get the library's clGetExtensionFunctionAddress pointer
Expand Down Expand Up @@ -184,6 +200,8 @@ void khrIcdVendorAdd(const char *libraryName)
KHR_ICD_TRACE("failed get platform handle to library\n");
continue;
}
vendor->libName = (char *)malloc(strlen(libName) + 1);
strcpy(vendor->libName, libName);
vendor->clGetExtensionFunctionAddress = p_clGetExtensionFunctionAddress;
vendor->platform = platforms[i];
vendor->suffix = suffix;
Expand Down Expand Up @@ -417,3 +435,15 @@ void khrIcdContextPropertiesGetPlatform(const cl_context_properties *properties,
}
}

void khrIcdFreeLibName()
{
KHRicdVendor *vendorIterator;
for (vendorIterator = khrIcdVendors; vendorIterator; vendorIterator = vendorIterator->next)
{
if (vendorIterator->libName != NULL)
{
free(vendorIterator->libName);
vendorIterator->libName = NULL;
}
}
}
6 changes: 6 additions & 0 deletions loader/icd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ struct KHRicdVendorRec
// the loaded library object (true type varies on Linux versus Windows)
void *library;

// the file name of the library
char *libName;

// the extension suffix for this platform
char *suffix;

Expand Down Expand Up @@ -160,6 +163,9 @@ void khrIcdContextPropertiesGetPlatform(
const cl_context_properties *properties,
cl_platform_id *outPlatform);

// free libName in KHRicdVendorRec struct
void khrIcdFreeLibName();

// internal tracing macros
#define KHR_ICD_TRACE(...) \
do \
Expand Down
2 changes: 2 additions & 0 deletions loader/linux/icd_linux.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ void khrIcdOsVendorsEnumerate(void)
closedir(dir);
}

khrIcdFreeLibName();

if (NULL != envPath)
{
khrIcd_free_getenv(envPath);
Expand Down
3 changes: 3 additions & 0 deletions loader/windows/icd_windows.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,9 @@ BOOL CALLBACK khrIcdOsVendorsEnumerate(PINIT_ONCE InitOnce, PVOID Parameter, PVO
{
KHR_ICD_TRACE("Failed to close platforms key %s, ignoring\n", platformsName);
}

khrIcdFreeLibName();

#if defined(CL_ENABLE_LAYERS)
khrIcdLayersEnumerateEnv();
#endif // defined(CL_ENABLE_LAYERS)
Expand Down