Using LoadLibraryEx and LOAD_LIBRARY_SEARCH_* flag for loading DLLs o…#37763
Using LoadLibraryEx and LOAD_LIBRARY_SEARCH_* flag for loading DLLs o…#37763peterjc123 wants to merge 4 commits intopytorch:masterfrom
Conversation
💊 CI failures summary and remediationsAs of commit 38e15cc (more details on the Dr. CI page): ✅ None of the CI failures appear to be your fault 💚
🚧 1 ongoing upstream failure:These were probably caused by upstream breakages that are not fixed yet:
This 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. This comment has been revised 20 times. |
|
Maybe @malfet is more opinionated here, but the proposed new behavior seems reasonable: I'm willing to give it a try. I hate that there's not really a "standard" in Windows for this sort of thing; it seems like everyone does something different :> |
There was a problem hiding this comment.
To be super careful here and support older versions of windows, why not check that kernel32 actually has the method
| else: | |
| elif hasattr(kernel32, 'AddDllDirectory'): |
There was a problem hiding this comment.
This one sounds reasonable.
Well, I guess it is because none of the current plans could be considered perfect. |
f6473a8 to
18211f1
Compare
d8ebe32 to
38e15cc
Compare
|
Looks good to me! |
|
Verified by the tests in https://github.com/peterjc123/pytorch_dll_load_smoketests. Marking it as ready. |
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.
|
This PR were reverted, as it caused TestTypeHints failure, see for example https://app.circleci.com/pipelines/github/pytorch/pytorch/167977/workflows/2cf6c758-57a3-4a45-a465-01dfd7832a31/jobs/5432835/tests |
pytorch#37763) Summary: …n Windows Without this PR, the OS try to find the DLL in the following directories. - The directory from which the application loaded. - The system directory. Use the GetSystemDirectory function to get the path of this directory. - The 16-bit system directory. There is no function that obtains the path of this directory, but it is searched. - The Windows directory. Use the GetWindowsDirectory function to get the path of this directory. - The current directory. - The directories that are listed in the PATH environment variable. Note that this does not include the per-application path specified by the App Paths registry key. The App Paths key is not used when computing the DLL search path. If we use LoadLibraryEx with LOAD_LIBRARY_SEARCH_* flags, the directories are searched in the following order. - The directory that contains the DLL (LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR). This directory is searched only for dependencies of the DLL to be loaded. - The application directory (LOAD_LIBRARY_SEARCH_APPLICATION_DIR). - Paths explicitly added to the application search path with the AddDllDirectory function (LOAD_LIBRARY_SEARCH_USER_DIRS) or the SetDllDirectory function. If more than one path has been added, the order in which the paths are searched is unspecified. - The System32 directory (LOAD_LIBRARY_SEARCH_SYSTEM32). Advantages: 1. The directory that contains the DLL comes first and it's desirable for us, because the dependencies in `lib` should always be preferred. 2. The system directory is considered in the last place. According to some of the bug reports, the DLL load failure are caused by loading the conflicting ones in systemroot. Neural: 1. The directories in `PATH` are not considered. Similar things happen as described in the previous point. So it may be beneficial for normal users. However, it may cause failures if there are some new dependencies if built from source. (Resolved by making the fallback to `LoadLibraryW` if error code is `126`) Disadvantages: 1. LoadLibraryEx with LOAD_LIBRARY_SEARCH_* flags is only available for Win7/2008 R2 + KB2533623 and up. (Resolved by making the fallback to `LoadLibraryW` if it is not supported) 2. Failure during the call of `LoadLibraryEx` will lead to the OS to pop up a modal dialog, which can block the process if user is using a CLI-only interface. This can be switched off by calling `SetErrorMode`. (Resolved by calling `SetErrorMode`) Pull Request resolved: pytorch#37763 Test Plan: Test some common cases (in a new repo maybe) including 1. Python 3.6/3.7/3.8, conda python, conda install 2. Python 3.6/3.7/3.8, conda python, pip install 3. Python 3.6/3.7/3.8, official python, pip install Plus some corner cases like 1. Conflicting DLLs in systemroot or `PATH` 2. Remove some local dependencies and use global ones References: 1. https://docs.microsoft.com/en-us/windows/win32/api/errhandlingapi/nf-errhandlingapi-seterrormode 2. https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexa 3. https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications What do you think, malfet ezyang ? Differential Revision: D21496081 Pulled By: malfet fbshipit-source-id: aa5e528e5134326b00ac98982f4db4b4bbb47a44
…n Windows
Without this PR, the OS try to find the DLL in the following directories.
If we use LoadLibraryEx with LOAD_LIBRARY_SEARCH_* flags, the directories are searched in the following order.
Advantages:
libshould always be preferred.Neural:
PATHare not considered. Similar things happen as described in the previous point. So it may be beneficial for normal users. However, it may cause failures if there are some new dependencies if built from source. (Resolved by making the fallback toLoadLibraryWif error code is126)Disadvantages:
LoadLibraryWif it is not supported)LoadLibraryExwill lead to the OS to pop up a modal dialog, which can block the process if user is using a CLI-only interface. This can be switched off by callingSetErrorMode. (Resolved by callingSetErrorMode)Test plan:
Test some common cases (in a new repo maybe) including
Plus some corner cases like
PATHReferences:
What do you think, @malfet @ezyang ?