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

Update CMakeLists.txt #734

Merged
merged 8 commits into from
Jan 23, 2025
Merged

Update CMakeLists.txt #734

merged 8 commits into from
Jan 23, 2025

Conversation

mjp41
Copy link
Member

@mjp41 mjp41 commented Jan 20, 2025

Handle platforms that have _GLIBCXX_ASSERTIONS, which require the stdlib++ to be included.

mjp41 added 2 commits January 20, 2025 19:41
Handle platforms that have `_GLIBCXX_ASSERTIONS`, which require the stdlib++ to be included.
@mjp41
Copy link
Member Author

mjp41 commented Jan 20, 2025

@jcelerier does this fix one of the two Arch Linux errors you reported?

CMakeLists.txt Outdated
@@ -95,6 +95,15 @@ if (NOT MSVC AND NOT (SNMALLOC_CLEANUP STREQUAL CXX11_DESTRUCTORS))
endif()
endif()

# FORTIFY_SOURCE prevents overriding memcpy, disable memcpy if detected.
if("${CMAKE_CXX_FLAGS}" MATCHES ".*_FORTIFY_SOURCE.*")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a safer way to try this would be to use a cmake try_compile test, as there are other ways for flags to end up in -DCMAKE_CXX_FLAGS than this:

  • CMAKE_CXX_FLAGS_
  • a host project adding the flag via add_compile_options or add_compile_definitions and then doing add_subdirectory(snmalloc)

Copy link
Contributor

Choose a reason for hiding this comment

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

example:

cmake_minimum_required(VERSION 3.31)
project(foo LANGUAGES CXX)

try_compile(no_macro 
  SOURCE_FROM_CONTENT main.cpp
[=[ 
#if defined(_FORTIFY_SOURCE)
#error
#endif
int main() { }   
]=]
)

message("Result: ${no_macro}")
``̀`

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great suggestion, I have done something similar to your suggestion.

Copy link
Contributor

@jcelerier jcelerier left a comment

Choose a reason for hiding this comment

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

LGTM!

@mjp41 mjp41 merged commit e3e5584 into main Jan 23, 2025
62 checks passed
@mjp41 mjp41 deleted the mjp41-patch-2 branch January 23, 2025 13:36
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.

2 participants