Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Set -march=armv7-a for Alpine Linux ARM32 #17727

Closed
wants to merge 1 commit into from
Closed

Set -march=armv7-a for Alpine Linux ARM32 #17727

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 22, 2018

Alpine ARM32 (armhf) comes with ARMv6 ABI with default ISA set to ARMv6 (16-bit instruction set) due to which the build fails at very beginning on Alpine ARM32 (1%).

Setting armv7-a in build option enables the usage of 32-bit instructions (like dmb ish) that CoreCLR is using.

This takes the build quite farther, but I haven't gotten to 100% yet.

Related to #17468.

@ghost ghost changed the title Set -march=arm7-a for Alpine Linux ARM32 Set -march=armv7-a for Alpine Linux ARM32 Apr 22, 2018
@@ -66,6 +66,8 @@ if(CLR_CMAKE_PLATFORM_UNIX_ARM)
add_definitions(-DARM_SOFTFP)
add_compile_options(-mfloat-abi=softfp)
add_compile_options(-target armv7-linux-gnueabi)
elseif(CLR_CMAKE_PLATFORM_ALPINE_LINUX)
add_compile_options("-march=armv7-a")
else()
add_compile_options(-target armv7-linux-gnueabihf)
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional that -target armv7-linux-gnueabihf is not longer going to be present for Alpine?

Copy link
Member

Choose a reason for hiding this comment

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

I also wonder how come you don't need to specify the target.

Copy link
Author

Choose a reason for hiding this comment

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

Yes it is intentional. Alpine has armv6-alpine-linux-musleabi no GNU stuff, which that sets arch to armv6 which CoreCLR does not support, so we only need to set march to armv7 and abi targets gets picked up on ARM32 system.

Copy link
Member

Choose a reason for hiding this comment

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

Would similar thing work on other platforms - that means, if we used just the -march instead of the -target?

Copy link
Author

Choose a reason for hiding this comment

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

I think the difference in case of Alpine+CoreCLR is that Alpine provides v6 ABI target for ARM32 that has arch set to armv6 by default that works on both ARMv6 and v7 versions of OS, while other platforms have specific armv7 ABI with default arch set correctly to armv7. We can test if setting -march=armv7-a is enough for those.

As noted in the issue that I was actually stuck to get pass 1% of build, asked a question richfelker/musl-cross-make#43 and Rich Felker (primary author of musl libc https://ewontfix.com/about/) told me how to deal with this using -march. It turned out to be working well (until 26% in 4 hours of compilation over qemu).

At 26%, I get:

[ 25%] Building CXX object src/debug/createdump/CMakeFiles/createdump_lib.dir/createdump.cpp.o
/coreclr/src/gc/unix/cgroup.cpp:407:32: error: implicit conversion from 'unsigned long long' to 'size_t'
      (aka 'unsigned int') changes value from 18446744073709551615 to 4294967295 [-Werror,-Wconstant-conversion]
    size_t rlimit_soft_limit = RLIM_INFINITY;
           ~~~~~~~~~~~~~~~~~   ^~~~~~~~~~~~~
/usr/include/sys/resource.h:72:24: note: expanded from macro 'RLIM_INFINITY'
#define RLIM_INFINITY (~0ULL)
                       ^~~~~
1 error generated.
make[2]: *** [src/gc/CMakeFiles/clrgc.dir/build.make:495: src/gc/CMakeFiles/clrgc.dir/unix/cgroup.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:61853: src/gc/CMakeFiles/clrgc.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 26%] Building CXX object src/debug/createdump/CMakeFiles/createdump_lib.dir/crashinfo.cpp.o
[ 26%] Building CXX object src/debug/createdump/CMakeFiles/createdump_lib.dir/threadinfo.cpp.o
[ 26%] Building CXX object src/debug/createdump/CMakeFiles/createdump_lib.dir/datatarget.cpp.o
/coreclr/src/debug/createdump/threadinfo.cpp:6:10: fatal error: 'asm/ptrace.h' file not found
#include <asm/ptrace.h>
         ^~~~~~~~~~~~~~
1 error generated.
make[2]: *** [src/debug/createdump/CMakeFiles/createdump_lib.dir/build.make:111: src/debug/createdump/CMakeFiles/createdump_lib.dir/threadinfo.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/Makefile2:61988: src/debug/createdump/CMakeFiles/createdump_lib.dir/all] Error 2
make: *** [Makefile:130: all] Error 2
Failed to build CoreCLR component.

I am planning to work on those errors separately after this one gets through.

@jkotas jkotas requested a review from janvorli April 22, 2018 15:31
@@ -66,6 +66,8 @@ if(CLR_CMAKE_PLATFORM_UNIX_ARM)
add_definitions(-DARM_SOFTFP)
add_compile_options(-mfloat-abi=softfp)
add_compile_options(-target armv7-linux-gnueabi)
elseif(CLR_CMAKE_PLATFORM_ALPINE_LINUX)
add_compile_options("-march=armv7-a")
Copy link
Member

Choose a reason for hiding this comment

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

A nit - can you please remove the double quotes to make it consistent with the other options we set?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Author

Choose a reason for hiding this comment

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

Couldn't reopen after the bad rebase. New PR #17730.

@ghost ghost closed this Apr 22, 2018
@rofl0r
Copy link

rofl0r commented Apr 22, 2018

not a good idea what you're doing here.

  1. you break build for any non-armv7 platform
  2. you hardcode different properties for a certain distribution.

what really should be done is something like the following

  • a configure test which checks whether dmb ish is supported by the user-set CC
  • if not another check which tests whether it is supported after adding -march=armv7 to the CFLAGS
  • if not, provide a fallback in the code that doesn't use dmb ish which gets activated depending on the tests of the configure check.

@janvorli
Copy link
Member

@rofl0r we don't and cannot support arm v6 in the runtime and JIT. The only supported arm32 target is arm v7. If someone tried to build it using a compiler that supports only armv6 and then tried to run it on arm v7 device, things would just break. The reason is that the missing instructions are needed to enable us properly run on multi-core devices. Without them, there is no way to ensure memory coherency.
That's why when we build for arm, we require arm v7 (we use the -target armv7-linux-gnueabi / -target armv7-linux-gnueabihf for other Linux distros).
@kasper3 has found that Alpine has decided to only support arm v6 in their arm toolchain, you can read more detail on that in #17468.
In order to not to have to check for Alpine in the cmake file, I have asked @kasper3 to see if we can actually use just the -march=armv7-a for all distros.

@ghost
Copy link
Author

ghost commented Apr 22, 2018

@rofl0r, this gets applied to only ARM32 platform:

if(CLR_CMAKE_PLATFORM_UNIX_ARM)
    # Because we don't use CMAKE_C_COMPILER/CMAKE_CXX_COMPILER to use clang
    # we have to set the triple by adding a compiler argument
    add_compile_options(-mthumb)
    add_compile_options(-mfpu=vfpv3)
    if(ARM_SOFTFP)
  ...
   elseif(CLR_CMAKE_PLATFORM_ALPINE_LINUX) # << i have added these two lines
     add_compile_options(-march=armv7-a)

CoreCLR does not support ARMv6 for 32 bit, they have used 32-bit instructions and reading through this thread https://github.com/dotnet/coreclr/issues/10605 feels like they won't be investing time on supporting v6.

I agree that it would be ideal to feature detect the target capability and add support, but that wold require codegen changes in other layers, which is beyond the scope (and probably awaiting mainainters' final decision on https://github.com/dotnet/coreclr/issues/10605).

@rofl0r
Copy link

rofl0r commented Apr 22, 2018

i think what @janvorli says

-march=armv7-a for all distros.

would be a cheap and non-offensive way to make it work.

it's not true that

Alpine has decided to only support arm v6 in their arm toolchain

it's just that they default to armv6, which, as already noticed, can be fixed by specifying the target v7 cpu arch using CFLAGS.

in either case, i think it would still be the cleanest solution to check whether the compiler can compile a testcase with dmb ish using the default CFLAGS, and if not add -march=whatever.

@ghost
Copy link
Author

ghost commented Apr 22, 2018

Sure, I am testing the patch here: #17730. Lets see if other platforms are happy with the change.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants