-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Update llvm-libunwind to v18 #102231
Update llvm-libunwind to v18 #102231
Conversation
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
96ca45b
to
74033b9
Compare
if(CLR_CMAKE_TARGET_APPLE) | ||
set(LLVM_LIBUNWIND_SOURCES_BASE | ||
${LLVM_LIBUNWIND_SOURCES_BASE} | ||
src/Unwind_AppleExtras.cpp |
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.
It was deleted in llvm/llvm-project@1ae57fe.
If these patches are getting difficult to update, are there subsets we can upstream to make it easier/slim down the diff? |
That would be nice; #72655. 😅 |
I think this change could use running additional legs for AOT testing. |
ldp x18,x19, [x0, #0x090] | ||
ldp x20,x21, [x0, #0x0A0] | ||
ldp x22,x23, [x0, #0x0B0] | ||
ldp x24,x25, [x0, #0x0C0] | ||
ldp x26,x27, [x0, #0x0D0] | ||
ldp x28,x29, [x0, #0x0E0] | ||
ldr x30, [x0, #0x100] // restore pc into lr | ||
ldr x1, [x0, #0x0F8] |
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.
Just a note to fix it in a separate PR.
This change is actually problematic. Loading SP early may cause corruption of the source context in case it is located on stack and an async signal fires. See #101709. We will need to do something similar to how it was fixed in the linked issue if we need to restore x16 too.
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.
@janvorli, thanks. In case you missed it, it's the first commit which put v18 code, then second commit applied our patches which undid this change. So in the current state of the PR, ti's not available. Would be nice to sync it with upstream at some point because they touched nearby code in recent updates.
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.
I understand, I meant that our state was problematic and that the V18 way is correct, except that it trashes x16. But I have missed the comment above that says that trashing x16 is ok. So maybe we should take the new code here?
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.
Sure, we can try it in a follow-up PR. Seems like it was like that from the beginning of the port: https://github.com/dotnet/corert/blame/c6af4cfc8b625851b91823d9be746c4f7abdc667/src/Native/libunwind/src/UnwindRegistersRestore.S#L585 and we diverged from upstream in llvm 9 timeframe llvm/llvm-project@d5323f6 (a change we never picked).
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.
Right, I guess we have just missed that change and skipping it was not intentional.
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Timeouts look unrelated. Perhaps one more outerloop run will help validating the update? 👀 |
I can see that Threading NativeAOT tests are failing, which was fixed last week. Let me close and reopen the PR to pick the fix to this issue and rerun the outerloop. |
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
In the history tabs of those failing tests in AzDo, they seem to have intermittently failed before on main as well, so I'm not completely sure if they are related to this version bump. |
linux-arm failure is being fixed by #102760 and System.IO tests on linux-x64 are also failing on main. e..g. https://dev.azure.com/dnceng-public/public/_build/results?buildId=689281&view=logs&j=20e6f673-0b0c-5c85-377c-5d941d4c8ee3&t=5801e1cc-59c5-5e47-8484-142b970afd58
Opened #102800 for linux-x64. |
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.
LGTM, thank you!
* Update llvm-libunwind to v18 * Apply all patches * Update version file
First commit: deleted
llvm-libunwind/
and downloaded fresh copy from v18.1.5 tree.Second commit: apply all our local patches.
Third commit: updated the version file (noting second commit).
These local patches are growing, making it a bit tricky to update.