Skip to content

Avoid unnecessary -L for implicit link dirs - #16233

Closed
dg0yt wants to merge 1 commit into
curl:masterfrom
dg0yt:patch-1
Closed

Avoid unnecessary -L for implicit link dirs#16233
dg0yt wants to merge 1 commit into
curl:masterfrom
dg0yt:patch-1

Conversation

@dg0yt

@dg0yt dg0yt commented Feb 7, 2025

Copy link
Copy Markdown
Contributor

The unnecessary -L may unexpectedly change the toolchain's chosen search order in implicit link directories.

Causing this error noticed by a vcpkg user, and reproduced, in the context of Android NDK r22b, API level 16, static library and CRT linkage:

configure:23635: checking whether libcurl is usable
configure:23669: armv7a-linux-androideabi16-clang -o conftest --sysroot=/home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot -DANDROID -DDECLARE_TIMEZONE -Wall -I/home/runner/work/boinc/boinc/3rdParty/buildCache/android/android-tc/arm/arm-linux-androideabi/include -O3 -fomit-frame-pointer -fPIE -march=armv7-a -mfloat-abi=softfp -mfpu=vfpv3-d16 -D__ANDROID_API__=16 -Wall  -I/home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/include -I/home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/lib/pkgconfig/../../include -DCURL_STATICLIB  -L/home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/lib -llog -fPIE -pie -latomic -static-libstdc++ -march=armv7-a -Wl,--fix-cortex-a8 -L/home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/lib conftest.c  -L/home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/lib/pkgconfig/../../lib -lcurl -L/home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/arm-linux-androideabi -lssl -lcrypto -ldl -pthread -lz  >&5
clang: warning: argument unused during compilation: '-static-libstdc++' [-Wunused-command-line-argument]
ld: error: undefined symbol: bsd_signal
>>> referenced by legacy_signal_inlines.h:116 (/home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/legacy_signal_inlines.h:116)
>>>               libcrypto-lib-ui_openssl.o:(read_string_inner) in archive /home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/lib/libcrypto.a

ld: error: undefined symbol: pthread_atfork
>>> referenced by libc_init_common.cpp:129 (bionic/libc/bionic/libc_init_common.cpp:129)
>>>               libc_init_common.o:(__libc_init_fork_handler()) in archive /home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/arm-linux-androideabi/libc.a
>>> referenced by guarded_pool_allocator_posix.cpp:109 (external/gwp_asan/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp:109)
>>>               guarded_pool_allocator_posix.o:(gwp_asan::GuardedPoolAllocator::installAtFork()) in archive /home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/arm-linux-androideabi/libc.a
>>> referenced by android_je_iterate.c:91 (external/jemalloc_new/src/android_je_iterate.c:91)
>>>               jemalloc.o:(je_malloc_disable_init) in archive /home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/arm-linux-androideabi/libc.a
>>> referenced 1 more times
clang: error: linker command failed with exit code 1 (use -v to see invocation)

with the unnecessary option

-L/home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/arm-linux-androideabi 

Reference: microsoft/vcpkg#43463 (comment)

@vszakats vszakats closed this in f72b848 Feb 7, 2025
@vszakats

vszakats commented Feb 7, 2025

Copy link
Copy Markdown
Member

Thanks @dg0yt, merged!

@vszakats

vszakats commented Feb 7, 2025

Copy link
Copy Markdown
Member

Ah, late, do you see anything against this shorter version?:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -2236,7 +2236,7 @@ if(NOT CURL_DISABLE_INSTALL)
   endforeach()
 
   # Avoid getting unnecessary -L options for known system directories.
-  set(_sys_libdirs "")
+  set(_sys_libdirs ${CMAKE_C_IMPLICIT_LINK_DIRECTORIES})
   foreach(_libdir IN LISTS CMAKE_SYSTEM_PREFIX_PATH)
     if(_libdir MATCHES "/$")
       string(APPEND _libdir "lib")

vszakats added a commit to vszakats/libssh2 that referenced this pull request Feb 7, 2025
vszakats added a commit to libssh2/libssh2 that referenced this pull request Feb 7, 2025
@dg0yt

dg0yt commented Feb 7, 2025

Copy link
Copy Markdown
Contributor Author
-  set(_sys_libdirs "")
+  set(_sys_libdirs ${CMAKE_C_IMPLICIT_LINK_DIRECTORIES})

CMake shenanigans: Written this way, you get inconsistent behavior depending on CMAKE_C_IMPLICIT_LINK_DIRECTORIES: When it is empty, the command becomes an unset, letting this scope see a potential parent scope value.

That's why I didn't do it that way.

On second thought, it is fine with double quotes.

-  set(_sys_libdirs "")
+  set(_sys_libdirs "${CMAKE_C_IMPLICIT_LINK_DIRECTORIES}")

@dg0yt
dg0yt deleted the patch-1 branch February 7, 2025 11:04
@vszakats

vszakats commented Feb 7, 2025

Copy link
Copy Markdown
Member
-  set(_sys_libdirs "")
+  set(_sys_libdirs ${CMAKE_C_IMPLICIT_LINK_DIRECTORIES})

CMake shenanigans: Written this way, you get inconsistent behavior depending on CMAKE_C_IMPLICIT_LINK_DIRECTORIES: When it is empty, the command becomes an unset, letting this scope see a potential parent scope value.

That's why I didn't do it that way.

On second thought, it is fine with double quotes.

-  set(_sys_libdirs "")
+  set(_sys_libdirs "${CMAKE_C_IMPLICIT_LINK_DIRECTORIES}")

Nice, thanks!

vszakats added a commit to libssh2/libssh2 that referenced this pull request Feb 7, 2025
For consistent initialization for `_sys_libdirs` and just for consistent
formatting in the other case.

Ref: curl/curl#16233 (comment)
Follow-up to 3de8731 #1540
vszakats added a commit to vszakats/curl that referenced this pull request Feb 7, 2025
vszakats added a commit to vszakats/curl that referenced this pull request Feb 7, 2025
vszakats added a commit that referenced this pull request Feb 7, 2025
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
The unnecessary `-L` may unexpectedly change the toolchain's chosen
search order in implicit link directories.

Causing this error noticed by a vcpkg user, and reproduced, in the
context of Android NDK r22b, API level 16, static library and CRT
linkage:
~~~
configure:23635: checking whether libcurl is usable
configure:23669: armv7a-linux-androideabi16-clang -o conftest
  --sysroot=/home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot
  -DANDROID -DDECLARE_TIMEZONE -Wall
  -I/home/runner/work/boinc/boinc/3rdParty/buildCache/android/android-tc/arm/arm-linux-androideabi/include
  -O3 -fomit-frame-pointer -fPIE -march=armv7-a -mfloat-abi=softfp -mfpu=vfpv3-d16 -D__ANDROID_API__=16 -Wall
  -I/home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/include
  -I/home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/lib/pkgconfig/../../include -DCURL_STATICLIB
  -L/home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/lib -llog -fPIE -pie -latomic -static-libstdc++
  -march=armv7-a -Wl,--fix-cortex-a8 -L/home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/lib conftest.c
  -L/home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/lib/pkgconfig/../../lib -lcurl
  -L/home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/arm-linux-androideabi
  -lssl -lcrypto -ldl -pthread -lz  >&5
clang: warning: argument unused during compilation: '-static-libstdc++' [-Wunused-command-line-argument]
ld: error: undefined symbol: bsd_signal
>>> referenced by legacy_signal_inlines.h:116 (/home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/legacy_signal_inlines.h:116)
>>>               libcrypto-lib-ui_openssl.o:(read_string_inner) in archive /home/runner/work/boinc/boinc/3rdParty/android/vcpkg/installed/arm-android/lib/libcrypto.a
ld: error: undefined symbol: pthread_atfork
~~~
with the unnecessary option
~~~
-L/home/runner/work/boinc/boinc/3rdParty/android/android-ndk-r22b/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/arm-linux-androideabi
~~~
Reference: microsoft/vcpkg#43463 (comment)

Closes curl#16233
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants