Skip to content

[LoongArch64] Add LoongArch64 support in libunwind. #62979

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Feb 15, 2022
Merged

[LoongArch64] Add LoongArch64 support in libunwind. #62979

merged 8 commits into from
Feb 15, 2022

Conversation

shushanhf
Copy link
Contributor

add the libunwind for LoongArch64 alone.

@ghost ghost added area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member labels Dec 18, 2021
@shushanhf shushanhf mentioned this pull request Dec 18, 2021
Merged
@am11
Copy link
Member

am11 commented Dec 18, 2021

@shushanhf, thank you. It looks like it is missing changes in:
https://github.com/dotnet/runtime/blob/3b2a700/src/coreclr/pal/src/libunwind/src/CMakeLists.txt
also please add a line Apply https://github.com/libunwind/libunwind/pull/316 in version file:
https://github.com/dotnet/runtime/blob/3b2a700/src/coreclr/pal/src/libunwind/libunwind-version.txt

@shushanhf
Copy link
Contributor Author

@shushanhf, thank you. It looks like it is missing changes in: https://github.com/dotnet/runtime/blob/3b2a700/src/coreclr/pal/src/libunwind/src/CMakeLists.txt also please add a line Apply https://github.com/libunwind/libunwind/pull/316 in version file: https://github.com/dotnet/runtime/blob/3b2a700/src/coreclr/pal/src/libunwind/libunwind-version.txt

Yes, I had moved it from the #62889.
Request for review again.
Thanks

@am11
Copy link
Member

am11 commented Dec 18, 2021

@shushanhf, please update libunwind-version.txt with upstream patch link which helps us when we bump the library version. Other than that, it looks good. 👍

cc @janvorli, @jkotas

@shushanhf
Copy link
Contributor Author

@shushanhf, please update libunwind-version.txt with upstream patch link which helps us when we bump the library version. Other than that, it looks good. +1

cc @janvorli, @jkotas

I am very sorry for I forgot add the version.
Had done.

Thanks for your review again !!!

kant2002
kant2002 reviewed Dec 29, 2021
View reviewed changes
src/coreclr/pal/src/libunwind/libunwind-version.txt Outdated
@@ -5,3 +5,6 @@ Replace CMakeLists.txt, src/CMakeLists.txt, configure.cmake with .NET custom ver
Keep .NET oop directory
Reapply changes from https://github.com/dotnet/runtime/commit/1b5719c2e3dde393531eaeb5b5cde05dabeef5b8
Apply https://github.com/libunwind/libunwind/pull/317

For LoongArch64:
Apply https://github.com/libunwind/libunwind/pull/316
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also should be added Apply https://github.com/libunwind/libunwind/pull/322

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least if I undertand things correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also should be added Apply https://github.com/libunwind/libunwind/pull/322

I don't see atomic_bool changes in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed the atomic_bool changes to the upstream.
If need, I will update it within this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merged the https://github.com/libunwind/libunwind/pull/322 by db234e5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shushanhf can you please add the libunwind/libunwind#322 to the list of "Apply" changes here too?

@am11
Copy link
Member

am11 commented Dec 29, 2021

@shushanhf, as agreed previously, everything outside of libunwind directory should go in a separate PR. This PR is about updating libunwind, and not .*unwind.* in this repo (which has a vast surface area encompassing PAL, VM, JIT and beyond...). So please revert commits after e9954d4.

@shushanhf
Copy link
Contributor Author

@am11 OK, I will revert it.

qiaopengcheng added 2 commits December 29, 2021 16:03
Revert "[LoongArch64] add unwinder/loongarch64/ directory."
98b471e 
This reverts commit a8fde89.

Revert "[LoongArch64] add unwind within the `inc/clrnt.h`."
This reverts commit c5e606d.
[LoongArch64] Fixed the compiling error by
db234e5 
              libunwind/libunwind#322.
@shushanhf shushanhf mentioned this pull request Jan 7, 2022
Merged
@shushanhf
Copy link
Contributor Author

Close & reopen for trigger CI

@shushanhf shushanhf closed this Jan 8, 2022
@shushanhf shushanhf reopened this Jan 8, 2022
@mangod9
Copy link
Member

mangod9 commented Feb 7, 2022

Hi @janvorli, could you please take a look so we can merge?

janvorli
janvorli approved these changes Feb 7, 2022
View reviewed changes
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the comment, thank you!

src/coreclr/pal/src/libunwind/libunwind-version.txt Outdated
@@ -5,3 +5,6 @@ Replace CMakeLists.txt, src/CMakeLists.txt, configure.cmake with .NET custom ver
Keep .NET oop directory
Reapply changes from https://github.com/dotnet/runtime/commit/1b5719c2e3dde393531eaeb5b5cde05dabeef5b8
Apply https://github.com/libunwind/libunwind/pull/317

For LoongArch64:
Apply https://github.com/libunwind/libunwind/pull/316
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shushanhf can you please add the libunwind/libunwind#322 to the list of "Apply" changes here too?

@shushanhf
Copy link
Contributor Author

LGTM modulo the comment, thank you!
@shushanhf can you please add the libunwind/libunwind#322 to the list of "Apply" changes here too?

OK, I will add it.
Thanks

@shushanhf
Copy link
Contributor Author

Hi @janvorli, could you please take a look so we can merge?

Hi, @mangod9
I had updated the patch.
If need, I can update again.
Thanks.

@BruceForstall BruceForstall added this to the 7.0.0 milestone Feb 14, 2022
@BruceForstall
Copy link
Contributor

@janvorli @mangod9 Is this ready to merge?

@janvorli
Copy link
Member

The change looks good, I have already approved it last week with one nit that was fixed since.

@BruceForstall BruceForstall merged commit 1615de6 into dotnet:main Feb 15, 2022
BruceForstall pushed a commit that referenced this pull request Feb 16, 2022
@shushanhf
[LoongArch64] add the coreclr/unwinder/loongarch64 from the #62979. (…
f1337d9 
…#63489)

* [LoongArch64] add the `coreclr/unwinder/loongarch64` from the #62979.

* [LoongArch64] replace the `__in` with `_In_`.

* [LoongArch64] update the version of the `LICENSE description`.

* [LoongArch64] update the macro-define for crossgen2.

* [LoongArch64] amend the comment notes.

Co-authored-by: qiaopengcheng <qiaopengcheng-hf@loongson.cn>
@ghost ghost locked as resolved and limited conversation to collaborators Mar 17, 2022
@shushanhf shushanhf deleted the feature/loongarch64/libunwind branch May 11, 2022 01:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@am11 am11 am11 left review comments

@kant2002 kant2002 kant2002 left review comments

@janvorli janvorli janvorli approved these changes

Assignees
No one assigned
Labels
arch-loongarch64 area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@shushanhf @am11 @mangod9 @BruceForstall @janvorli @kant2002 @jkotas