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

Modification for making Foonathan memory compatible with UWP #80

Merged
merged 5 commits into from
Jun 23, 2020

Conversation

quattrinili
Copy link
Contributor

@quattrinili quattrinili commented May 28, 2020

Modifications to VirtualAlloc for Windows compilation, so that it is compatible with UWP. In particular: VirtualAlloc was changed to VirtualAllocFromApp so that it is compatible for both Windows Desktop and UWP (https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualallocfromapp). In addition, in the CMakeLists.txt, -static option was changed to /wholearchive as -static does not work anymore (please see https://docs.microsoft.com/en-us/cpp/build/reference/linker-options?view=vs-2019).

This has been tested on both current Visual Studio 2017 and 2019.

@foonathan
Copy link
Owner

Thank you. However, it seems that it breaks CI which tests it on older version of windows. Is there some check for UWP so you can switch to VirtualAllocFromApp conditionally?

@quattrinili
Copy link
Contributor Author

It looks the checks provided here should work

https://docs.microsoft.com/en-us/cpp/porting/how-to-use-existing-cpp-code-in-a-universal-windows-platform-app?redirectedfrom=MSDN&view=vs-2019

I updated my repo and it seems that this time it didn't break CI. However, I need to test it again on whether a UWP compilation works. I will confirm it as soon as I can.

As UWP app would just link to this program and given that the library depends on the build tools, the check on whether to use VirtualAlloc vs. VirtualAllocFromApp is on the Visual Studio compiler version (https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=vs-2019).
@quattrinili
Copy link
Contributor Author

Ok, I completed the check and actually using the macros I mentioned last time wouldn't work. So the easiest way to deal with it is just to check the version of the VS compiler. Now that works also when the library is linked to a UWP app. So, given that it passes also the CI, I think this pull request can be merged. Thanks!

@foonathan foonathan merged commit 44fc98e into foonathan:master Jun 23, 2020
@foonathan
Copy link
Owner

Sorry, missed that you fixed this. Thanks!

@@ -7,7 +7,7 @@
add_executable(foonathan_memory_node_size_debugger test_types.hpp node_size_debugger.hpp node_size_debugger.cpp)
if (CMAKE_CROSSCOMPILING)
# statically link when cross compiling so emulator doesn't need library paths
set_target_properties(foonathan_memory_node_size_debugger PROPERTIES LINK_FLAGS "-static")
set_target_properties(foonathan_memory_node_size_debugger PROPERTIES LINK_FLAGS "/WHOLEARCHIVE")

Choose a reason for hiding this comment

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

This breaks cross-compiling in Linux

Copy link
Owner

Choose a reason for hiding this comment

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

Should be fixed by the commit I've referenced below, please check and open an issue if it doesn't work.

foonathan added a commit that referenced this pull request Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants