Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

am11
Copy link
Member

@am11 am11 commented May 14, 2024

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.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 14, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 14, 2024
@am11 am11 added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 14, 2024
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@am11 am11 force-pushed the feature/vendor/llvm-libunwind-18 branch from 96ca45b to 74033b9 Compare May 14, 2024 23:16
if(CLR_CMAKE_TARGET_APPLE)
set(LLVM_LIBUNWIND_SOURCES_BASE
${LLVM_LIBUNWIND_SOURCES_BASE}
src/Unwind_AppleExtras.cpp
Copy link
Member Author

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.

@am11 am11 requested review from filipnavara and VSadov May 14, 2024 23:20
@jkoritzinsky
Copy link
Member

If these patches are getting difficult to update, are there subsets we can upstream to make it easier/slim down the diff?

@am11
Copy link
Member Author

am11 commented May 15, 2024

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 it's best if original authors send their respective patches to avoid losing context and authorship, otherwise we can open PRs on their behalf (git commit --amend --author="name <email>" --no-edit).

@am11 am11 marked this pull request as ready for review May 15, 2024 07:03
@am11 am11 closed this May 15, 2024
@am11 am11 reopened this May 15, 2024
@am11 am11 requested review from jkotas and janvorli May 16, 2024 18:49
@am11
Copy link
Member Author

am11 commented May 16, 2024

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]
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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).

Copy link
Member

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.

@jkotas
Copy link
Member

jkotas commented May 16, 2024

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@am11
Copy link
Member Author

am11 commented May 23, 2024

Timeouts look unrelated. Perhaps one more outerloop run will help validating the update? 👀

@janvorli
Copy link
Member

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.

@janvorli janvorli closed this May 23, 2024
@janvorli janvorli reopened this May 23, 2024
@janvorli
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@am11
Copy link
Member Author

am11 commented May 23, 2024

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.

@am11 am11 closed this May 28, 2024
@am11 am11 reopened this May 28, 2024
@am11
Copy link
Member Author

am11 commented May 28, 2024

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

[SKIP] System.IO.Tests.TextWriterTests.CreateBroadcasting_AllMethodsOverridden
./RunTests.sh: line 179:    21 Killed                  ./System.IO.Tests -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing -xml testResults.xml $RSP_FILE
/root/helix/work/workitem/e
----- end Tue May 28 10:50:09 UTC 2024 ----- exit code 137 ----------------------------------------------------------
exit code 137 means SIGKILL Killed either due to out of memory/resources (see /var/log/messages) or by explicit kill.
ulimit -c value: unlimited

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-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.

None yet

4 participants