[mono][debugger] Implementing mscordbi to support iCorDebug on mono runtime #47639

thaystg · 2021-01-29T17:18:49Z

There are some things hardcoded yet, because we still need to decide how we will launch the process and decide the debugger port.
Using a dbgshim changed to accept mono runtime as a coreclr runtime with this version is possible to:

Watch simple variables
Watch string variables
Watch array variables
Watch object variables
Watch this
Step over / step into / step out
Stop / Continue
Evaluate static methods (with and without parameters)

For this implementation we decided to use metadata reader from coreclr, this is great because I could reuse a lot of code from mscordbi of coreclr.
I'm also using the log infrastructure from coreclr, if we start vsdbg-ui and set these parameters on environment:
COMPlus_LogFile=c:\thays\example.txt
COMPlus_LogToFile=1
COMPlus_LogLevel=8
COMPlus_LogFacility=0x00000200
COMPlus_LogEnable=1

We will see the log message with the icordebug messages which are not implemented. This may help to continue the implementation.

dotnet-issue-labeler · 2021-01-29T17:18:55Z

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 · 2021-01-30T03:09:37Z

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

Issue Details

There are some things hardcoded yet, because we still need to decide how we will launch the process and decide the debugger port.
Using a dbgshim changed to accept mono runtime as a coreclr runtime with this version is possible to:

Watch simple variables
Watch string variables
Watch array variables
Watch object variables
Watch this
Step over / step into / step out
Stop / Continue
Evaluate static methods (with and without parameters)

For this implementation we decided to use metadata reader from coreclr, this is great because I could reuse a lot of code from mscordbi of coreclr.
I'm also using the log infrastructure from coreclr, if we start vsdbg-ui and set these parameters on environment:
COMPlus_LogFile=c:\thays\example.txt
COMPlus_LogToFile=1
COMPlus_LogLevel=8
COMPlus_LogFacility=0x00000200
COMPlus_LogEnable=1

We will see the log message with the icordebug messages which are not implemented. This may help to continue the implementation.

TODO: delete objects, count references and delete when it gets on 0.

Author:	thaystg
Assignees:	-
Labels:	`area-Debugger-mono`
Milestone:	-

src/mono/mono/mini/debugger-protocol.c

@lambdageek

Changing what @lambdageek suggested about mono_atomic_inc_i32

formating with cland-format, renaming properties to follow the used pattern.

…hays_icordbg_pr2

lambdageek · 2021-02-19T21:41:03Z

@dotnet/dotnet-diag Could we get help reviewing this PR:

It's implementing ICorDebug - are we're implementing the right behavior
This is using the CoreCLR metadata subsystem - are we using it correctly?

windows

available on macCatalyst

lambdageek · 2021-03-04T17:53:47Z

src/mono/mono/mini/debugger-agent.c

+ case MDBGPROT_CMD_VM_READ_MEMORY: {
+ guint8* memory = (guint8*) decode_long (p, &p, end);
+ int size = decode_int (p, &p, end);
+ buffer_add_byte_array (buf, memory, size);


Does this need segfault protection like in mono/mono#15612

If final user tries to evaluate an arbitraty address we would need to protect, but I wouldn't like to use this because icordebug uses this(ReadMemory) a lot, and create a file to check memory address would be really slow... I'm planning to study how coreclr checks if it is a valid memory. But I was not planning to do it in this PR, do you think I should add the segfault protection for now and then remove when I have a better solution?

lambdageek

LGTM 🎉

There are a few things that could be changed, but all of them are ok for follow-up PRs.

src/mono/dbi/cordb-process.h

lambdageek · 2021-03-05T21:23:47Z

src/mono/dbi/cordb-process.cpp

+ arrayType->GetClass(&eleClass);
+ eleClass->GetToken(&eleToken);
+ }
+ hash = (long)(pow(2, eleToken & 0xffffff) * pow(3, type) * pow(5, eleType)); //TODO: define a better hash


You could look at mono_metadata_type_hash.

Actually the general suggestion here is to define a separate hash function for CordbType and CordbClass rather than scattering it piece by piece in the Find functions

lambdageek · 2021-03-07T22:09:11Z

eng/native/configureplatform.cmake

+ elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL aarch64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL arm64)
 set(CLR_CMAKE_HOST_UNIX_ARM64 1)
- elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL i686)
+ elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL i686 OR CMAKE_SYSTEM_PROCESSOR STREQUAL x86)


@akoeplinger @ViktorHofer is this ok?

The intention is to build a piece of CoreCLR for Mono's new cordb library - Is it the case that during the mono build we use slightly different system names?

where is this happening? the values we use in src/mono/CMakeLists.txt are upper-cased (ARM64 and X86) so I'm not sure this is really what you want?

I'm not sure if you can see the errors in this commit: a1cb8e6

These were the CI failures without this change:
Build Linux arm Debug AllSubsets_Mono
Build Linux arm64 Debug AllSubsets_Mono_LLVMJIT
Build Linux arm64 Release AllSubsets_Mono_LLVMAOT
Mono Product Build Linux arm64 debug
Mono Product Build Linux arm64 release
Mono llvmaot Product Build Linux arm64 release

lambdageek · 2021-03-07T22:10:08Z

src/mono/CMakeLists.txt

+set(CMAKE_C_FLAGS_CHECKED "")
+set(CMAKE_CXX_FLAGS_CHECKED "")
+set(CMAKE_EXE_LINKER_FLAGS_CHECKED "")
+set(CMAKE_SHARED_LINKER_FLAGS_CHECKED "")
+
+set(CMAKE_SHARED_LINKER_FLAGS_DEBUG "")
+set(CMAKE_SHARED_LINKER_FLAGS_RELEASE "")
+set(CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO "")
+set(CMAKE_EXE_LINKER_FLAGS_DEBUG "")
+set(CMAKE_EXE_LINKER_FLAGS_DEBUG "")
+set(CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO "")


nit: I would like a comment here what this is for (CoreCLR's checked build, I guess?)

do we really need to set these _DEBUG, _RELEASE, _RELWITHDEBINFO variants of the flags? those should already be set by CMake and I'd like to not clear them out here.

Unfortunatelly I don't remember, I will remove and wait for CI.

@akoeplinger, I removed, now do you think it's okay to merge?

lambdageek · 2021-03-08T15:48:23Z

src/mono/dbi/cordb-value.h

+union CordbContent
+{
+ int16_t charValue;
+ int8_t booleanValue;


nit: byteValue might be a better name

lambdageek · 2021-03-08T16:04:31Z

src/mono/dbi/cordb-value.h

+{
+ int16_t charValue;
+ int8_t booleanValue;
+ int32_t intValue;


I don't like that the same union member is used for 32-bit integer values (when it's an System.Int32 or UInt32 value) and for debugger ids (for classes, strings, arrays). Maybe add a debuggerId member that is also an int32_t.

Also in all the places this is used it's paired witha CorElementType, right? Any reason not to store them together in a tagged union:

class CordbTaggedValue { CorElementType m_type; union { int8_t byteValue; ... } m_value; CordbTaggedValue (CorElementType t) {} public: static CordbTaggedValue ByteValue(int8_t x) { CordbTaggedValue v (ELEMENT_TYPE_I1); v.m_value.byteValue = x; return v; } static CordbTaggedValue BoolValue(bool b) { CordbTaggedValue v (ELEMENT_TYPE_BOOLEAN); v.m_value.byteValue = b; return v; } ... public: CordbTaggedValue (const CordbTaggedValue&) = default; CordbTaggedValue& operator=(const CordbTaggedValue&) = default; }

(not completely sure about this, it might be too hard to work with. up to you. and no need to change in this PR; can be followup).

lambdageek · 2021-03-08T16:41:47Z

src/mono/dbi/cordb-chain.cpp

+ chains[0] = new CordbChain(conn, m_pThread, CHAIN_PROCESS_START, false);
+ chains[1] = new CordbChain(conn, m_pThread, CHAIN_ENTER_MANAGED, true);


needs EX_TRY/CATCH

(There are other places, too, in this file and others - I just looked for new and check if the function is one of the COM methods)

lambdageek · 2021-03-08T16:52:00Z

src/mono/dbi/cordb-value.cpp

+ if (m_pCordbType)
+ {
+ m_pCordbType->QueryInterface(IID_ICorDebugType, (void**)ppType);
+ goto __Exit;


I think it's better to break up CordbReferenceValue::GetExactType into some helper functions and use else if instead of goto. Also you can reorganize this to be a little bit simpler: the top three cases are: m_pCordbType != nullptr, m_pClass != nullptr), m_debuggerId != -1, and after those you can switch on m_type

lambdageek · 2021-03-08T16:54:47Z

src/mono/dbi/cordb-value.cpp

+ CorElementType type = (CorElementType)m_dbgprot_decode_byte(pReply->p, &pReply->p, pReply->end);
+ CordbContent value;
+
+ if ((MdbgProtValueTypeId)type == MDBGPROT_VALUE_TYPE_ID_NULL)


the null case can be several helper functions.

lambdageek · 2021-03-08T17:02:13Z

src/mono/mono/mini/debugger-agent.c

+ break;
+ }
+ case MDBGPROT_CMD_VM_SET_USING_ICORDBG: {
+ agent_config.using_icordbg = TRUE;


I don't like this for some reason, but I don't know why. I see it's used to change some of the protocol encoding. Can't we encode this in the protocol version in some way? (Or will that break the old sdb-based debugging?)

(if that's not reasonable, we could extend CMD_VM_SET_PROTOCOL_VERSION to include a "client is icordbg" flag if the major/minor is new enough. and then send "false" from debugger-libs)

Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>

This reverts commit 25f24bc.

This reverts commit a8f0318.

This reverts commit b5de06d.

This reverts commit 2aae463.

This reverts commit ae5f701.

This reverts commit 94d957c.

thaystg added 3 commits January 27, 2021 18:02

Implementing mscordbi on mono to support icordebug

46acad4

Fix eval, fix step and cancel step, remove suspend status on mscordbi

086f051

Using log from coreclr as mscordbi does

7baa9d3

thaystg requested a review from lambdageek January 29, 2021 17:18

monojenkins mentioned this pull request Jan 29, 2021

[mono][debugger] Implementing mscordbi to support iCorDebug on mono runtime mono/mono#20804

Closed

danmoseley added the area-Debugger-mono label Jan 30, 2021

thaystg added 2 commits February 2, 2021 14:24

Missing files

c6cd6e8

Removing eglib from implementation

c5ce818

lambdageek reviewed Feb 4, 2021

View reviewed changes

src/mono/mono/mini/debugger-protocol.c Outdated Show resolved Hide resolved

thaystg added 7 commits February 5, 2021 16:13

Compiling on Mac

b3629ff

Compiling and Working on windows.

d78b7a2

Changing what @lambdageek suggested about mono_atomic_inc_i32

Generating libmscordbi on mac

130866a

Fix socket on mac

759501d

Counting references, deleting objects, removing unecessary properties,

74524ab

formating with cland-format, renaming properties to follow the used pattern.

Merge branch 'thays_icordbg_pr2' of github.com:thaystg/runtime into t…

2dcaeb4

…hays_icordbg_pr2

Checking error when get reply from debugger

12127b9

thaystg marked this pull request as ready for review February 19, 2021 20:10

thaystg requested review from CoffeeFlux, marek-safar, SamMonoRT and vargaz as code owners February 19, 2021 20:10

Merge branch 'master' into thays_icordbg_pr2

45dce7b

thaystg added 5 commits February 22, 2021 12:03

Fix format, reuse regmeta from coreclr and try to fix compilation on

217dc72

windows

Dont build when target is android or ios or wasm

91e7497

Fix error unused variable

d01a9ed

Fixing error: 'CFUserNotificationDisplayAlert' is unavailable: not

be8f56d

available on macCatalyst

Fix compilation error on Linux

11a62cb

lambdageek reviewed Mar 4, 2021

View reviewed changes

runfoapp bot mentioned this pull request Mar 4, 2021

Inability to unzip assets during build on Unix x64 #32805

Closed

thaystg added 2 commits March 5, 2021 16:27

Using 1:1 with runtime for cordbtype and cordbclass

c5b4d4c

Rnaming methods as suggested by @lambdageek

02349c3

lambdageek approved these changes Mar 8, 2021

View reviewed changes

thaystg and others added 9 commits March 8, 2021 14:16

Update src/mono/dbi/cordb-process.h

c02a256

Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>

Update CMakeLists.txt

a16ce8e

Fix arm64 compilation

94d957c

Fix arm64 compilation

ae5f701

Fix error cmake

2aae463

fix arm64 compilation

b5de06d

Fix arm64 compilation

a8f0318

Fix arm64 compilation

25f24bc

Fix arm64 compilation

6032aa3

jkotas mentioned this pull request Mar 10, 2021

Move host components out of installer into a common location #49233

Open

thaystg added 7 commits March 11, 2021 00:04

Revert "Fix arm64 compilation"

8be3bfd

This reverts commit 25f24bc.

Revert "Fix arm64 compilation"

946f27a

This reverts commit a8f0318.

Revert "fix arm64 compilation"

df1ac59

This reverts commit b5de06d.

Revert "Fix error cmake"

1886964

This reverts commit 2aae463.

Revert "Fix arm64 compilation"

5082ae4

This reverts commit ae5f701.

Revert "Fix arm64 compilation"

eb47443

This reverts commit 94d957c.

Removing arm64 compilation

88f5021

This was referenced Mar 11, 2021

OSX deprovision jaredpar/runfo#41

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

Test failure Wasm.Build.Tests.WasmBuildAppTest.InvariantGlobalization #49494

Closed

Remove arm64 compilation

8278121

thaystg merged commit fe8bc17 into dotnet:main Mar 12, 2021

jkotas mentioned this pull request Mar 14, 2021

Add clr.iltools subset #49545

Merged

ghost locked as resolved and limited conversation to collaborators Apr 11, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono][debugger] Implementing mscordbi to support iCorDebug on mono runtime #47639

[mono][debugger] Implementing mscordbi to support iCorDebug on mono runtime #47639

thaystg commented Jan 29, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Jan 29, 2021

ghost commented Jan 30, 2021

lambdageek commented Feb 19, 2021

lambdageek Mar 4, 2021

thaystg Mar 4, 2021

lambdageek left a comment

lambdageek Mar 5, 2021

lambdageek Mar 7, 2021

akoeplinger Mar 8, 2021

thaystg Mar 8, 2021 •

edited

Loading

lambdageek Mar 7, 2021

akoeplinger Mar 8, 2021

thaystg Mar 8, 2021

thaystg Mar 11, 2021

lambdageek Mar 8, 2021

lambdageek Mar 8, 2021

lambdageek Mar 8, 2021

lambdageek Mar 8, 2021

lambdageek Mar 8, 2021

lambdageek Mar 8, 2021

		chains[0] = new CordbChain(conn, m_pThread, CHAIN_PROCESS_START, false);
		chains[1] = new CordbChain(conn, m_pThread, CHAIN_ENTER_MANAGED, true);

[mono][debugger] Implementing mscordbi to support iCorDebug on mono runtime #47639

[mono][debugger] Implementing mscordbi to support iCorDebug on mono runtime #47639

Conversation

thaystg commented Jan 29, 2021 • edited Loading

dotnet-issue-labeler bot commented Jan 29, 2021

ghost commented Jan 30, 2021

lambdageek commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek 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

thaystg Mar 8, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaystg commented Jan 29, 2021 •

edited

Loading

thaystg Mar 8, 2021 •

edited

Loading