Skip to content

Conversation

@filipnavara
Copy link
Member

Mark labels in x64 write barrier code as local/alt_entry to prevent incorrect transformations by the ld64/ld-prime linkers.

Contributes to #119174

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 1, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 1, 2025
@filipnavara filipnavara added area-VM-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 1, 2025
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@filipnavara filipnavara marked this pull request as ready for review September 1, 2025 19:09
@filipnavara filipnavara requested a review from janvorli September 1, 2025 21:50
@filipnavara
Copy link
Member Author

filipnavara commented Sep 2, 2025

I am not sure if the failures are new to the PR or introduced by something in main in the meantime...

The symbols in my local build look okay (nm shows just the addresses, I verified the flags in the object files separately):

nm /Users/filipnavara/Projects/runtime/artifacts/bin/coreclr/osx.x64.Debug/./libcoreclr.dylib | grep "_JIT_WriteBarrier\|_JIT_PatchedCode\|_JIT_CheckedWriteBarrier" | sort
000000000065cdb0 t _JIT_WriteBarrierEnsureNonHeapTarget
00000000008d9410 t _JIT_WriteBarrier_Callable
00000000008d9470 t _JIT_WriteBarrier_PreGrow64
00000000008d9476 t _JIT_WriteBarrier_PreGrow64_Patch_Label_Lower
00000000008d9486 t _JIT_WriteBarrier_PreGrow64_Patch_Label_CardTable
00000000008d94a6 t _JIT_WriteBarrier_PreGrow64_Patch_Label_CardBundleTable
00000000008d94d2 t _JIT_WriteBarrier_PreGrow64_End
00000000008d94e0 t _JIT_WriteBarrier_PostGrow64
00000000008d94e6 t _JIT_WriteBarrier_PostGrow64_Patch_Label_Lower
00000000008d94f6 t _JIT_WriteBarrier_PostGrow64_Patch_Label_Upper
00000000008d9506 t _JIT_WriteBarrier_PostGrow64_Patch_Label_CardTable
00000000008d9526 t _JIT_WriteBarrier_PostGrow64_Patch_Label_CardBundleTable
00000000008d9552 t _JIT_WriteBarrier_PostGrow64_End
00000000008d9560 t _JIT_WriteBarrier_SVR64
00000000008d9566 t _JIT_WriteBarrier_SVR64_PatchLabel_CardTable
00000000008d9586 t _JIT_WriteBarrier_SVR64_PatchLabel_CardBundleTable
00000000008d95a1 t _JIT_WriteBarrier_SVR64_End
00000000008d95b0 t _JIT_WriteBarrier_Byte_Region64
00000000008d95b6 t _JIT_WriteBarrier_Byte_Region64_Patch_Label_RegionToGeneration
00000000008d95c0 t _JIT_WriteBarrier_Byte_Region64_Patch_Label_RegionShrDest
00000000008d95ce t _JIT_WriteBarrier_Byte_Region64_Patch_Label_Lower
00000000008d95de t _JIT_WriteBarrier_Byte_Region64_Patch_Label_Upper
00000000008d95ef t _JIT_WriteBarrier_Byte_Region64_Patch_Label_RegionShrSrc
00000000008d95fe t _JIT_WriteBarrier_Byte_Region64_Patch_Label_CardTable
00000000008d961e t _JIT_WriteBarrier_Byte_Region64_Patch_Label_CardBundleTable
00000000008d9637 t _JIT_WriteBarrier_Byte_Region64_End
00000000008d9640 t _JIT_WriteBarrier_Bit_Region64
00000000008d9646 t _JIT_WriteBarrier_Bit_Region64_Patch_Label_RegionToGeneration
00000000008d9650 t _JIT_WriteBarrier_Bit_Region64_Patch_Label_RegionShrDest
00000000008d965e t _JIT_WriteBarrier_Bit_Region64_Patch_Label_Lower
00000000008d966e t _JIT_WriteBarrier_Bit_Region64_Patch_Label_Upper
00000000008d967f t _JIT_WriteBarrier_Bit_Region64_Patch_Label_RegionShrSrc
00000000008d968e t _JIT_WriteBarrier_Bit_Region64_Patch_Label_CardTable
00000000008d96b6 t _JIT_WriteBarrier_Bit_Region64_Patch_Label_CardBundleTable
00000000008d96d3 t _JIT_WriteBarrier_Bit_Region64_End
00000000008d96e0 t _JIT_WriteBarrier_WriteWatch_PreGrow64
00000000008d96e6 t _JIT_WriteBarrier_WriteWatch_PreGrow64_Patch_Label_WriteWatchTable
00000000008d96f6 t _JIT_WriteBarrier_WriteWatch_PreGrow64_Patch_Label_Lower
00000000008d9716 t _JIT_WriteBarrier_WriteWatch_PreGrow64_Patch_Label_CardTable
00000000008d972e t _JIT_WriteBarrier_WriteWatch_PreGrow64_Patch_Label_CardBundleTable
00000000008d9752 t _JIT_WriteBarrier_WriteWatch_PreGrow64_End
00000000008d9760 t _JIT_WriteBarrier_WriteWatch_PostGrow64
00000000008d9766 t _JIT_WriteBarrier_WriteWatch_PostGrow64_Patch_Label_WriteWatchTable
00000000008d9776 t _JIT_WriteBarrier_WriteWatch_PostGrow64_Patch_Label_Lower
00000000008d9796 t _JIT_WriteBarrier_WriteWatch_PostGrow64_Patch_Label_Upper
00000000008d97a6 t _JIT_WriteBarrier_WriteWatch_PostGrow64_Patch_Label_CardTable
00000000008d97c6 t _JIT_WriteBarrier_WriteWatch_PostGrow64_Patch_Label_CardBundleTable
00000000008d97e2 t _JIT_WriteBarrier_WriteWatch_PostGrow64_End
00000000008d97f0 t _JIT_WriteBarrier_WriteWatch_SVR64
00000000008d97f6 t _JIT_WriteBarrier_WriteWatch_SVR64_PatchLabel_WriteWatchTable
00000000008d9806 t _JIT_WriteBarrier_WriteWatch_SVR64_PatchLabel_CardTable
00000000008d982e t _JIT_WriteBarrier_WriteWatch_SVR64_PatchLabel_CardBundleTable
00000000008d984b t _JIT_WriteBarrier_WriteWatch_SVR64_End
00000000008d9850 t _JIT_WriteBarrier_WriteWatch_Byte_Region64
00000000008d9856 t _JIT_WriteBarrier_WriteWatch_Byte_Region64_Patch_Label_WriteWatchTable
00000000008d986a t _JIT_WriteBarrier_WriteWatch_Byte_Region64_Patch_Label_RegionShrDest
00000000008d9876 t _JIT_WriteBarrier_WriteWatch_Byte_Region64_Patch_Label_RegionToGeneration
00000000008d988e t _JIT_WriteBarrier_WriteWatch_Byte_Region64_Patch_Label_Lower
00000000008d989e t _JIT_WriteBarrier_WriteWatch_Byte_Region64_Patch_Label_Upper
00000000008d98af t _JIT_WriteBarrier_WriteWatch_Byte_Region64_Patch_Label_RegionShrSrc
00000000008d98be t _JIT_WriteBarrier_WriteWatch_Byte_Region64_Patch_Label_CardTable
00000000008d98de t _JIT_WriteBarrier_WriteWatch_Byte_Region64_Patch_Label_CardBundleTable
00000000008d98f7 t _JIT_WriteBarrier_WriteWatch_Byte_Region64_End
00000000008d9900 t _JIT_WriteBarrier_WriteWatch_Bit_Region64
00000000008d9906 t _JIT_WriteBarrier_WriteWatch_Bit_Region64_Patch_Label_WriteWatchTable
00000000008d991a t _JIT_WriteBarrier_WriteWatch_Bit_Region64_Patch_Label_RegionShrDest
00000000008d9926 t _JIT_WriteBarrier_WriteWatch_Bit_Region64_Patch_Label_RegionToGeneration
00000000008d993e t _JIT_WriteBarrier_WriteWatch_Bit_Region64_Patch_Label_Lower
00000000008d994e t _JIT_WriteBarrier_WriteWatch_Bit_Region64_Patch_Label_Upper
00000000008d995f t _JIT_WriteBarrier_WriteWatch_Bit_Region64_Patch_Label_RegionShrSrc
00000000008d996e t _JIT_WriteBarrier_WriteWatch_Bit_Region64_Patch_Label_CardTable
00000000008d9996 t _JIT_WriteBarrier_WriteWatch_Bit_Region64_Patch_Label_CardBundleTable
00000000008d99b3 t _JIT_WriteBarrier_WriteWatch_Bit_Region64_End
00000000008d9a00 t _JIT_WriteBarrier_Debug
00000000008d9b02 t _JIT_WriteBarrier_Debug_End
00000000008d9b10 t _JIT_PatchedCodeStart
00000000008d9b20 t _JIT_CheckedWriteBarrier
00000000008d9b42 t _JIT_CheckedWriteBarrier_End
00000000008d9b50 t _JIT_WriteBarrier
00000000008d9c21 t _JIT_WriteBarrier_End
00000000008d9c30 t _JIT_PatchedCodeLast
0000000000afea28 s _JIT_WriteBarrier_Loc

The "rep retq" sequence also seems to be at the correct spot:

00000000008d9750 <_Exit_WriteWatch_PreGrow64>:
  8d9750: f3 c3                        	rep		retq

@filipnavara
Copy link
Member Author

filipnavara commented Sep 2, 2025

I will check the unwinding information. Not sure if that one is correct now for the patched code, and that's most likely thing to get broken.

The x64 code in patchedcode.S never produced the unwinding info, and the arm64 continues to produce the same empty unwinding info codes, so this seems to be fine.

@filipnavara filipnavara marked this pull request as draft September 3, 2025 10:31
@filipnavara
Copy link
Member Author

filipnavara commented Sep 3, 2025

I downloaded the failing payload from Helix and it has broken unwind info (it gets split on .alt_entry and creates bogus 0x00000000 unwind code):

                        [691] funcOffset=0x00582F80, encoding[  0]=0x02000000 (no frame, no saved registers                            ) _JIT_PatchedCodeStart
                        [692] funcOffset=0x00582F84, encoding[ 12]=0x00000000 (no frame, no saved registers                            ) _JIT_ByRefWriteBarrier

This is not happening for my local builds because I use Xcode 26 beta 6 which already fixed this particular bug (#119005). I'll try to workaround it.

@janvorli
Copy link
Member

janvorli commented Sep 8, 2025

@filipnavara it seems to me that the option 2 you've suggested would be a reasonable fix for now.

@filipnavara
Copy link
Member Author

I'll look into it once I get back to my Mac. I think we just need a right place to put something like this:

  if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND
     CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 15 AND
     CMAKE_CXX_COMPILER_VERSION VERSION_LESS 16)
    add_link_options("-Wl,-ld_classic")
  endif()

@janvorli
Copy link
Member

janvorli commented Sep 8, 2025

@filipnavara
Copy link
Member Author

I was going to put it in configurecompiler.cmake. Just need to perform the tests with couple of Xcode versions to verify the behavior.

@filipnavara filipnavara marked this pull request as ready for review September 8, 2025 15:54
@agocke
Copy link
Member

agocke commented Sep 17, 2025

Now that xcode 26 is released, can we rely on "upgrade to xcode 26" as the solution going forward? Or is this PR still necessary?

@akoeplinger
Copy link
Member

we don't control when AzDO/GitHub updates Xcode on the mac machines, and afaik the problem is with the new linker in Xcode 16+ so it presumably has the same problem in Xcode 26

@filipnavara
Copy link
Member Author

The originally reported problem affects all Xcodes with new linker, so presumably anything with Xcode 16+, including Xcode 26. It may or may not be triggered in a specific build depending on what layout and dead code elimination the linker decides to use. The added .alt_entry flags force the linker to always do the correct thing.

Unfortunately, there're numerous disjoint bugs in support for .alt_entry in the Xcode 15 ld-prime [new] linker and the ld-classic linker. Essentially any serious usage of .alt_entry in Xcode 15 ld-prime will produce corrupted output, often in suble ways like corrupting only unwind information that gets used for exception, stack traces and GC. The only serious issue with .alt_entry in ld-classic is that it misbehaves when it's a symbol in the object file with the same address as the beginning of a section. More generally, the issues in ld-classic are limited to overlapping symbols with same address. As long as the .alt_entry is used with address higher than the function start it always behaves correctly.

In summary, .alt_entry is unusable in Xcode 15 with ld-prime and carefully usable with ld-classic. With Xcode 16 there are bugs with ld-prime, .alt_entry and unwind info but they are workaroundable (#119005). Xcode 26 fixes all the bugs I reported in this specific area. This PR aims to apply workarounds for Xcode <26 but also do the right thing going forward.

@agocke
Copy link
Member

agocke commented Sep 17, 2025

Thanks! I'm interested in anything we can do to make this problem more manageable in the future.

@akoeplinger
Copy link
Member

I think we also need to at least backport the switch to ld_classic to servicing branches since we won't be able to stay on older macOS for very long

@agocke agocke requested a review from jkoritzinsky October 1, 2025 18:28
@agocke
Copy link
Member

agocke commented Oct 1, 2025

@janvorli Anything else here?

@agocke agocke requested a review from davidwrighton October 1, 2025 19:57
@janvorli
Copy link
Member

janvorli commented Oct 1, 2025

@agocke it looks good to me as is

@agocke agocke merged commit 974f9af into dotnet:main Oct 2, 2025
154 checks passed
@agocke
Copy link
Member

agocke commented Oct 20, 2025

/backport to release/10.0

@github-actions
Copy link
Contributor

@agocke
Copy link
Member

agocke commented Oct 20, 2025

/backport to release/9.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/18666818024

@agocke
Copy link
Member

agocke commented Oct 20, 2025

/backport to release/8.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/18666828338

@github-actions
Copy link
Contributor

@agocke backporting to "release/9.0-staging" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Mark labels in x64 write barrier code as local/alt_entry to prevent incorrect transformations by the ld64/ld-prime linkers
Applying: Fix the LEAF_END_MARKED macro for Apple platforms too
Applying: Mark all labels inside JIT_PatchedCode region as .alt_entry on Apple platforms
Using index info to reconstruct a base tree...
M	src/coreclr/pal/inc/unixasmmacrosamd64.inc
M	src/coreclr/pal/inc/unixasmmacrosarm64.inc
M	src/coreclr/vm/arm64/patchedcode.S
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/pal/inc/unixasmmacrosamd64.inc
Auto-merging src/coreclr/pal/inc/unixasmmacrosarm64.inc
Auto-merging src/coreclr/vm/arm64/patchedcode.S
CONFLICT (content): Merge conflict in src/coreclr/vm/arm64/patchedcode.S
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0003 Mark all labels inside JIT_PatchedCode region as .alt_entry on Apple platforms
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@agocke backporting to "release/8.0-staging" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Mark labels in x64 write barrier code as local/alt_entry to prevent incorrect transformations by the ld64/ld-prime linkers
Applying: Fix the LEAF_END_MARKED macro for Apple platforms too
Applying: Mark all labels inside JIT_PatchedCode region as .alt_entry on Apple platforms
Using index info to reconstruct a base tree...
M	src/coreclr/pal/inc/unixasmmacrosamd64.inc
M	src/coreclr/pal/inc/unixasmmacrosarm64.inc
A	src/coreclr/vm/amd64/patchedcode.S
A	src/coreclr/vm/arm64/patchedcode.S
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/pal/inc/unixasmmacrosamd64.inc
Auto-merging src/coreclr/pal/inc/unixasmmacrosarm64.inc
CONFLICT (modify/delete): src/coreclr/vm/amd64/patchedcode.S deleted in HEAD and modified in Mark all labels inside JIT_PatchedCode region as .alt_entry on Apple platforms.  Version Mark all labels inside JIT_PatchedCode region as .alt_entry on Apple platforms of src/coreclr/vm/amd64/patchedcode.S left in tree.
CONFLICT (modify/delete): src/coreclr/vm/arm64/patchedcode.S deleted in HEAD and modified in Mark all labels inside JIT_PatchedCode region as .alt_entry on Apple platforms.  Version Mark all labels inside JIT_PatchedCode region as .alt_entry on Apple platforms of src/coreclr/vm/arm64/patchedcode.S left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0003 Mark all labels inside JIT_PatchedCode region as .alt_entry on Apple platforms
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

agocke pushed a commit that referenced this pull request Oct 22, 2025
…ry (#120923)

Backport of #119245 to release/10.0

/cc @agocke @filipnavara

## Customer Impact

- [ ] Customer reported
- [X] Found internally

[Select one or both of the boxes. Describe how this issue impacts
customers, citing the expected and actual behaviors and scope of the
issue. If customer-reported, provide the issue number.]

## Regression

- [X] Yes
- [ ] No

This is a regression in the new Apple toolset.

## Testing

Tested manually, and in macios CI.

## Risk

Medium, product change coming somewhat late. However, this change has
been running in the `main` branch for a couple weeks with no issue.

---------

Co-authored-by: Filip Navara <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-VM-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants