Allow DlLoader to have multiple fallback names when searching for a library#1635
Allow DlLoader to have multiple fallback names when searching for a library#1635qining merged 3 commits intogoogle:masterfrom
Conversation
AWoloszyn
left a comment
AWoloszyn
left a comment
There was a problem hiding this comment.
Looks ok to me, but I will leave it to @ben-clayton to do the final review, in case he was any other opinions.
ben-clayton
left a comment
ben-clayton
left a comment
There was a problem hiding this comment.
What is this actually trying to fix?
Please can we have a "why" in the CL description?
core/cc/dl_loader.cpp
Outdated
| size_t len = strlen(name) + sizeof(".1"); | ||
| char* fallback_name = new char[len]; | ||
| strcat(fallback_name, ".1"); | ||
| return dlopen(name, RTLD_NOW | RTLD_LOCAL); |
There was a problem hiding this comment.
... and don't you mean fallback_name?
core/cc/dl_loader.cpp
Outdated
| // Try name + ".1" | ||
| size_t len = strlen(name) + sizeof(".1"); | ||
| char* fallback_name = new char[len]; | ||
| strcat(fallback_name, ".1"); |
There was a problem hiding this comment.
- You need to clear the memory.
- You need to actually copy in name.
- You need to free the memory.
- You're better of using
std::stringstream
ben-clayton
left a comment
ben-clayton
left a comment
There was a problem hiding this comment.
Perhaps it would be better to add the fallback to the places that attempt to load libvulkan.so - at least then we're loading a version we're confident should be compatible?
|
Note: we already do this for |
|
But the situation for Vulkan is different to libGL.so. At least on my machine, The current |
|
Or do you mean we should always only searching for a specific version of Vulkan? |
qining
force-pushed
the
search-libvulkan-so-1-also
branch
from
36623a3 to
bf661a1
Compare
|
Changed to just loading |
|
I think it's working on Android because Android uses core/cc/android/get_vulkan_proc_address.cpp which still has libvulkan.so, which is good, since I don't have libvulkan.so.1 on my Android O Nexus 5X. |
|
I think we can't be 100% sure that all Linux drivers have libvulkan.so.1. I prefer the version that tries libvulkan.so and if it is not found looks for libvulkan.so.1. |
|
Also, you probably need to change HasVulkanLoader as well. |
qining
force-pushed
the
search-libvulkan-so-1-also
branch
from
bf661a1 to
92887c6
Compare
|
@y-novikov 5ae192f should have the support to searching across multiple names in order and |
|
Thank you, @qining, looks good. You probably should also update the description to reference issue 1634. |
qining
changed the title
core/cc/dl_loader.cpp
Outdated
| DL(CC _1, CC _2, CC _3); | ||
| DL(CC _1, CC _2, CC _3, CC _4); | ||
| DL(CC _1, CC _2, CC _3, CC _4, CC _5); | ||
| #undef DLLOADER |
| // (if any). Names will be used to try to find the library in order. If the | ||
| // library cannot be loaded then this is a fatal error. For *nix systems, | ||
| // a nullptr can be used to search the application's functions. | ||
| template<typename... ConstCharPtrs> |
There was a problem hiding this comment.
Is there a particular reason we are doing this with template magic, rather than something like std::initializer_list?
There was a problem hiding this comment.
hmm, I just feel DlLoader("libvulkan.so") looks a little bit better than DlLoader({"libvulkan.so"}). But yes, initializer list can do the same and does not need explicit instantiation...Don't have other particular reasons.
There was a problem hiding this comment.
Ok, as much as I personally love templates, reading
void* load() {
return nullptr;
}
template<typename... ConstCharPtrs>
void* load(const char* name, ConstCharPtrs... fallback_names) {
....
...
return load(fallback_names...);
...
...
Bunch of macros that will cause a link error if we hit 6 arguments.
...
is a lot harder to parse than
void* load(std::initializer_list<const char* name> names) {
for (auto& nm: names) {
}
4 participants
Fix #1634