Skip to content

Do not depend on the HAVE_STRLCAT and HAVE_STRLCPY preprocessor definitions. #5013

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Do not depend on the HAVE_STRLCAT and HAVE_STRLCPY preprocessor definitions. #5013

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3130186
Add checks for strlcat and strlcpy symbols in CMake
bc-lee Jul 17, 2024
ffeaad9
Do not depend on the HAVE_STRLCAT and HAVE_STRLCP` preprocessor defin…
bc-lee Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,18 @@ else()
endif()
FetchContent_MakeAvailable(SwiftFoundationICU SwiftFoundation)

include(CheckSymbolExists)
include(CheckLinkerFlag)

check_symbol_exists(strlcat "string.h" HAVE_STRLCAT)
check_symbol_exists(strlcpy "string.h" HAVE_STRLCPY)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way that we can replicate this behavior in the package manifest as well so that when building via SwiftPM we get the same behavior? Or a way that we can add this check into the code itself rather than on the CMake/SwiftPM side?

Copy link
Author

Choose a reason for hiding this comment

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

Generally, no, SwiftPM doesn't have a way to add such checks or generate "config.h" files.

However, in these cases (HAVE_STRLCAT and HAVE_STRLCPY in Swift), I think we can change swift-corelibs-foundation (and other projects) to use local versions of these functions only if certain conditions are not met. Below is an example of how we can do this:

#if !TARGET_OS_MAC
#if defined(__linux__) && ((defined(__GLIBC__) && __GLIBC__ >= 2 && defined(__GLIBC_MINOR__) && __GLIBC_MINOR__ >= 38) || !defined(__GLIBC__))
// strlcat and strlcpy are available in the system
#else
// Define local versions of strlcpy and strlcat for glibc < 2.38
CF_INLINE size_t
strlcpy(char * dst, const char * src, size_t maxlen) {
  ...
}
#endif
#endif  // !TARGET_OS_MAC

This code assumes the following things:

  • Swift is supported on macOS (and other Apple platforms), Linux, and Windows.
  • Android's bionic supports strlcpy and strlcat. Link
  • Other than Android, Linux uses glibc or musl. Musl supports strlcpy and strlcat. Link
  • glibc 2.38 and later supports strlcpy and strlcat.
  • Windows doesn't have strlcpy and strlcat.

This approach is somewhat error-prone and may not be the best way to handle this, but I think this is the only way to handle this in SwiftPM. Also, I don't think supported platforms change frequently, so this approach should be fine with appropriate compile flags. Do you think this approach is acceptable?

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 this is an alright way to right this given that SwiftPM can't perform a check - assuming there's no other macro defined in the header that we can use to indicate the presence or lack of this function. @parkera had to do something similar for CURL: https://github.com/apple/swift-corelibs-foundation/blob/15ee5c32d2b808fd912c586c535feaf9af2eb2d5/Sources/_CFURLSessionInterface/include/CFURLSessionInterface.h#L39-L57

The only tweak I'd suggest - rather than doing this for !TARGET_OS_MAC, should we just only do this for TARGET_OS_LINUX (or whatever the right macro is for that)? In other words, do we need to define strlcpy for Windows/Android or does the source code in Foundation already handle not calling that function?

Copy link
Author

Choose a reason for hiding this comment

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

If you think it is a good idea to remove autoconf-style feature test preprocessor macros and check features in the code for compatibility with SwiftPM, I'll revisit this patch to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 yep since the alternative would break the SwiftPM build of this project, checking conditionally in the code is the way to go here to avoid breaking the build

if(HAVE_STRLCAT)
add_compile_definitions($<$<OR:$<COMPILE_LANGUAGE:C>,$<COMPILE_LANGUAGE:CXX>>:HAVE_STRLCAT>)
endif()
if(HAVE_STRLCPY)
add_compile_definitions($<$<OR:$<COMPILE_LANGUAGE:C>,$<COMPILE_LANGUAGE:CXX>>:HAVE_STRLCPY>)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

I think that we need to have an entry for Swift as well to pass this to the ClangImporter. Something like $<$<COMPILE_LANGUAGE:Swift>:SHELL:-Xcc -DHAVE_STRLCPY>

Copy link
Author

Choose a reason for hiding this comment

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

All changes within CMakeLists.txt were reverted in the latest commit. Please check the header file for the changes.


check_linker_flag(C "LINKER:--build-id=sha1" LINKER_SUPPORTS_BUILD_ID)

# Precompute module triple for installation
Expand Down