Skip to content

Using LoadLibraryEx and LOAD_LIBRARY_SEARCH_* flag for loading DLLs o…#37763

Closed
peterjc123 wants to merge 4 commits intopytorch:masterfrom
peterjc123:windows_dll_fix
Closed

Using LoadLibraryEx and LOAD_LIBRARY_SEARCH_* flag for loading DLLs o…#37763
peterjc123 wants to merge 4 commits intopytorch:masterfrom
peterjc123:windows_dll_fix

Conversation

@peterjc123
Copy link
Copy Markdown
Collaborator

@peterjc123 peterjc123 commented May 4, 2020

…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)

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
  4. Conflicting DLLs in systemroot or PATH
  5. 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 ?

@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented May 4, 2020

💊 CI failures summary and remediations

As 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.

See how this bot performed.

This comment has been revised 20 times.

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented May 4, 2020

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 :>

Comment thread torch/__init__.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To be super careful here and support older versions of windows, why not check that kernel32 actually has the method

Suggested change
else:
elif hasattr(kernel32, 'AddDllDirectory'):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This one sounds reasonable.

Comment thread torch/__init__.py Outdated
@peterjc123
Copy link
Copy Markdown
Collaborator Author

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 :>

Well, I guess it is because none of the current plans could be considered perfect.

@mruberry mruberry added module: build Build system issues triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels May 5, 2020
@peterjc123 peterjc123 linked an issue May 7, 2020 that may be closed by this pull request
@malfet
Copy link
Copy Markdown
Contributor

malfet commented May 7, 2020

Looks good to me!

@peterjc123
Copy link
Copy Markdown
Collaborator Author

Verified by the tests in https://github.com/peterjc123/pytorch_dll_load_smoketests. Marking it as ready.

@peterjc123 peterjc123 requested review from ezyang and malfet May 10, 2020 10:44
@peterjc123 peterjc123 marked this pull request as ready for review May 10, 2020 10:45
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@malfet merged this pull request in 00f3790.

@malfet
Copy link
Copy Markdown
Contributor

malfet commented May 12, 2020

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

laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: build Build system issues open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pytorch upgrade 1.4 -> 1.5 broken on Windows

6 participants