Skip to content

Add x64 arch and freebsd arm64 in cross toolchain #9834

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

Merged
merged 4 commits into from
Jul 13, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented Jun 28, 2022

Currently we recognize x64 architecture in cross toolchain for FreeBSD and illumos, but we do not treat it the same way as other architectures in the script. This PR moves x64 to the same plan as other architectures and adjusts the code flow.

Also added FreeBSD arm64.

@am11
Copy link
Member Author

am11 commented Jun 28, 2022

cc @sec, @Thefrank, @janvorli

This makes it easier to investigate issues like dotnet/runtime#71338 using docker on linux, mac or windows (without installing FreeBSD arm64). This script is used at https://github.com/dotnet/dotnet-buildtools-prereqs-docker/blob/main/src/ubuntu/18.04/cross/freebsd/12/hooks/pre-build to produce docker images for runtime build prerequisites.

set(TIZEN_TOOLCHAIN "aarch64-tizen-linux-gnu/9.2.0")
endif()
elseif(FREEBSD)
set(triple "aarch64-unknown-freebsd12")
Copy link

Choose a reason for hiding this comment

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

From my tests, isn't set(TOOLCHAIN "llvm") also needed here, as later eng/native/configuretools.cmake will fail while looking for objcopy ?

Copy link
Member Author

Choose a reason for hiding this comment

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

TOOLCHAIN is used to indicate triplet for other platforms. For freebsd-x64, it should be the same value before and after this refactoring. The arm64 block is a copy of refactored x64 block.

@sec
Copy link

sec commented Jun 28, 2022

For compile to even start, I think that also lines:
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fuse-ld=lld")
should be
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fuse-ld=lld -stdlib=libc++ -Qunused-arguments")
so that llvm pickup the right library. Dunno if this is right variable, but without the second silence warning, it will fail some configure checks.

And also
if((TARGET_ARCH_NAME MATCHES "^(arm|armv6|armel|arm64|ppc64le|s390x)$" AND NOT ANDROID) OR ILLUMOS)
to
if((TARGET_ARCH_NAME MATCHES "^(arm|armv6|armel|arm64|ppc64le|s390x)$" AND NOT ANDROID AND NOT FREEBSD) OR ILLUMOS)
so that compiler targets are not overwritten, which will fail later.

thanks @am11 for starting this - as for docker images with arm64, I assume there will be a need for separate PR to add those Dockerfiles, right?

@Thefrank
Copy link
Contributor

Just as a heads up from the readme.txt on FreeBSD's download FTP:

releases/${MACHINE}/${MACHINE_ARCH}/*-RELEASE/
    The official FreeBSD releases as individual tarballs to download; useful
    for jail hosting, updating machines in situ, etc.  Make sure to look in
    the ${MACHINE_ARCH} sub-directory for the most recent releases (e.g.,
    releases/amd64/amd64); this is a change from the earlier ${ARCH} naming
    scheme (e.g., releases/amd64).

so https://download.freebsd.org/ftp/releases/${__FreeBSDArch}/${__FreeBSDBase}/base.txz will work for now but might break in the future.

set(CMAKE_SYSTEM_PROCESSOR x86_64)
if(LINUX)
set(TOOLCHAIN "x86_64-linux-gnu")
if(TIZEN)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if there's x64 Tizen. I have thought Tizen is only arm / x86. @alpencolt am I right?

Copy link
Member Author

Choose a reason for hiding this comment

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

According to Wikipedia https://en.wikipedia.org/wiki/Tizen, x86-64 is supported. Here are the downloadable packages for x86_64: https://download.tizen.org/live/Tizen/standard/x86_64/

@am11
Copy link
Member Author

am11 commented Jun 28, 2022

@sec, thanks. I have updated the TOOLCHAIN related condition. As to the unused variable suppression, we can handle that in runtime/eng/native/configurecompiler.cmake etc. This toolchain.cmake is essentially to initialize variables in cross build scenario.

@sec
Copy link

sec commented Jun 28, 2022

What about a place to add -stdlib=libc++ -Qunused-arguments to that it won't fail with ld.lld: error: unable to find library -lstdc++ ?

@sec
Copy link

sec commented Jun 28, 2022

For the objcopy error, will this be ok?

diff --git a/eng/native/configuretools.cmake b/eng/native/configuretools.cmake
index 6697524c659..8476d82cca8 100644
--- a/eng/native/configuretools.cmake
+++ b/eng/native/configuretools.cmake
@@ -54,7 +54,11 @@ if(NOT WIN32 AND NOT CLR_CMAKE_TARGET_BROWSER)
       set(TOOLSET_PREFIX ${ANDROID_TOOLCHAIN_PREFIX})
     elseif(CMAKE_CROSSCOMPILING AND NOT DEFINED CLR_CROSS_COMPONENTS_BUILD AND
         CMAKE_SYSTEM_PROCESSOR MATCHES "^(armv8l|armv7l|armv6l|aarch64|arm|s390x|ppc64le)$")
-      set(TOOLSET_PREFIX "${TOOLCHAIN}-")
+        if(FREEBSD)
+         set(TOOLSET_PREFIX "llvm-")
+        else()
+         set(TOOLSET_PREFIX "${TOOLCHAIN}-")
+        endif()
     else()
       set(TOOLSET_PREFIX "")
     endif()

@am11
Copy link
Member Author

am11 commented Jun 28, 2022

objcopy configuration does not belong to toolchain.cmake. We set them in runtime/eng/native/configuretools.cmake. For more info on what "toolchain file" is for in cmake, see https://cmake.org/cmake/help/book/mastering-cmake/chapter/Cross%20Compiling%20With%20CMake.html#toolchain-files.

@am11
Copy link
Member Author

am11 commented Jun 28, 2022

as for docker images with arm64, I assume there will be a need for separate PR to add those Dockerfiles, right?

@sec, yes, we can add that after this PR is merged.

https://download.freebsd.org/ftp/releases/${__FreeBSDArch}/${__FreeBSDBase}/base.txz will work for now but might break in the future.

@Thefrank, thanks. Updated in 00dab08.

Thefrank
Thefrank previously approved these changes Jun 28, 2022
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli janvorli merged commit 57d358a into dotnet:main Jul 13, 2022
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.

4 participants