[RISC-V] Add more gcdump and gcinfo code. #94219

viewizard · 2023-10-31T14:47:15Z

Part of diagnostics related changes (that should be synced with runtime) (SOS command gcinfo, ...).

CC @clamp03 @wscho77 @HJLeee @JongHeonChoi @t-mustafin @gbalykov

ghost · 2023-10-31T14:47:27Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Part of diagnostics related changes (that should be synced with runtime) (SOS command gcinfo, ...).

CC @clamp03 @wscho77 @HJLeee @JongHeonChoi @t-mustafin @gbalykov

Author:	viewizard
Assignees:	-
Labels:	`area-Diagnostics-coreclr`, `community-contribution`
Milestone:	-

src/coreclr/gcdump/gcdumpnonx86.cpp

clamp03

Thank you for your hard work. Please check my comments.

clamp03 · 2023-11-07T01:50:37Z

src/coreclr/gcinfo/gcinfodumper.cpp

+#elif defined(TARGET_RISCV64)
+    assert(!"unimplemented on RISCV64 yet");
+    BYTE* pContext = (BYTE*)pRD->pCurrentContext;


Is it implemented or unimplemented? I don't think we need to keep both codes for the future. Isn't it better to remove one? Please let me know your opinion.

Note, this is unimplemented block (and will not be implemented in this PR for sure). We stay with previous logic - assert in debug build and compile default code in release build. I can't remove any of this lines since this will broke previous logic - will not assert in debug or will not compile code in #else block.

clamp03 · 2023-11-07T01:51:12Z

src/coreclr/gcinfo/gcinfodumper.cpp

+#elif defined(TARGET_RISCV64)
+    assert(!"unimplemented on RISCV64 yet");
+                pReg = (SIZE_T*)(pContext + rgRegisters[iReg].cbContextOffset);


Please check and remove one.

Note, this is unimplemented block (and will not be implemented in this PR for sure). We stay with previous logic - assert in debug build and compile default code in release build. I can't remove any of this lines since this will broke previous logic - will not assert in debug or will not compile code in #else block.

Sorry. I cannot understand well. If the block is unimplemented, I don't understand why we need those default codes for release build? And pReg = (SIZE_T*)(pContext + rgRegisters[iReg].cbContextOffset); is necessary for only Release build? Could you explain for me?

We have 2 situation here: 1) without some of this code blocks you can't compile, since this blocks provide local variables definitions; 2) I can't guarantee that in release build, code will avoid "infinity loop" or something like that, in case we don't change some variables, that changed by initial logic;

This mean, in case someone will use diagnostics SOS command gcinfo (after this code will be synced with diagnostics repo) it should not hangs or sigsegv in release build.

clamp03 · 2023-11-07T01:52:33Z

src/coreclr/gcinfo/gcinfodumper.cpp

+#elif defined(TARGET_RISCV64)
+                    assert(!"unimplemented on RISCV64 yet");
+                    base = GC_SP_REL;


Please check and remove one.

Note, this is unimplemented block (and will not be implemented in this PR for sure). By default, I use same code here as arm/loongarch, since we will have same logic as arch with volatile registers, but could switch to default from #else block for sure.

clamp03 · 2023-11-07T01:52:41Z

src/coreclr/gcinfo/gcinfodumper.cpp

+#elif defined(TARGET_RISCV64)
+    assert(!"unimplemented on RISCV64 yet");
+        pContext = (BYTE*)pRD->pCallerContext;


Please check and remove one.

Note, this is unimplemented block (and will not be implemented in this PR for sure). We stay with previous logic - assert in debug build and compile default code in release build. I can't remove any of this lines since this will broke previous logic - will not assert in debug or will not compile code in #else block.

clamp03 · 2023-11-07T01:54:21Z

src/coreclr/gcinfo/gcinfodumper.cpp

+    FILL_REGS(pCurrentContext->R0, 33);
+    FILL_REGS(pCallerContext->R0, 33);


I think it is 32 because you removed PC. And please do not force-push. I want to check the number is updated when you remove PC. However, I cannot see your previous commits. Please add commits not force-push. Thank you.

I want to check the number is updated when you remove PC.

We stay with PC here since all other architectures have this logic.
Note, I removed PC only from switch in another source, not really connected to this code logic.

However, I cannot see your previous commits.

The only changes I did is:
https://github.com/dotnet/runtime/compare/ebfbce12b56e40f36c197729700479e2f276eebb..ea435f756d5bf3dc01ded976da44cb00c5ac91e7

I see. Thank you!

clamp03 · 2023-11-08T01:09:05Z

@jkotas Could you take a look at this PR? Thank you.

jkotas · 2023-11-08T01:48:29Z

SOS command gcinfo,

How well does it work with so many TODOs remaining?

jkotas · 2023-11-08T01:46:29Z

src/coreclr/gcinfo/gcinfodumper.cpp

 #endif

 #if defined(TARGET_ARM) || defined(TARGET_ARM64)
    BYTE* pContext = (BYTE*)&(pRD->volatileCurrContextPointers);
 #elif defined(TARGET_LOONGARCH64)
    assert(!"unimplemented on LOONGARCH yet");
    BYTE* pContext = (BYTE*)pRD->pCurrentContext;
+#elif defined(TARGET_RISCV64)
+    assert(!"unimplemented on RISCV64 yet");
+    // TODO implement risc-v code, that should care about volatile registers (same as arm/arm64 architectures)


Add || defined(TARGET_RISCV64) to the ARM / ARM64 case above?

I am not sure we could change pointer to another size structure here and after this operate with offsets for another structure size in code below.

viewizard · 2023-11-08T08:18:33Z

How well does it work with so many TODOs remaining?

In case of release build it don't show or show wrong (at least I think so) related part of output (last part with registers related block).

viewizard · 2023-11-08T09:00:37Z

Correction
This PR don't change SOS gcinfo command at all, since this changes should be synced into diagnostics repo and SOS plugin should be rebuild with this code.

dotnet-issue-labeler bot added the area-Diagnostics-coreclr label Oct 31, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 31, 2023

This was referenced Oct 31, 2023

Timeout in System.Net.Quic.Functional.Tests #86019

Closed

CI error: System.Net.Quic.QuicException: The connection timed out from inactivity #91757

Closed

HJLeee approved these changes Oct 31, 2023

View reviewed changes

wscho77 approved these changes Oct 31, 2023

View reviewed changes

clamp03 assigned viewizard Nov 1, 2023

clamp03 added the arch-riscv Related to the RISC-V architecture label Nov 1, 2023

gbalykov approved these changes Nov 1, 2023

View reviewed changes

src/coreclr/gcdump/gcdumpnonx86.cpp Outdated Show resolved Hide resolved

[RISC-V] Add more gcdump and gcinfo code.

ea435f7

viewizard force-pushed the riscv64 branch from ebfbce1 to ea435f7 Compare November 1, 2023 09:49

clamp03 suggested changes Nov 7, 2023

View reviewed changes

Add more comments.

8a61666

gbalykov approved these changes Nov 7, 2023

View reviewed changes

HJLeee approved these changes Nov 7, 2023

View reviewed changes

wscho77 approved these changes Nov 7, 2023

View reviewed changes

clamp03 approved these changes Nov 8, 2023

View reviewed changes

clamp03 requested a review from jkotas November 8, 2023 01:07

jkotas reviewed Nov 8, 2023

View reviewed changes

JongHeonChoi approved these changes Nov 9, 2023

View reviewed changes

jkotas approved these changes Nov 10, 2023

View reviewed changes

jkotas merged commit 3c5123e into dotnet:main Nov 10, 2023

viewizard deleted the riscv64 branch November 10, 2023 08:19

github-actions bot locked and limited conversation to collaborators Dec 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISC-V] Add more gcdump and gcinfo code. #94219

[RISC-V] Add more gcdump and gcinfo code. #94219

viewizard commented Oct 31, 2023

ghost commented Oct 31, 2023

clamp03 left a comment

clamp03 Nov 7, 2023

viewizard Nov 7, 2023

clamp03 Nov 7, 2023

viewizard Nov 7, 2023

clamp03 Nov 7, 2023

viewizard Nov 7, 2023

clamp03 Nov 7, 2023

viewizard Nov 7, 2023

clamp03 Nov 7, 2023

viewizard Nov 7, 2023

clamp03 Nov 7, 2023

viewizard Nov 7, 2023

clamp03 Nov 7, 2023

clamp03 commented Nov 8, 2023

jkotas commented Nov 8, 2023

jkotas Nov 8, 2023

viewizard Nov 8, 2023

viewizard commented Nov 8, 2023

viewizard commented Nov 8, 2023 •

edited

Loading

		FILL_REGS(pCurrentContext->R0, 33);
		FILL_REGS(pCallerContext->R0, 33);

[RISC-V] Add more gcdump and gcinfo code. #94219

[RISC-V] Add more gcdump and gcinfo code. #94219

Conversation

viewizard commented Oct 31, 2023

ghost commented Oct 31, 2023

clamp03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamp03 commented Nov 8, 2023

jkotas commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viewizard commented Nov 8, 2023

viewizard commented Nov 8, 2023 • edited Loading

viewizard commented Nov 8, 2023 •

edited

Loading