Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use CAFFE2_USE_MSVC_STATIC_RUNTIME to determine when to avoid waiting for global destructors on Windows #46014

Conversation

adampauls
Copy link

Cherry-pick of #43532 onto release/1.7.

Summary:
We are trying to build libtorch statically (BUILD_SHARED_LIBS=OFF) then link it into a DLL. Our setup hits the infinite loop mentioned here because we build with BUILD_SHARED_LIBS=OFF but still link it all into a DLL at the end of the day.

This PR fixes the issue by changing the condition to guard on which windows runtime the build links against using the CAFFE2_USE_MSVC_STATIC_RUNTIME flag. CAFFE2_USE_MSVC_STATIC_RUNTIME defaults to ON when BUILD_SHARED_LIBS=OFF, so backwards compatibility is maintained.

I'm not entirely confident I understand the subtleties of the windows runtime versus linking setup, but this setup works for us and should not affect the existing builds.

Fixes #44470

Pull Request resolved: #43532

Reviewed By: mrshenli

Differential Revision: D24053767

Pulled By: albanD

fbshipit-source-id: 1127fefe5104d302a4fc083106d4e9f48e50add8

Fixes #{issue number}

… for global destructors on Windows (pytorch#43532)

Summary:
We are trying to build libtorch statically (BUILD_SHARED_LIBS=OFF) then link it into a DLL. Our setup hits the infinite loop mentioned [here](https://github.com/pytorch/pytorch/blob/54c05fa34e1fbbb5096746a8ae92e27a08116de4/torch/csrc/autograd/engine.cpp#L228) because we build with `BUILD_SHARED_LIBS=OFF` but still link it all into a DLL at the end of the day.

This PR fixes the issue by changing the condition to guard on which windows runtime the build links against using the `CAFFE2_USE_MSVC_STATIC_RUNTIME` flag. `CAFFE2_USE_MSVC_STATIC_RUNTIME` defaults to ON when `BUILD_SHARED_LIBS=OFF`, so backwards compatibility is maintained.

I'm not entirely confident I understand the subtleties of the windows runtime versus linking setup, but this setup works for us and should not affect the existing builds.

Fixes pytorch#44470

Pull Request resolved: pytorch#43532

Reviewed By: mrshenli

Differential Revision: D24053767

Pulled By: albanD

fbshipit-source-id: 1127fefe5104d302a4fc083106d4e9f48e50add8
@mruberry mruberry requested a review from malfet October 8, 2020 21:31
@mruberry mruberry added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 8, 2020
@mruberry mruberry removed the request for review from albanD October 8, 2020 21:31
@mruberry
Copy link
Collaborator

mruberry commented Oct 8, 2020

@malfet, are you the correct person to review this?

@malfet
Copy link
Contributor

malfet commented Oct 8, 2020

@mruberry, I am, although this is just a cherry-pick from master.

@adampauls
Copy link
Author

Just double-checking all is well here? It seems most branches in the 1.7.0 issue tracker mentioned after this one have been merged?

@malfet
Copy link
Contributor

malfet commented Oct 14, 2020

@adampauls Looking at the code, it seems that this change disabled this codepath on Windows for both default static library build, which can lead to crashes during application shutdown is library is linked to an executable.
Please add a rule to CMakeLists.txt to default it to !C10_BUILD_SHARED_LIBS and then it should be good to go.

@adampauls
Copy link
Author

I think that's already accomplished by

pytorch/CMakeLists.txt

Lines 135 to 137 in b1d24dd

cmake_dependent_option(
CAFFE2_USE_MSVC_STATIC_RUNTIME "Using MSVC static runtime libraries" ON
"NOT BUILD_SHARED_LIBS" OFF)
, or am I misunderstanding?

@adampauls
Copy link
Author

Friendly ping, I noticed this missed the RC2 train.

@adampauls
Copy link
Author

@malfet friendly ping, seems this missed RC3 too.

@adampauls
Copy link
Author

@malfet should I assume this won't make it in for 1.7 then?

@adampauls adampauls closed this Oct 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

4 participants