- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
Write barrier without any RWX pages #114982
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bfd57cd
              02b9ea6
              273b640
              17ad508
              4669265
              a24a67e
              a858993
              1c4c79a
              999b466
              547fb96
              9fd2921
              9c2e247
              4601ff8
              ff8a695
              a882fbf
              f5a07a7
              45a866b
              516656e
              2b791c6
              ecc06a7
              e8c9a34
              e992b86
              db5d24f
              f3f9320
              c35cbbd
              0fa2885
              89422b6
              fd2cc83
              4dd7420
              5ab9e76
              9467e25
              4e1ee53
              3383a68
              6ca2479
              435c220
              2950b1f
              247ae5f
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|  | ||
| // This file is used to allow sharing of assembly code between NativeAOT and CoreCLR, which have different conventions about how to ensure that constants offsets are accessible | ||
|  | ||
| #include "AsmOffsets.inc" | ||
| #include <unixasmmacros.inc> | 
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|  | ||
| // This file is used to allow sharing of assembly code between NativeAOT and CoreCLR, which have different conventions about how to ensure that constants offsets are accessible | ||
|  | ||
| // TODO: Implement | 
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| ; Licensed to the .NET Foundation under one or more agreements. | ||
| ; The .NET Foundation licenses this file to you under the MIT license. | ||
|  | ||
| ; This file is used to allow sharing of assembly code between NativeAOT and CoreCLR, which have different conventions about how to ensure that constants offsets are accessible | ||
|  | ||
| include AsmMacros.inc | 
This file was deleted.
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|  | ||
| // This file is used to allow sharing of assembly code between NativeAOT and CoreCLR, which have different conventions about how to ensure that constants offsets are accessible | ||
|  | ||
| #include <unixasmmacros.inc> | ||
| #include "AsmOffsets.inc" | 
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|  | ||
| // This file is used to allow sharing of assembly code between NativeAOT and CoreCLR, which have different conventions about how to ensure that constants offsets are accessible | ||
|  | ||
| #include <unixasmmacros.inc> | ||
| #include "AsmOffsets.inc" | 
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|  | @@ -2,7 +2,15 @@ | |||||||||||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||||||||||||
|  | ||||||||||||||||||
| .intel_syntax noprefix | ||||||||||||||||||
| #include <unixasmmacros.inc> | ||||||||||||||||||
| #include "AsmMacros_Shared.h" | ||||||||||||||||||
|  | ||||||||||||||||||
| #if defined(__APPLE__) | ||||||||||||||||||
| // Currently the build is failing without this due to an issue if the first method in the assembly file has an alternate entry at the start of the file. | ||||||||||||||||||
| // Fix, but adding an empty, unused method | ||||||||||||||||||
| LEAF_ENTRY RhpWriteBarriersDoNotFailToBuild, _TEXT | ||||||||||||||||||
| ret | ||||||||||||||||||
| LEAF_END RhpWriteBarriersDoNotFailToBuild, _TEXT | ||||||||||||||||||
| #endif | ||||||||||||||||||
|  | ||||||||||||||||||
| #ifdef WRITE_BARRIER_CHECK | ||||||||||||||||||
|  | ||||||||||||||||||
|  | @@ -261,7 +269,15 @@ LEAF_END RhpCheckedXchg, _TEXT | |||||||||||||||||
| LEAF_ENTRY RhpByRefAssignRef, _TEXT | ||||||||||||||||||
| ALTERNATE_ENTRY RhpByRefAssignRefAVLocation1 | ||||||||||||||||||
| mov rcx, [rsi] | ||||||||||||||||||
| #ifdef TARGET_APPLE | ||||||||||||||||||
| // Apple's linker has issues which break unwind info if | ||||||||||||||||||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the manifestation of the bad unwind info? If it is just when single stepping in lldb, do we care? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The manifestation is that if we AV on this location, we are unable to unwind to the managed frame, so we failfast the process instead of producing a NullReferenceException which can be caught. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be better to use the same manual uwnind routine in regular CoreCLR as we use in NAOT to avoid this problem? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is another approach where we don't use libunwind for unwinding these entries, instead we use our own unwinder which is special cased to know what to do. (This is what NativeAot does.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Unlike NativeAOT, CoreCLR implements the Apple compact unwinding through a modified copy of the llvm-libunwind code; libunwind should not even come into play for this since it's covered by the compact codes.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I was doing fixes of the osx-arm64 NativeAOT port I found a bug in llvm-libunwind comparison of the boundary comparison for IP addresses and function start/end (cb6fb60#diff-fa7a2ccbf98ccd844bfc21e48f2bc700137c2903cf97d487fa011e16b1f9b3c0). I looked at the custom unwinding code in CoreCLR and there are similar comparisons that look sketchy: runtime/src/coreclr/pal/src/exception/remote-unwind.cpp Lines 1034 to 1037 in c201ad1 
 runtime/src/coreclr/pal/src/exception/remote-unwind.cpp Lines 1064 to 1067 in c201ad1 
 (in both cases the condition should presumably be ip >= funcEnd, notip > funcEnd; shouldn't really cause an error for alt_entry though..) | ||||||||||||||||||
| // an ALTERNATE_ENTRY is present in the middle of a function see https://github.com/dotnet/runtime/pull/114982#discussion_r2083272768 | ||||||||||||||||||
| .cfi_endproc | ||||||||||||||||||
| #endif | ||||||||||||||||||
| ALTERNATE_ENTRY RhpByRefAssignRefAVLocation2 | ||||||||||||||||||
| #ifdef TARGET_APPLE | ||||||||||||||||||
| .cfi_startproc | ||||||||||||||||||
| #endif | ||||||||||||||||||
| mov [rdi], rcx | ||||||||||||||||||
|  | ||||||||||||||||||
| // Check whether the writes were even into the heap. If not there's no card update required. | ||||||||||||||||||
|  | ||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not failing in NativeAOT build today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I look at it, this probably isn't necessary in the amd64 write barrier since it wasn't complaining before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leads me to wonder if maybe the issue on MacOS arm64 with having an alternate entry as the first symbol in the object file was that since our alternate entry macro doesn't set the thumb bit, the offset for the alternate entry was before that of the normal function symbol for the first symbol in the object file. Time for an experiment. Sigh.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thumb bit is only relevant for arm32 so that should not matter here.
Also, the issue with first symbol in object file is known ld64 bug, see #114982 (comment).