-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add a pragma comment to pull in the synchronization lib #1507
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
Conversation
|
AFAIK this only resolves the second case, where it can't find the Windows functions. I don't think that the first broken case, which can't find __std_atomic_wait_direct and friends, will be fixed. |
CaseyCarter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this change but not with the suggestion that it completely fixes the linked bug - as @sylveon commented. There's some linker funkiness - either a bug in the ARM linker or some unsupported behavior on x86 that we're taking advantage of - that still needs addressing.
That said, I'd be happy to go ahead and approve this as an improvement that partially addresses but does not close #1504.
|
|
||
| #if _ATOMIC_WAIT_ON_ADDRESS_STATICALLY_AVAILABLE | ||
|
|
||
| #pragma comment(lib, "synchronization") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we had this comment, but it was replaced with linking using CMakeList.txt.
@BillyONeal , do you remeber anything relevant to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The final binary which links against the atomic_wait.obj needs to also know to link in the synchronization.lib. It is insufficient to just link against the synchronization.lib at build time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is insufficient to just link against the
synchronization.libat build time.
Do I understand correctly that this isn't a problem for x86/x64 because _ATOMIC_WAIT_ON_ADDRESS_STATICALLY_AVAILABLE is zero because they have to support Windows 7? (I should have asked this in my review instead of simply assuming.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, _ATOMIC_WAIT_ON_ADDRESS_STATICALLY_AVAILABLE is zero on x86/x64
|
@CaseyCarter I updated the original issue with information concerning the linking issue. It appears to be a separate issue that I reached out to Jimmy to confirm. |
|
Thanks for fixing this linker error! Your fix will ship in VS 2019 16.9 Preview 3. 🚢 🐈 |
Works towards #1504.