Temp fix for Android NDK REG_* conflicts #111681
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
src/coreclr/jit/register.h
Outdated
| #define REG_FT11 JITREG_FT11 | ||
| #undef REG_STK | ||
| #define REG_STK JITREG_STK | ||
| #endif // defined(TARGET_RISCV64) |
There was a problem hiding this comment.
Do we really need this for all targets? Can we just do it for only the targets with the conflicts?
There was a problem hiding this comment.
My understanding is that we only need this for x64 and arm32, so a few more can be removed. Also would suggest to self contain the workaround where the enum gets defined if that's possible (so do the undef+defs right after the enums)
There was a problem hiding this comment.
My understanding is that we only need this for x64 and arm32
that doesn't match with what I get, still needed for all four
There was a problem hiding this comment.
It's needed for arm32, x86 and x64 at very least. It may be needed for RISC-V (depends on __USE_GNU define). It's not needed for arm64 with the NDK 27 version on my machine but there's no guarantee about different versions.
There was a problem hiding this comment.
Hmm, odd. @janvorli #110471 (comment) that he was able to get it to build for arm64.
From what I read in that thread it used to build for x64 as well for a different NDK version
There was a problem hiding this comment.
Anyway, if you have ideas how to narrow down the amount of undef, please feel free to send a PR once Android support is landed, so that can be validate on CI instead of creating build errors for that PR
There was a problem hiding this comment.
Why do you think it would not work? While it is not supported by us, I believe linux x86 it is actively being used by Samsung.
There was a problem hiding this comment.
I believe linux x86 it is actively being used by Samsung.
I think it was broken for a while and doesn't pass (some of) the tests. Hello World worked last time I tried (around the time of NativeAOT/win-x86 bring-up). I intend to look into it at some point and I would certainly prefer to keep the x86 code around.
There was a problem hiding this comment.
Why do you think it would not work? While it is not supported by us, I believe linux x86 it is actively being used by Samsung.
I have not seen contributions to linux-x86 for a long time, and we have recently completed some quite substantial internal ABI representation changes within the JIT. Since it is untested I would be surprised if something hasn't bitrotted.
|
@dotnet/jit-contrib ready for review (to unblock android folks). Checked locally that this fixes build for #110471 (comment) |
|
Thanks to @EgorBo and the JIT team for jumping on this quick! |
src/coreclr/jit/register.h
Outdated
| @@ -133,6 +133,180 @@ REGDEF(K7, 7+KBASE, KMASK(7), "k7" ) | |||
|
|
|||
| REGDEF(STK, 8+KBASE, 0x0000, "STK" ) | |||
|
|
|||
| // Temporary workaround to avoid changing all the code that uses REG_* enum values. | |||
There was a problem hiding this comment.
I wouldn't mention "temporary" in the comments... Unless we want to find this comment and laugh about it in 10 years 😉
There was a problem hiding this comment.
comment is still needed to explain why this copy-paste is needed, I am fine with removing the Temporary, just don't want to re-run the CI for it, I can remove it before merge with b-ag
|
/ba-g latest build was green, merging comment-only change |
|
/ba-g merging comment-only change |
Addresses build failures mentioned in this comment.
The Android NDK includes
REG_*constants (and definitions like#define REG_RAX REG_RAX) in system headers, specificallyucontext.h, which is indirectly included in the JIT. This conflicts with JIT'sThere are several options to resolve this issue:
REG_*withJITREG_*using regex. This is a simple solution but would change around 18k places in the JIT codebase + noise from clang-formatter.enum _regNumber_enumwithenum class Regand update all uses fromREG_RAXtoReg::RAX. This adds type safety, as we perform a lot of complex operations on REGs. However, it also means updating 18k places and additional work where we perform operations on registers (casts, comparisons, arithmetic, even shifts!), which can't be fully resolved with overridden operators (I tried). This is likely the most correct approach.We decided to go with the fourth option for now (to unblock the Android folks) as it is the least intrusive and doesn't require many changes, thus minimizing merge conflicts for other PRs.