Skip to content

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
@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 !!!

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
This reverts commit a8fde89.

Revert "[LoongArch64] add unwind within the `inc/clrnt.h`."
This reverts commit c5e606d.
@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?

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!

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
…#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 <[email protected]>
@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.

Labels

arch-loongarch64 area-PAL-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.

7 participants