Check for CMAKE_C_COMPILER_ID not CXX #64226

directhex · 2022-01-24T20:36:37Z

In some cases, CMAKE_CXX_COMPILER_ID might not be populated, which causes the toolchain prefix checks to be ignored, which causes the build to look for ranlib not llvm-ranlib, which causes it to use XCode ranlib instead of Android NDK ranlib for builds, which doesn't work.

Hopefully (we'll see what CI says) this doesn't regress other builds.

Closes: #55412

In some cases, CMAKE_CXX_COMPILER_ID might not be populated, which causes the toolchaine prefix checks to be ignored, which causes the build to look for `ranlib` not `llvm-ranlib`, which causes it to use XCode ranlib instead of Android NDK ranlib for builds, which doesn't work. Hopefully (we'll see what CI says) this doesn't regress other builds. Closes: dotnet#55412

dotnet-issue-labeler · 2022-01-24T20:36:41Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

jkotas · 2022-01-24T20:39:25Z

cc @am11

am11

LGTM

eng/native/configuretools.cmake

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

directhex · 2022-01-25T16:06:09Z

Failing lane is an unrelated timeout issue, will do a separate PR to make that one not run on PR

…rt-is-moderately-cursed

It's available in r21 (which we use today) and works for r23 (which has no GNU objcopy any more and breaks without this change)

am11 · 2022-01-26T15:39:26Z

@jkotas, @directhex, looking at it again with fresh eyes I just remember making this ditto change in the past ba50840 to use "correct" objcopy. Unfortunately, it caused binary size regression and ultimately we had to revert it.

The issue was that on my local system (Ubuntu) I was using llvm-10 at the time but on build systems, we were (and still are) using llvm-9 in most cases. There is a bug in llvm-9 objcopy which fails to strip the symbols. LLVM 10 onwards has that bug fixed (it was never backported).

Therefore, I think we should hold this until we update official baseline to clang 10 or above, and continue to use GNU's objcopy instead of LLVM9's one.

directhex · 2022-01-26T15:47:35Z

@am11 this is only using llvm-objcopy from the Android NDK, not in general. Do you know where in the LLVM tree objcopy was fixed? I can check the Android repo to see if Google patched it downstream in their llvm 9 (NDK r21)

am11 · 2022-01-26T16:04:19Z

@directhex, the effect was on objcopy but the change was exactly what you have CMAKE_CXX_COMPILER_ID to CMAKE_C_COMPILER_ID that broke the world.

directhex · 2022-01-26T17:09:33Z

@am11 I can't reproduce that.

Using mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-20210714125435-9b5bbc2, on git master:

-rwxr-xr-x 1 root root 14M Jan 26 16:23 /home/directhex/Projects/runtime/artifacts/bin/coreclr/Linux.x64.Debug/./libcoreclr.so
-rw-r--r-- 1 root root 79M Jan 26 16:23 /home/directhex/Projects/runtime/artifacts/bin/coreclr/Linux.x64.Debug/./libcoreclr.so.dbg

this branch:

-rwxr-xr-x 1 root root 14M Jan 26 16:29 /home/directhex/Projects/runtime/artifacts/bin/coreclr/Linux.x64.Debug/./libcoreclr.so
-rw-r--r-- 1 root root 79M Jan 26 16:29 /home/directhex/Projects/runtime/artifacts/bin/coreclr/Linux.x64.Debug/./libcoreclr.so.dbg

Looking at the build logs and CMake output, CMAKE_C_COMPILER_ID and CMAKE_CXX_COMPILER_ID are both set correctly to Clang on Linux, so the behaviour should not be changed by this PR.

Actually, looking even closer, the core behaviour here is:

The TOOLSET_PREFIX is set to llvm- both before and after my PR:

runtime/eng/native/configuretools.cmake

Lines 9 to 22 in df69293

 if(NOT WIN32 AND NOT CLR_CMAKE_TARGET_BROWSER) 

 if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") 

 if(APPLE) 

 set(TOOLSET_PREFIX "") 

 else() 

 set(TOOLSET_PREFIX "llvm-") 

 endif() 

 elseif(CMAKE_CXX_COMPILER_ID MATCHES "GNU") 

 if(CMAKE_CROSSCOMPILING) 

 set(TOOLSET_PREFIX "${CMAKE_CXX_COMPILER_TARGET}-") 

 else() 

 set(TOOLSET_PREFIX "") 

 endif() 

 endif()

ar, nm, ranlib, link and objdump are set based on this value of TOOLSET_PREFIX:

runtime/eng/native/configuretools.cmake

Lines 42 to 51 in df69293

 locate_toolchain_exec(ar CMAKE_AR) 

 locate_toolchain_exec(nm CMAKE_NM) 

 locate_toolchain_exec(ranlib CMAKE_RANLIB) 

 if(CMAKE_C_COMPILER_ID MATCHES "Clang") 

 locate_toolchain_exec(link CMAKE_LINKER) 

 endif() 

 if(NOT CLR_CMAKE_TARGET_OSX AND NOT CLR_CMAKE_TARGET_MACCATALYST AND NOT CLR_CMAKE_TARGET_IOS AND NOT CLR_CMAKE_TARGET_TVOS AND (NOT CLR_CMAKE_TARGET_ANDROID OR CROSS_ROOTFS)) 

 locate_toolchain_exec(objdump CMAKE_OBJDUMP)

TOOLSET_PREFIX is reconfigured to be ${ANDROID_TOOLCHAIN_PREFIX} for Android, ${TOOLCHAIN}- if cross compiling for ARM or s390x, and crucially blank otherwise - this means the value for TOOLSET_PREFIX is now blank on linux-x64:

runtime/eng/native/configuretools.cmake

Lines 53 to 60 in df69293

 if(CLR_CMAKE_TARGET_ANDROID) 

 set(TOOLSET_PREFIX ${ANDROID_TOOLCHAIN_PREFIX}) 

 elseif(CMAKE_CROSSCOMPILING AND NOT DEFINED CLR_CROSS_COMPONENTS_BUILD AND 

 CMAKE_SYSTEM_PROCESSOR MATCHES "^(armv7l|armv6l|aarch64|arm|s390x)$") 

 set(TOOLSET_PREFIX "${TOOLCHAIN}-") 

 else() 

 set(TOOLSET_PREFIX "") 

 endif()

set objcopy with this blank prefix (which means GNU objcopy will be used, due to the empty TOOLSET_PREFIX)

The reasoning given in #49092 for the revert is definitely the linux-x64 case

am11 · 2022-01-26T17:14:59Z

set objcopy with this blank prefix (which means GNU objcopy will be used, due to the empty TOOLSET_PREFIX)

Cool. Thanks for confirming it. We can update it to also use matching/correct toolchain, once baseline is updated to clang 10 or beyond.

directhex · 2022-01-26T17:30:18Z

Do we have a CI lane to check for size regression? Because I don't see how this change ever affected the build in the way reported in #49092

am11 · 2022-01-26T17:45:42Z

Not sure if there is such a lane, but there is a label to that effect: https://github.com/dotnet/runtime/issues?q=is:open+is:issue+label:size-reduction.

I think in this case, the shipping installer package (which includes runtimes+sdks+templates) was manually compared with the previous release at the time. Similar inspection is done for file count differences in packages near the releases to prevent unintended files from getting shipped (or find missing essential files or even duplicate files in nuget packages). Some of those targets are automated and run only during the official builds AFAIK.

am11 · 2022-01-27T04:28:09Z

so the behaviour should not be changed by this PR.

Technically the behavior does change for libs.native subset due to second argument to project command:

runtime/src/native/libs/CMakeLists.txt

Line 18 in 5a9bcde

project(CoreFX C)

In this case, cmake only locates and activates C toolchain and CMAKE_CXX_COMPILER_ID is empty string, but CMAKE_C_COMPILER_ID has a value.

directhex · 2022-01-27T14:51:31Z

Okay, it seems it does have an effect on libs.native - for ar, link, nm, objdump and ranlib it picks the LLVM versions instead of GNU

But it still uses GNU objcopy either way, due to TOOLSET_PREFIX being wiped, so the file size is no different on libSystem.Native.so before or after the patch

directhex · 2022-01-28T14:33:06Z

@am11 can I have an approval, if you're confident with my analyses of the file size concern? Is there someone else we should ask?

am11 · 2022-01-28T14:57:22Z

LGTM, thank you!

cc @janvorli

janvorli

LGTM, thank you!

ghost assigned directhex Jan 24, 2022

am11 reviewed Jan 24, 2022

View reviewed changes

eng/native/configuretools.cmake Outdated Show resolved Hide resolved

Update eng/native/configuretools.cmake

baca797

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

directhex marked this pull request as ready for review January 25, 2022 02:17

directhex and others added 3 commits January 25, 2022 12:53

Merge remote-tracking branch 'origin/main' into cmake-toolchain-suppo…

75ae939

…rt-is-moderately-cursed

Merge remote-tracking branch 'origin/main' into cmake-toolchain-suppo…

3288046

…rt-is-moderately-cursed

Use llvm-objcopy, not GNU, on Android

799dfdc

It's available in r21 (which we use today) and works for r23 (which has no GNU objcopy any more and breaks without this change)

directhex requested a review from marek-safar as a code owner January 26, 2022 14:58

janvorli approved these changes Jan 28, 2022

View reviewed changes

directhex merged commit c7adf55 into dotnet:main Jan 28, 2022

ghost locked as resolved and limited conversation to collaborators Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for CMAKE_C_COMPILER_ID not CXX #64226

Check for CMAKE_C_COMPILER_ID not CXX #64226

directhex commented Jan 24, 2022

dotnet-issue-labeler bot commented Jan 24, 2022

jkotas commented Jan 24, 2022

am11 left a comment

directhex commented Jan 25, 2022

am11 commented Jan 26, 2022

directhex commented Jan 26, 2022

am11 commented Jan 26, 2022

directhex commented Jan 26, 2022

am11 commented Jan 26, 2022

directhex commented Jan 26, 2022

am11 commented Jan 26, 2022

am11 commented Jan 27, 2022

directhex commented Jan 27, 2022

directhex commented Jan 28, 2022

am11 commented Jan 28, 2022

janvorli left a comment

Check for CMAKE_C_COMPILER_ID not CXX #64226

Check for CMAKE_C_COMPILER_ID not CXX #64226

Conversation

directhex commented Jan 24, 2022

dotnet-issue-labeler bot commented Jan 24, 2022

jkotas commented Jan 24, 2022

am11 left a comment

Choose a reason for hiding this comment

directhex commented Jan 25, 2022

am11 commented Jan 26, 2022

directhex commented Jan 26, 2022

am11 commented Jan 26, 2022

directhex commented Jan 26, 2022

am11 commented Jan 26, 2022

directhex commented Jan 26, 2022

am11 commented Jan 26, 2022

am11 commented Jan 27, 2022

directhex commented Jan 27, 2022

directhex commented Jan 28, 2022

am11 commented Jan 28, 2022

janvorli left a comment

Choose a reason for hiding this comment