Improve Dynamic Library for Windows #40365
Conversation
peterjc123
commented
Jun 22, 2020
peterjc123
commented
- Use LoadLibraryEx if available
- Print more info on error
💊 CI failures summary and remediationsAs of commit a166018 (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 23 times. |
peterjc123
force-pushed
the
dynamic_library_win_fix
branch
from
60f371e to
da24813
Compare
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
Do you know if we actually have test coverage on Windows for this?
|
Test failure is legit: (answer: yes we do!) |
Fixed. Update: It doesn't. Because we are relying on PATH. Change to reverting back to the default behavior (LoadLibrary if error code 126). |
|
no dice? |
LOL. I've made a stupid mistake. |
| HMODULE theModule; | ||
| bool reload = true; | ||
| if (GetProcAddress(GetModuleHandle("KERNEL32.DLL"), "AddDllDirectory") != NULL) { | ||
| theModule = LoadLibraryExA( | ||
| name, | ||
| NULL, | ||
| LOAD_LIBRARY_SEARCH_DEFAULT_DIRS); | ||
| if (theModule != NULL || (GetLastError() != ERROR_MOD_NOT_FOUND)) { | ||
| reload = false; | ||
| } | ||
| } | ||
|
|
||
| if (reload) { | ||
| theModule = LoadLibraryA(name); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why theModule != NULL is not a sufficient check? Moreover, why simply not do something like:
theModule = has_LOAD_LIBRARY_SEARCH_DEFAULT_DIRS() ? LoadLibraryEx(...) : LoadLibrary(...);
| HMODULE theModule; | |
| bool reload = true; | |
| if (GetProcAddress(GetModuleHandle("KERNEL32.DLL"), "AddDllDirectory") != NULL) { | |
| theModule = LoadLibraryExA( | |
| name, | |
| NULL, | |
| LOAD_LIBRARY_SEARCH_DEFAULT_DIRS); | |
| if (theModule != NULL || (GetLastError() != ERROR_MOD_NOT_FOUND)) { | |
| reload = false; | |
| } | |
| } | |
| if (reload) { | |
| theModule = LoadLibraryA(name); | |
| } | |
| HMODULE theModule = NULL; | |
| if (GetProcAddress(GetModuleHandle("KERNEL32.DLL"), "AddDllDirectory") != NULL) { | |
| theModule = LoadLibraryExA( | |
| name, | |
| NULL, | |
| LOAD_LIBRARY_SEARCH_DEFAULT_DIRS); | |
| } | |
| if (theModule == NULL) { | |
| theModule = LoadLibraryA(name); | |
| } | |
There was a problem hiding this comment.
Because the main differences between LoadLibrary and LoadLibraryEx is that the search order and the directories being searched. So only when we get ERROR_MOD_NOT_FOUND, it is meaningful to try LoadLibrary afterwards.
There was a problem hiding this comment.
I still can't imagine a valid PyTorch installation, where LoadLibraryEx would fail but LoadLibrary will succeed.
Alternatively, if we will fall-back to LoadLibrary if LoadLibraryEx would fail, shouldn't we always call LoadLibrary (because, it would not ignore paths specified via AddDllDirectory, would it?)
Or, why not do something like that:
DWORD loadFlags = 0;
if (hasAddDllDirectory) loadFlags = LOAD_LIBRARY_SEARCH_DEFAULT_DIRS;
theModule = loadLibraryExA(name, NULL, loadFlags);
There was a problem hiding this comment.
I still can't imagine a valid PyTorch installation, where
LoadLibraryExwould fail butLoadLibrarywill succeed.
LoadLibraryExA with LOAD_LIBRARY_SEARCH_DEFAULT_DIRS doesn't considers directories in PATH.
Alternatively, if we will fall-back to
LoadLibraryifLoadLibraryExwould fail, shouldn't we always callLoadLibrary(because, it would not ignore paths specified viaAddDllDirectory, would it?)
Yes, it is partially the reason. Also, the search order is more reasonable because SYSTEM dir is searched in the last step.
Co-authored-by: Nikita Shulga <nikita.shulga@gmail.com>
zou3519
added
the
triaged
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@malfet is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@malfet is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: 1. Use LoadLibraryEx if available 2. Print more info on error Pull Request resolved: pytorch#40365 Differential Revision: D22194974 Pulled By: malfet fbshipit-source-id: e8309f39d78fd4681de5aa032288882910dff928 (cherry picked from commit a2d4d9e)
7 participants
