-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Support generic Armel builds #38798
Support generic Armel builds #38798
Conversation
// Auto-generated message 69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts. To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others. |
@dotnet/runtime-infrastructure could someone please assign themselves this PR to help resolve it one way or another? It is 164 days old and I am trying to get to a 90th percentile of 90 days for community PR's. |
While we're here, i'll try to explain the situation with this PR The original intent was to run dotnet core on a VFP-less machine. That turned out to be impossible but i think some of the changes that i made could be useful for others With this PR i made some adjustments to be able to build the codebase while targeting an embedded Arm machine (my modem/router combo) running Linux 3.4 and with uClibc (non -ng). Now it's been a while since i looked into this, but maybe if you can give me some guidance i can try to fix the remaining issues Thanks |
Directory.Build.props
Outdated
@@ -74,7 +74,7 @@ | |||
|
|||
<!-- Indicates this is not an officially supported release. Release branches should set this to false. --> | |||
<!-- Keep it in sync with PRERELEASE in eng/native/configureplatform.cmake --> | |||
<IsPrerelease>true</IsPrerelease> | |||
<IsPrerelease>false</IsPrerelease> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be set to true
on non release branches. It seems like you have extra commits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i started from the last prerelease branch, which included that commit.
It should be easily reversible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should put your commits on top of a branch created from master to make it more reviewable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Santi, in general your changes look reasonable and valuable for certain scenarios, we just need to figure out the necessary logistics. I would be also grateful if @janvorli could take a look at the cmake changes as he's one of our biggest linux / cmake experts.
@@ -26,6 +26,9 @@ add_compile_options(-Wno-cast-align) | |||
add_compile_options(-Wno-typedef-redefinition) | |||
add_compile_options(-Wno-c11-extensions) | |||
add_compile_options(-Wno-unknown-pragmas) | |||
add_compile_options(-Wno-atomic-alignment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we get the warning if it is not disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember off hand, i'll need to rebuild without it to check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to remove this change and the build succeeded fine without it and without any warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm this. I might have unknowingly fixed it in the toolchain or it was related to something else.
Actually, i wonder if it's related to the GCC version used (i might have changed the gcc version used in the toolchain along the way).
According to my original commit message, it was emitting an error similar to this
__atomic_compare_exchange_n (large atomic operation may incur significant performance penalty)
A simple grep shows this is part of
src/libraries/Native/Unix/Common/pal_atomic.h
src/libraries/Native/Unix/System.Native/pal_random.c
@@ -447,10 +447,11 @@ check_symbol_exists( | |||
HAVE_DISCONNECTX) | |||
|
|||
set(PREVIOUS_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS}) | |||
set(CMAKE_REQUIRED_FLAGS "-Werror -Wsign-conversion") | |||
set(CMAKE_REQUIRED_FLAGS "-Werror=sign-conversion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain this change? We want error on all warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is related to other warnings caused by uClibc headers with new compilers, so the change is needed to catch only the warning relevant for the check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to error on all warnings that are enabled. Our policy w.r.t. warning has been to fix them if reasonably feasible or disable the specific ones where fixing is not possible or too cumbersome. It would be good to get a complete list of warnings that we are getting to reason about what to do here. It seems that the the warnings are uClibs rather than armel specific. So we could start with disabling them all just when compiling against uClibc. Is there a reasonable way to detect we are compiling against uCLibs at build time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to remove this change too and the build succeeded without any warnings or errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might depend on the gcc version i used back then, similarly to -Wno-atomic-alignment
I can revert this. However, i believe that the check should only care about sign-conversion
related errors since it's checking whether or not the flags are signed
@smx-smx I was wondering if you could share details on how to create a rootfs for the OS you are targeting. I'd like to try it locally , as it would give me more insight into the warnings etc. |
@janvorli thanks for the interest, first of all. We are talking about Broadcom modem/router combos. Many vendors modify uClibc to fit with their idea of "embedded", but don't actually share the modifications or the configuration required to build a binary compatible toolchain (which is needed to run programs on the device without chroot) I created a custom fork of uClibc that aims to be compatible with said BSP, and it's already built as part of the rootfs i linked
You should be able to build the toolchain+rootfs combo by doing
The toolchain will be saved in My original goal was to run dotnet core on these devices, but it turns out that the CPU cannot do SMP and VFP at the same time (and SMP was preferred). in-depth explanation:
However, even without VFP (i.e. without softfp), it's possible to build and run mono with the netcore profile. This is how far i got, with a working mono + netcore setup (i had to package a test assembly with #!/bin/sh
scriptDir="$(readlink -f $(dirname "$0"))"
assembly="$(readlink -f "$1")"
assemblyDir="$(dirname "$assembly")"
cd "$assemblyDir"
#MONO_LOG_LEVEL=debug \
DOTNET_SYSTEM_GLOBALIZATION_INVARIANT=1 \
MONOPATH="$assemblyDir" \
LD_LIBRARY_PATH="$assemblyDir" $scriptDir/mono-sgen "$assembly"
Hope everything is clear. |
Ok, i found the toolchain file and script i made to build dotnet/runtime for the target: https://gist.github.com/smx-smx/0c5738e219145996912d5e289445c9cc |
the |
@@ -26,6 +26,9 @@ add_compile_options(-Wno-cast-align) | |||
add_compile_options(-Wno-typedef-redefinition) | |||
add_compile_options(-Wno-c11-extensions) | |||
add_compile_options(-Wno-unknown-pragmas) | |||
add_compile_options(-Wno-atomic-alignment) | |||
# Required for clang versions that don't understand -Wno-atomic-alignment | |||
add_compile_options(-Wno-unknown-warning-option) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than disabling this you can also check whether the compiler accepts the -Wno-atomic-alignment
option like this:
runtime/src/libraries/Native/Unix/CMakeLists.txt
Lines 37 to 40 in f8f63b1
check_c_compiler_flag(-Wimplicit-fallthrough COMPILER_SUPPORTS_W_IMPLICIT_FALLTHROUGH) | |
if (COMPILER_SUPPORTS_W_IMPLICIT_FALLTHROUGH) | |
add_compile_options(-Wimplicit-fallthrough) | |
endif() |
@smx-smx the bcm63138-buildroot's |
It should be done for you if you do "make bcm63138_defconfig", which installs the default config. |
Great, thank you! |
Thank you too! If it can help you can also reach me on the dotnet Discord and/or Gitter rooms for quicker replies |
Hmmm maybe it wasn't needed then, since the variable name was wrong but the POC runs fine without it anyways |
@smx-smx I have built the rootfs and attempted to build stuff using your scripts. However, the build fails with: Restore was successful. I have modified your scripts to point TOOLCHAIN_DIR to my local directory where I've built the rootfs. |
Starting from a clean state (my I surely get past your situation. The scripts are updated, i used Tomorrow i can try looking into why Edit: I probably used the |
Ah, now it is running, I had a stale .dotnet directory in the root of the repo. |
The mono part build has passed, but it has failed on the libraries build - it was unable to find objcopy:
|
…of the coreclr build Fallback to linux/ptrace.h if we don't.
OK, I think I've fixed all remaining CoreCLR and library test failures. @akoeplinger, would you be able to look at the Mono failures or find someone who would? @smx-smx, a double-check whether this still builds / works for you would be certainly beneficial to verify we haven't inadvertently nullified the gist of your change. |
@@ -10,7 +10,7 @@ elseif(CLR_CMAKE_TARGET_MACCATALYST) | |||
endif() | |||
cmake_policy(SET CMP0042 NEW) | |||
|
|||
project(CoreFX C) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should remove the "C". All corefx sources are intentionally C code (because of Mono)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In such case we need to figure out a different way to encode the HAVE_SIGACTION_ULONG_FLAGS check, I haven't found any way to do that with plain C due to its less stringent type conversion checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't understand then why the C compiler complains when compiling the actual code, but doesn't fail on the same thing in the check. I am starting to wonder whether the changes related to HAVE_SIGACTION_ULONG_FLAGS originated from the time when the libraries were still being compiled as C++ code. @smx-smx could it be the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm the test works for me in plain C
$ arm-buildroot-linux-uclibcgnueabi-gcc -Werror=incompatible-pointer-types t.c -o t
$ gcc -Werror=incompatible-pointer-types t.c -o t
t.c: In function ‘main’:
t.c:5:32: error: initialization of ‘long unsigned int *’ from incompatible pointer type ‘int *’ [-Werror=incompatible-pointer-types]
5 | unsigned long *flags = &action.sa_flags;
| ^
cc1: some warnings being treated as errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so maybe setting
CMAKE_REQUIRED_FLAGS=-Werror=incompatible-pointer-types
before the test code and clearing it after could do the trick.
Hello, I was thinking about setting up a CI pipeline for the environment and I made some progress in that regard. I'll see if I can find some time to get it finalized |
CI Environment is ready: https://cirrus-ci.com/task/5243984923590656
|
…configure.cmake" This reverts commit 70e9547.
@@ -30,7 +30,7 @@ set(OS_LIBS "-framework CoreFoundation" "-lobjc" "-lc++") | |||
elseif(HOST_ANDROID) | |||
set(OS_LIBS m dl log) | |||
elseif(HOST_LINUX) | |||
set(OS_LIBS pthread m dl) | |||
set(OS_LIBS pthread m dl atomic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this needs to be turned into a check.
Some C libraries require this, others don't have it
https://dev.azure.com/dnceng/public/_build/results?buildId=1146706&view=logs&j=38c63b74-c97f-5bda-376d-1084248ecb57&t=b7130259-7da2-5518-9023-82792e689970&l=1631
Closing for now as there's no traffic on this PR and it regularly shows up in our stale PR reports. @smx-smx, please reopen if you get back to working on this change. Thanks. |
The armel (VFP) target is currently quite obscure.
It's there, but in reality it seems dedicated to Tizen only.
I'm experimenting with some Linux ARM (VFP) devices and i wanted to run coreclr on them.
To my surprise i noticed that no build for linux-armel was available, so i tried to get one going for non-tizen platforms.
Here is a list of issues discovered:
Tizen specific toolchain rules are hardcoded in the toolchain file
WORKAROUND: add support for a custom toolchain file (see PR)
Missing armel/armv7-a handling in a few places (see PR for fixes)
Build errors in coreclr with old 3.x kernel (incorrect ptrace header for the older kernel, see PR for a possible fix)
Build errors in System.Native due to
-Wimplicit-int-conversion
(this happens due to getnameinfo, htons, and a few others having unsigned prototypes)WORKAROUND (uncommitted): drop
-Wimplicit-int-conversion
(dirty, needs a better fix)Armel cross build is Tizen specific and does not support Portable RID build
runtime/eng/native/init-distro-rid.sh
Line 149 in 16b451d
(ignore the prerelease commit as i initially based these fixes on the latest preview. i can remove it later)
Do you think it's possible to introduce a specific "tizen" target or convert it into a generic linux-armel target?
Thank you