Prevent ICD from loading outdated amdocl binary 1.Keep track of libra… #181
Conversation
|
|
There was a problem hiding this comment.
This is a very impactful change, and I think it will need to be discussed. Especially, I don't know how many users could be loading several version of the same OpenCL implementation for legitimate uses.
Also I am not a Windows expert, so I can't really comment on the specifics. But if other vendors will encounter the same issue, they should be part of this discussion.
…ry name in KHRicdVendorRec 2.Check if library with the same name has already been loaded before adding it in ICD list
lttamd
force-pushed
the
outdated-lib-check
branch
from
2420c66 to
1bea2c2
Compare
As far as I understand, it is wrong to load multiple versions of the same OpenCL implementation from the same vendor. (unlike loading OpenCL implementations from different vendors) Also, even before this bug showing up as result of new u-inf installation using the new registry location, I don't think ICD ever allowed "multiple versions of the same OpenCL implementation" to be loaded at the same time. Therefore, I do not think this is an impactful change, but a merely a bug fix following existing ICD loading rule. |
| for (c = libraryName; *c; ++c) | ||
| { | ||
| if (*c == DIRECTORY_SYMBOL) | ||
| { | ||
| libName = c + 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Do we have any update on this? |
|
As per my action item from the December 3rd teleconference, here were a few prior teleconferences where we discussed this issue:
For next steps, perhaps we can converge on solution requirements? Briefly, in prior discussions proposed requirements were:
Is this correct? If so, are some of these requirements more important than others and hence require a short-term solution, whereas others could be solved longer-term? |
Prevent ICD from loading outdated amdocl binary
1.Keep track of library name in KHRicdVendorRec
2.Check if library with the same name has already been loaded before adding it in ICD list
There are going to be two sets of amdocl in registry for ICD to load if we install a new driver on top of a previously installed old driver.
It will cause subsequent undefined behaviors such as duplicate devices.
This is because AMD driver installation switched from c-inf to u-inf which no longer uses SOFTWARE\Khronos\OpenCL\Vendors.
If a new diver is installed by either user or Windows update without manual cleanup, there will be two libraries to load according to registry.
This is not fixable from driver or installation level.
(The other vendors do not have this issue only because they haven't switched to u-inf installation yet as far as I know.)