Initial fixes for macOS <10.9 and PowerPC #21655
Conversation
barracuda156
requested review from
lambdageek,
vargaz,
BrzVlad,
SamMonoRT,
thaystg and
marek-safar
as code owners
|
@dotnet-policy-service agree |
| kern_return_t mach_ret; | ||
|
|
||
| if (pid == getpid ()) { | ||
| /* task_for_pid () doesn't work on ios, even for the current process */ |
There was a problem hiding this comment.
could be simplified unless you also want to target legacy iOS
There was a problem hiding this comment.
iOS is not my concern, so how better to rewrite this chunk?
| @@ -3706,41 +3785,76 @@ mono_arch_output_basic_block (MonoCompile *cfg, MonoBasicBlock *bb) | |||
| */ | |||
| g_assert (!cfg->method->save_lmf); | |||
| /* | |||
| * Note: we can use ppc_r12 here because it is dead anyway: | |||
| * we're leaving the method. | |||
| * Note: we can use ppc_r12 (ppc_r11 on Darwin) here because it is dead | |||
There was a problem hiding this comment.
i think instead of ifdef on every site where r11/r12 is referenced, it would be best to put this as a define ifdef'd on OS and use that for the simple r11 vs. r12 cases, and save ifdef for the more complex ABI differences
There was a problem hiding this comment.
Let’s perhaps see what upstream says. That was my initial idea, but then I thought it may introduce extra complications down the road.
| * (C) 2023 Xamarin, Inc. | ||
| */ | ||
|
|
||
| /* The code below assumes 10.5.x or 10.6.x (10A190 or Rosetta). |
There was a problem hiding this comment.
i'll note my previous half-hearted attempt was on 10.4, so this might explain why you had better success :)
There was a problem hiding this comment.
@NattyNarwhal Tiger is painful to develop for. I have a 10.4.11 set-up to run builds or test, but don’t really have motivation or time to work on it. 10.5.8 and 10A190/10.6.8 are reasonably homogeneous, on the other hand.
There was a problem hiding this comment.
(Registers are not prefixed on Tiger, so to support it here we need to add defines which would add __ on >10.4, but not on <10.5. This is not hard, just extra code, but I have no idea if the rest of the code needs extra modifications.)
| * (C) 2023 Xamarin, Inc. | ||
| */ | ||
|
|
||
| /* The code below assumes 10.5.x. |
There was a problem hiding this comment.
comment could be dropped; IIRC ppc64 was introduced in 10.5
There was a problem hiding this comment.
AFAIK 10.4.11 does support ppc64, at least to some extent.
|
Rebased to fix a typo in |
|
Note that active development of mono is done in the dotnet/runtime repository. |
So should I open a new PR there and close this one, or this can be rebased? |
|
It would be better to submit to dotnet/runtime and later backport here, we have better CI there. |
|
For the general PPC code I agree dotnet/runtime would be better, but I don't think we'll want to take any macOS <10.9 fixes there since a lot of that repo assumes much newer macOS. |
@akoeplinger @vargaz So what should I do? :) |
Provides a fallback for older macOS after the following commit has broken build there: mono@db5f384
Usage of r11/r12 was reversed for the sake of ELFv2 in mono@02cdf6a We do not need that change to apply for macOS, so make it conditional.
barracuda156
force-pushed
the
ppc-fix1
branch
from
f061b85
to
c658152
Compare
| @@ -3148,8 +3148,13 @@ emit_reserve_param_area (MonoCompile *cfg, guint8 *code) | |||
| if (ppc_is_imm16 (-size)) { | |||
| ppc_stwu (code, ppc_r0, -size, ppc_sp); | |||
| } else { | |||
| #if defined(__POWERPC__) | |||
There was a problem hiding this comment.
Is this needed ? Its a mips file.
| @@ -2763,6 +2763,40 @@ arch_emit_imt_trampoline (MonoAotCompile *acfg, int offset, int *tramp_size) | |||
|
|
|||
| code = buf; | |||
|
|
|||
| #if defined(TARGET_OSX) | |||
| /* Load the mscorlib got address */ | |||
There was a problem hiding this comment.
Can all these changes be simplified by defining a tmp_reg or something which is r11 on apple and r12 elsewhere ?
Feedback is welcome.
These fixes are needed, but not sufficient. However this is good enough for the first PR, I believe.
Current status of the build on macOS PPC in the issue link below.
@marek-safar This fixes errors reported in #21649
@susematz Please review r11/r12 commit. Initially I thought to make defines to switch these registers in the code, but then decided to put Darwin code into separate chunks – this will make it easier to modify it as needed without affecting anything else.
@NattyNarwhal If you could take a look too, please.