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

bc-lee · 2024-07-17T23:41:54Z

While checking whether the system provides the strlcat and strlcpy
functions, using an autoconf-style check to determine their availability
does not seem to be a good idea. This is because the code is also
used with SwiftPM, which does not have a similar feature test.
Instead, we should directly check the availability of these functions
in the system.

Given that strlcpy and strlcat are not standard C functions,
we should check whether the system is macOS, Linux with Bionic or
Musl libc, or glibc version 2.38 or later. This patch adds the necessary
checks in the header file so that SwiftPM can use these checks as well.

Fixes #5012

These checks are required because glibc 2.38+ added strlcat and strlcpy functions. swift-corelibs-foundation should use these functions if they are available. The checks originally existed in commit 8d1a679, but were not added to the new CMakeLists.txt in commit c772413. Fixes swiftlang#5012

jmschonfeld · 2024-07-18T17:13:02Z

CMakeLists.txt

 include(CheckLinkerFlag)

+check_symbol_exists(strlcat "string.h" HAVE_STRLCAT)
+check_symbol_exists(strlcpy "string.h" HAVE_STRLCPY)


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?

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?

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?

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.

👍 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

compnerd

This definitely seems like a good improvement once we add the Swift case as well.

compnerd · 2024-07-18T20:54:56Z

CMakeLists.txt

+endif()
+if(HAVE_STRLCPY)
+    add_compile_definitions($<$<OR:$<COMPILE_LANGUAGE:C>,$<COMPILE_LANGUAGE:CXX>>:HAVE_STRLCPY>)
+endif()


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>

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

…itions. While checking whether the system provides the `strlcat` and `strlcpy` functions, using an autoconf-style check to determine their availability does not seem to be a good idea. This is because the code is also used with SwiftPM, which does not have a similar feature test. Instead, we should directly check the availability of these functions in the system. Given that `strlcpy` and `strlcat` are not standard C functions, we should check whether the system is macOS, Linux with Bionic or Musl libc, or glibc version 2.38 or later. This patch adds the necessary checks in the header file so that SwiftPM can use these checks as well. Fixes swiftlang#5012

parkera · 2024-07-19T03:39:52Z

@swift-ci test

parkera · 2024-07-19T03:40:23Z

Sources/CoreFoundation/internalInclude/CoreFoundation_Prefix.h

-#if !TARGET_OS_MAC
-#if !HAVE_STRLCPY
+#if TARGET_OS_MAC ||                                                           \
+    (TARGET_OS_LINUX &&                                                        \


Is this going to be a problem for platforms which are not Linux but do have strlcat (FreeBSD, e.g.)?

(maybe not depending on the definition of __GLIBC__ but I'm unsure of where that's set)

That's the weakness of this approach, and that's also why every project has a list of supporting platforms. From what I know, Swift isn't officially supported on BSD variants (obviously, except for Darwin). So, I think it's not a problem. I don't even have a FreeBSD machine to test it. But if it is absolutely necessary, I may add another check for BSD variants based on https://sourceforge.net/p/predef/wiki/OperatingSystems/ and other sources, but honestly, I don't think it's necessary.

FreeBSD does have a port as does Haiku, PS4, etc. I think that the target os macros do not work even on Linux. Does this cover uclibc and newlib? There is a plethora of libcs, and the fact that this "breaks" on SPM is fine - you can mirror this into SPM as:

cSettings: [ .define("HAVE_STRLCAT", .when(platforms: [.linux]))), .define("HAVE_STRLCPY", .when(platforms: [.linux]))), ]

@jmschonfeld - I think that this is a sufficient means of mapping this in SPM, which will need to be changed if your environment is different as SPM does not have the necessary flexibility.

What if this (swift-corelibs-foundation) library is used as a transitive dependency in a Swift Package Manager project? Since SPM can't replace dependencies on the consumer side, it is extremely annoying to patch existing projects, as all dependent projects need to be patched as well. If we had a similar mechanism like go mod replace or npm's override mechanism, it would be much easier to handle this kind of situation, but that is a different topic.

You would need to update the dependency. Fortunately, swift-corelibs-foundation cannot be used as a package dependency and is shipped as part of the toolchain.

It seems that the annoyance of replacing transitive dependencies is not a problem for the swift-corelibs-foundation repository. However, I'm still unsure about defining HAVE_STRLCAT and HAVE_STRLCPY in the SwiftPM manifest file. Even though it's the same OS (Linux), the actual availability of these functions can vary. I think it would be better to define these macros in the source code, even though it might be a bit messy.

I also wish Swift had clearer documentation on which platforms are supported and which are not, so we can avoid confusions with platforms like FreeBSD. I have made a post on the Swift forums to clarify this: https://forums.swift.org/t/clarifying-supported-platforms-for-swift/73309

jmschonfeld reviewed Jul 18, 2024

View reviewed changes

compnerd reviewed Jul 18, 2024

View reviewed changes

bc-lee changed the title ~~Add checks for strlcat and strlcpy symbols in CMake~~ Do not depend on the HAVE_STRLCAT and HAVE_STRLCP` preprocessor definitions. Jul 19, 2024

bc-lee requested review from jmschonfeld and compnerd July 19, 2024 01:38

parkera reviewed Jul 19, 2024

View reviewed changes

bc-lee changed the title ~~Do not depend on the HAVE_STRLCAT and HAVE_STRLCP` preprocessor definitions.~~ Do not depend on the HAVE_STRLCAT and HAVE_STRLCPY preprocessor definitions. Jul 19, 2024

jmschonfeld mentioned this pull request Aug 8, 2024

Fixing post-swift-foundation corelibs-foundation static SDK builds #5060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bc-lee commented Jul 17, 2024 •

edited

Loading

jmschonfeld Jul 18, 2024

bc-lee Jul 18, 2024

jmschonfeld Jul 18, 2024

bc-lee Jul 18, 2024

jmschonfeld Jul 18, 2024

compnerd left a comment

compnerd Jul 18, 2024

bc-lee Jul 19, 2024

parkera commented Jul 19, 2024

parkera Jul 19, 2024

parkera Jul 19, 2024

bc-lee Jul 19, 2024

compnerd Jul 19, 2024

bc-lee Jul 19, 2024

compnerd Jul 19, 2024

bc-lee Jul 21, 2024

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

Are you sure you want to change the base?

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

Conversation

bc-lee commented Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkera commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bc-lee commented Jul 17, 2024 •

edited

Loading