[RISC-V] Implement SOS related Debugger API code. #94454

viewizard · 2023-11-07T11:26:41Z

All changes in runtime we need for diagnostic's SOS commands that use ICorDebug (clrstack -i, clrstack -i -a, ...) to work.

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

ghost · 2023-11-07T11:26:55Z

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

Issue Details

All changes in runtime we need for diagnostic's SOS commands that use ICorDebug (clrstack -i, clrstack -i -a, ...) to work.

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

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

@clamp03

Implement ICorDebug related `clrstack -i`. Implement `clrstack -r` output. Related PR in runtime: dotnet/runtime#94454 ``` > clrstack OS Thread Id: 0x40614 (0) Child SP IP Call Site 0000003FCBDD6F70 0000000000000000 [InlinedCallFrame: 0000003fcbdd6f70] Interop+Sys.<ReadStdin>g____PInvoke|44_0(Byte*, Int32) 0000003FCBDD6F70 0000003f32be5758 [InlinedCallFrame: 0000003fcbdd6f70] Interop+Sys.<ReadStdin>g____PInvoke|44_0(Byte*, Int32) 0000003FCBDD6F50 0000003F32BE5758 ILStubClass.IL_STUB_PInvoke(Byte*, Int32) 0000003FCBDD7050 0000003F32BE55BC Interop+Sys.ReadStdin(Byte*, Int32) [/home/runtime/src/libraries/System.Console/src/Microsoft.Interop.LibraryImportGenerator/Microsoft.Interop.LibraryImportGenerator/LibraryImports.g.cs @ 800] 0000003FCBDD7080 0000003F32BE5464 System.IO.StdInReader.ReadStdin(Byte*, Int32) [/home/runtime/src/libraries/System.Console/src/System/IO/StdInReader.cs @ 83] 0000003FCBDD70B0 0000003F32BE4FBC System.IO.StdInReader.ReadKey() [/home/runtime/src/libraries/System.Console/src/System/IO/StdInReader.cs @ 337] 0000003FCBDD7560 0000003F32BE3A24 System.IO.StdInReader.ReadLineCore(Boolean) [/home/runtime/src/libraries/System.Console/src/System/IO/StdInReader.cs @ 160] 0000003FCBDD7740 0000003F32BE3694 System.IO.StdInReader.ReadLine() [/home/runtime/src/libraries/System.Console/src/System/IO/StdInReader.cs @ 90] 0000003FCBDD77A0 0000003F32BE353C System.IO.SyncTextReader.ReadLine() [/home/runtime/src/libraries/System.Console/src/System/IO/SyncTextReader.cs @ 77] 0000003FCBDD77F0 0000003F32BE144C System.Console.ReadLine() [/home/runtime/src/libraries/System.Console/src/System/Console.cs @ 752] 0000003FCBDD7820 0000003F32B9DFD0 TestApp.Program.Main(System.String[]) [/home/viewizard/Desktop/projects_test/test_hr/Program.cs @ 11] ``` ``` > clrstack -i Dumping managed stack and managed variables using ICorDebug. ============================================================================= Child SP IP Call Site 0000003FCBDD6EF0 0000000000000000 [NativeStackFrame] 0000003FCBDD6F50 0000003f32be5758 0000003FCBDD6F70 (null) [Managed to Unmanaged transition: 0000003FCBDD6F70] 0000003FCBDD7050 0000003f32be55bc [DEFAULT] I4 Interop+Sys.ReadStdin(Ptr UI1,I4) (/home/mkurinnoi/dotnet/System.Console.dll) 0000003FCBDD7080 0000003f32be5464 [DEFAULT] I4 System.IO.StdInReader.ReadStdin(Ptr UI1,I4) (/home/mkurinnoi/dotnet/System.Console.dll) 0000003FCBDD70B0 0000003f32be4fbc [DEFAULT] [hasThis] ValueClass System.ConsoleKeyInfo System.IO.StdInReader.ReadKey() (/home/mkurinnoi/dotnet/System.Console.dll) 0000003FCBDD7560 0000003f32be3a24 [DEFAULT] [hasThis] Boolean System.IO.StdInReader.ReadLineCore(Boolean) (/home/mkurinnoi/dotnet/System.Console.dll) 0000003FCBDD7740 0000003f32be3694 [DEFAULT] [hasThis] String System.IO.StdInReader.ReadLine() (/home/mkurinnoi/dotnet/System.Console.dll) 0000003FCBDD77A0 0000003f32be353c [DEFAULT] [hasThis] String System.IO.SyncTextReader.ReadLine() (/home/mkurinnoi/dotnet/System.Console.dll) 0000003FCBDD77F0 0000003f32be144c [DEFAULT] String System.Console.ReadLine() (/home/mkurinnoi/dotnet/System.Console.dll) 0000003FCBDD7820 0000003f32b9dfd0 [DEFAULT] Void TestApp.Program.Main(SZArray String) (/home/mkurinnoi/test_hr.dll) 0000003FCBDD7850 0000003fb1e2307e [NativeStackFrame] Stack walk complete. ============================================================================= ``` CC @clamp03 @wscho77 @HJLeee @JongHeonChoi @t-mustafin @gbalykov

src/coreclr/inc/cordebug.idl

jkotas · 2023-11-08T11:06:01Z

src/coreclr/debug/inc/dbgipcevents.h

@@ -1171,6 +1171,41 @@ struct MSLAYOUT DebuggerREGDISPLAY
    SIZE_T  S7;
    SIZE_T  S8;
    SIZE_T  PC;
+#elif defined(TARGET_RISCV64)
+    #define DebuggerIPCE_FloatCount 32
+    SIZE_T  R0;


What is the purpose of having R0 here? It is not a real register - it should be always zero.

Looks like I miss this one, when copy registers from register context. Sure, no reason store R0 for frames at stack trace.

jkotas · 2023-11-08T11:10:01Z

src/coreclr/inc/cordebug.idl

        REGISTER_RISCV64_SP,
+        REGISTER_RISCV64_FP,
+        REGISTER_RISCV64_RA,
        REGISTER_RISCV64_GP,


There is REGISTER_RISCV64_X0 with TODO at the end of this list that does not appear to be used anywhere. Delete it while you are on it?

I am still not sure about REGISTER_RISCV64_X0, plus, I see it used here for some purpose:

runtime/src/coreclr/debug/inc/riscv64/primitives.h

Line 52 in 23695f9

REGISTER_RISCV64_X0, // TODO-RISCV64-CQ: Add X0 to access DbgReg in correct order

Note, current implementation aimed and tested only with diagnostics SOS command with simple tests, that is extremely limited and I belive not cover all cases of this code usage. So, I leave it as is for now.

I think REGISTER_RISCV64_X0 tries to be the R0 register that is not a real register, so it does not need to be in the enum.

@clamp03 could you please clarify, is X0 here used in the R0 meaning?

ICorDebugInfo::RegNum have REGNUM_R0

runtime/src/coreclr/inc/cordebuginfo.h

Line 183 in 0ad3723

REGNUM_R0,

and (looks like) REGISTER_RISCV64_X0 used just as "padding" for proper mapping with g_JITToCorDbgReg

runtime/src/coreclr/debug/inc/riscv64/primitives.h

Lines 155 to 165 in 23695f9

//

// Mapping from ICorDebugInfo register numbers to CorDebugRegister

// numbers. Note: this must match the order in corinfo.h.

//

inline CorDebugRegister ConvertRegNumToCorDebugRegister(ICorDebugInfo::RegNum reg)

{

LIMITED_METHOD_CONTRACT;

_ASSERTE(reg >= 0);

_ASSERTE(static_cast<size_t>(reg) < ARRAY_SIZE(g_JITToCorDbgReg));

return g_JITToCorDbgReg[reg];

}

I am not really strong in this code, do we really could remove R0 from ICorDebugInfo::RegNum ? Is not ICorDebugInfo::RegNum used as indexes for "normal" (not debug related) register context?

This padding is an internal implementation detail. It should not leak through the public ICorDebug surface.

Arm64 has similar wzr register and we do not have a constant for it in the ICorDebug register enum either.

Thank you for explanation, I got the idea. Will prepare appropriate changes.

jkotas · 2023-11-09T14:05:03Z

src/coreclr/debug/inc/riscv64/primitives.h

@@ -49,7 +49,7 @@ inline CORDB_ADDRESS GetPatchEndAddr(CORDB_ADDRESS patchAddr)

 constexpr CorDebugRegister g_JITToCorDbgReg[] =
 {
-    REGISTER_RISCV64_X0, // TODO-RISCV64-CQ: Add X0 to access DbgReg in correct order
+    (CorDebugRegister)(-1), // R0 is not using, but we need padding here for proper mapping with ICorDebugInfo::RegNum.


Suggested change

(CorDebugRegister)(-1), // R0 is not using, but we need padding here for proper mapping with ICorDebugInfo::RegNum.

(CorDebugRegister)(-1), // X0 is zero register that is not a real register. We need padding here for proper mapping with ICorDebugInfo::RegNum.

jkotas · 2023-11-09T14:06:05Z

src/coreclr/debug/inc/riscv64/primitives.h

@@ -49,7 +49,7 @@ inline CORDB_ADDRESS GetPatchEndAddr(CORDB_ADDRESS patchAddr)

 constexpr CorDebugRegister g_JITToCorDbgReg[] =
 {
-    REGISTER_RISCV64_X0, // TODO-RISCV64-CQ: Add X0 to access DbgReg in correct order
+    (CorDebugRegister)(-1), // R0 is not using, but we need padding here for proper mapping with ICorDebugInfo::RegNum.
    REGISTER_RISCV64_RA,


Also, could you please update the assert in ConvertRegNumToCorDebugRegister from _ASSERTE(reg >= 0); to _ASSERTE(reg > 0);.
`

Sure, will do.

Noti: Build fails on clang16
Error message

/home/clamp/runtime/src/coreclr/debug/inc/riscv64/primitives.h:52:5: error: integer value -1 is outside the valid range of values [0, 255] for this enumeration type [-Wenum-constexpr-conversion] (CorDebugRegister)(-1), // X0 is zero register that is not a real register. We need padding here for proper mapping with ICorDebugInfo::RegNum. ^

Thanks! This probably mean we should use something like (CorDebugRegister)(255) instead here. Initially, the sense of -1 usage was "out of range for sure". Will fix this in next PR.

jkotas

LGTM modulo feedback

[RISC-V] Implement SOS related Debugger API code.

c39db9f

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

dotnet-issue-labeler bot added the area-Diagnostics-coreclr label Nov 7, 2023

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

clamp03 assigned viewizard Nov 7, 2023

viewizard mentioned this pull request Nov 7, 2023

[RISC-V] Implement SOS related code. dotnet/diagnostics#4385

Merged

This was referenced Nov 7, 2023

Test_EventSource_EtwManifestGeneration* tests failing in CI #48798

Closed

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

Closed

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

Closed

[RISC-V] Fix R0 copy.

bdb6876

viewizard force-pushed the riscv64_3 branch from d47ba09 to bdb6876 Compare November 7, 2023 17:52

clamp03 approved these changes Nov 8, 2023

View reviewed changes

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

jkotas reviewed Nov 8, 2023

View reviewed changes

src/coreclr/inc/cordebug.idl Show resolved Hide resolved

gbalykov approved these changes Nov 8, 2023

View reviewed changes

jkotas reviewed Nov 8, 2023

View reviewed changes

jkotas reviewed Nov 9, 2023

View reviewed changes

jkotas approved these changes Nov 9, 2023

View reviewed changes

[RISC-V] Remove R0 from DebuggerREGDISPLAY.

ab84b8c

viewizard force-pushed the riscv64_3 branch from 4f8594f to ab84b8c Compare November 9, 2023 14:48

jkotas merged commit cdd8c7b into dotnet:main Nov 9, 2023
109 of 111 checks passed

viewizard deleted the riscv64_3 branch November 9, 2023 17:23

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISC-V] Implement SOS related Debugger API code. #94454

[RISC-V] Implement SOS related Debugger API code. #94454

viewizard commented Nov 7, 2023

ghost commented Nov 7, 2023

jkotas Nov 8, 2023

viewizard Nov 8, 2023

jkotas Nov 8, 2023

viewizard Nov 8, 2023

jkotas Nov 8, 2023

viewizard Nov 8, 2023

viewizard Nov 8, 2023 •

edited

Loading

jkotas Nov 9, 2023

viewizard Nov 9, 2023

jkotas Nov 9, 2023

jkotas Nov 9, 2023

viewizard Nov 9, 2023

clamp03 Nov 14, 2023

viewizard Nov 14, 2023

jkotas left a comment

	//
	// Mapping from ICorDebugInfo register numbers to CorDebugRegister
	// numbers. Note: this must match the order in corinfo.h.
	//
	inline CorDebugRegister ConvertRegNumToCorDebugRegister(ICorDebugInfo::RegNum reg)
	{
	LIMITED_METHOD_CONTRACT;
	_ASSERTE(reg >= 0);
	_ASSERTE(static_cast<size_t>(reg) < ARRAY_SIZE(g_JITToCorDbgReg));
	return g_JITToCorDbgReg[reg];
	}

	(CorDebugRegister)(-1), // R0 is not using, but we need padding here for proper mapping with ICorDebugInfo::RegNum.
	(CorDebugRegister)(-1), // X0 is zero register that is not a real register. We need padding here for proper mapping with ICorDebugInfo::RegNum.

[RISC-V] Implement SOS related Debugger API code. #94454

[RISC-V] Implement SOS related Debugger API code. #94454

Conversation

viewizard commented Nov 7, 2023

ghost commented Nov 7, 2023

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

viewizard Nov 8, 2023 • edited Loading

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

jkotas left a comment

Choose a reason for hiding this comment

viewizard Nov 8, 2023 •

edited

Loading