search

dotnet / arcade

Tools that provide common build infrastructure for multiple .NET Foundation projects.
MIT License
672 stars 349 forks source link

eng/common/cross/toolchain.cmake incorrect default compiler options. #7607

Open michaelgsharp opened 3 years ago

michaelgsharp commented 3 years ago

When cross-targeting for arm eng/common/cross/toolchain.cmake adds the -mthumb flag. If the -mthumb flag is added but the -fPIC -march=armv7-a flags are not added then it compiles for armv6 or v5 and uses thumb instead of thumb2, which is quite different. One of the differences with thumb is that its 16 bits, which causes interop with floating point numbers to not work correctly. Since -mthumb is added automatically, but -fPIC and -march=armv7-a are not, this makes the default compilation work incorrectly.

I synced with @janvorli and he suggested that:

The toolchain file is used only in the crosscompilation case, so it actually seemed to me that it should not contain any compiler options and they should all come from the eng/native/configurecompiler.cmake in the runtime build case or from some other file specific to your component build.

We could remove the all compilation flags as suggested by Jan, just remove the -mthumb flag from the cross/toolchain.cmake` file, or add in the missing flags, but whatever approach we take we should make sure that the default case works correctly.

markwilkie commented 3 years ago

@jeffschwMSFT - who are the right people to talk to about the ramifications of removing the flags for x-compile?

jeffschwMSFT commented 3 years ago

Adding @janvorli and @jkoritzinsky who have a lot of knowledge about compiler flags.

janvorli commented 3 years ago

The thing is that we set some compiler flags in the eng/common/cross/toolchain.cmake that comes from arcade, but the dotnet/runtime build then sets them in its local eng/native/configurecompiler.cmake. The toolchain.cmake is used only for cross compilation, but not for native compilation. But both cross and native compilation use eng/native/configurecompiler.cmake. E.g. for Linux arm, one can build the runtime on a x64 Linux box using cross compilation or on native linux arm box natively. Both builds need the same compiler options, so having some of them set in the toolchain.cmake and setting some / overriding some in the configurecompiler.cmake doesn't make sense. The configurecompiler.cmake should be the source of the options. I think this discrepancy is some remainder of some cleanups we've made in the past. If other repos than runtime use the cross compilation toolchain.cmake, it should have its own mean to set the compiler options it needs.

janvorli commented 3 years ago

(@michaelgsharp has created this issue after our discussion on the problems he was having due to this discrepancy)

markwilkie commented 3 years ago

Sounds like this is sorted so am closing. @michaelgsharp - feel free to reopen if you need more of course.

michaelgsharp commented 3 years ago

@markwilkie I have the issue sorted in my repo, but as far as I know the discrepancy that @janvorli pointed out still exists in Arcade itself.

I think it would be good to re-open this issue to track the resolution of this in Arcade so that anyone else who onboards to arcade doesn't run into this same issue. Otherwise if the new repo doesn't have the right flags set, every new onboarding experience will probably keep wasting time trying to figure out why the cross-compilation doesn't work the way you would expect. Currently, the default is a non-working build, and that doesn't seem like a good idea to me.

Thoughts?

riarenas commented 3 years ago

Should this go into the arcade SDK epic? doesn't seem like anyone is blocked right now, but also sounds like this is the right thing to do.

ChadNedzlek commented 3 years ago

Is the arcade SDK epic just a random pile of tiny feature requests? Those can be... hard to prioritize/organize (and the issues tend to just flow right back out of them)

markwilkie commented 3 years ago

Is the arcade SDK epic just a random pile of tiny feature requests? Those can be... hard to prioritize/organize

Unfortunately the answer for now is - yes. Also, it's the best one we have given that we're not current prioritizing a holistic push on Arcade.

I'll move this issue into the "pile" (backlog for Arcade) and as always if someone wants to submit a PR that'd be awesome.