Skip to content

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

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 6 commits into from
Feb 16, 2022
Merged

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

merged 6 commits into from
Feb 16, 2022

Conversation

shushanhf
Copy link
Contributor

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

@ghost
Copy link

ghost commented Jan 7, 2022

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 7, 2022
@shushanhf
Copy link
Contributor Author

Close & reopen for trigger CI

@shushanhf shushanhf closed this Jan 8, 2022
@shushanhf shushanhf reopened this Jan 8, 2022
@shushanhf
Copy link
Contributor Author

Close & reopen for trigger CI

1 similar comment
@shushanhf
Copy link
Contributor Author

Close & reopen for trigger CI

@shushanhf shushanhf closed this Jan 9, 2022
@shushanhf shushanhf reopened this Jan 9, 2022
@runfoapp runfoapp bot mentioned this pull request Jan 10, 2022
Closed
@BruceForstall BruceForstall added this to the 7.0.0 milestone Feb 14, 2022
@BruceForstall
Copy link
Contributor

@shushanhf #62979 Has been merged. Does this need to be updated?

Is there an unwinding specification somewhere?

@janvorli Can you please review?

@janvorli
Copy link
Member

@BruceForstall this is essentially a close clone of the same arm64 stuff, so it uses basically the same format of unwind info as arm64 (modulo the differences in register architecture).

@janvorli
Copy link
Member

@shushanhf what was the reason for replacing __in by _In_ and __out by _Out_? I am comparing your code to the original arm64 one that you've initially cloned and I've noticed this subtle difference.

@BruceForstall
Copy link
Contributor

@BruceForstall this is essentially a close clone of the same arm64 stuff, so it uses basically the same format of unwind info as arm64 (modulo the differences in register architecture).

Yes, the JIT unwind code implementation is basically a raw copy of the arm64 code right now.

janvorli
janvorli reviewed Feb 16, 2022
View reviewed changes
src/coreclr/unwinder/loongarch64/unwinder_loongarch64.cpp Outdated
ContextRecord->Sp += 16 * (CurCode & 0x1f);
}

#if 0
Copy link
Member

Choose a reason for hiding this comment

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

Is any of the code under #if 0 going to be used in the future? If not, it would be better to remove all the stuff under #if 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will delete them.
If needed in future, I will add them back.

Thanks

src/coreclr/unwinder/loongarch64/unwinder_loongarch64.cpp Outdated
function, as an offset relative to the IamgeBase.

FunctionEntry - Supplies the address of the function table entry for the
specified function. If appropriate, this should have araeady been
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
specified function. If appropriate, this should have araeady been
specified function. If appropriate, this should have already been

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

src/coreclr/unwinder/loongarch64/unwinder_loongarch64.cpp Outdated
function being unwound.

FunctionEntry - Supplies the address of the function table entry for the
specified function. If appropriate, this should have araeady been
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
specified function. If appropriate, this should have araeady been
specified function. If appropriate, this should have already been

src/coreclr/unwinder/loongarch64/unwinder_loongarch64.cpp Outdated

SpOffset - Specifies a stack offset. Positive values are simply used
as a base offset. Negative values assume a predecrement behavior:
a 0 offset is used for restoration, but the absoute value of the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
a 0 offset is used for restoration, but the absoute value of the
a 0 offset is used for restoration, but the absolute value of the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

src/coreclr/unwinder/loongarch64/unwinder_loongarch64.cpp Outdated
FRegOpcodes = (FloatSize + 8) / 16;
HOpcodes = 4 * HBit;
StackAdjustOpcodes = (Cr == 3) ? 1 : 0;
if (Cr != 3 || LocalSize > 512) {///TODO for LOONGARCH64: 512 is smaller than real!
Copy link
Member

Choose a reason for hiding this comment

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

Should this TODO be handled 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.

OK, I will amend it.
Thanks

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 will just delete the comment TODO.
Here the 512 is just the JIT unwind info rather than the ISA's info.
Now the LoongArch64's implementation for JIT-unwind is referenced the arm64, althought we can redesign based on the LoongArch64's ISA which might be better.

src/coreclr/unwinder/loongarch64/unwinder_loongarch64.cpp Outdated
FRegOpcodes = (FloatSize + 8) / 16;
HOpcodes = HBit;
StackAdjustOpcodes = (Cr == 3) ? 1 : 0;
if (Cr != 3 || LocalSize > 512) {///TODO for LOONGARCH64: 512 is smaller than real!
Copy link
Member

Choose a reason for hiding this comment

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

Should this TODO be handled 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.

OK, Thanks

src/coreclr/unwinder/loongarch64/unwinder_loongarch64.cpp Outdated

CurrentOffset = 0;
if (Cr == 3) {
if (LocalSize <= 512) {///TODO for LOONGARCH64: 512 is smaller!
Copy link
Member

Choose a reason for hiding this comment

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

Should this TODO be handled 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.

OK, thanks

src/coreclr/unwinder/loongarch64/unwinder_loongarch64.cpp Outdated
}
}
while (LocalSize != 0) {
Count = (LocalSize + 4087) % 4088 + 1;///TODO: should amend the LOONGARCH64 !
Copy link
Member

Choose a reason for hiding this comment

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

Should this TODO be handled 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.

ok, thanks

@shushanhf
Copy link
Contributor Author

shushanhf commented Feb 16, 2022 •
edited
Loading

@shushanhf what was the reason for replacing __in by _In_ and __out by _Out_? I am comparing your code to the original arm64 one that you've initially cloned and I've noticed this subtle difference.

When I cloned it's __in, but it had been changed by commit 9472bfe afterwards.
If I update from the latest main branch, I have to modify it for LoongArch64.
Thanks

@janvorli
Copy link
Member

When I cloned it's __in, but it had been changed

Ah, I see, I was looking at an older state of the repo by accident, please ignore that comment then.

janvorli
janvorli approved these changes Feb 16, 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, thank you!

@BruceForstall BruceForstall merged commit f1337d9 into dotnet:main Feb 16, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2022
@shushanhf shushanhf deleted the main_loongarch64_7 branch May 11, 2022 01:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@janvorli janvorli janvorli approved these changes

Assignees
No one assigned
Labels
arch-loongarch64 area-VM-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.
5 participants
@shushanhf @BruceForstall @janvorli @danmoseley @jkotas