Skip to content
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.

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

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

Merged
merged 67 commits into from
Mar 12, 2021
Merged
Show file tree
Hide file tree
Changes from 43 commits
Commits
Show all changes
67 commits
Select commit Hold shift + click to select a range
46acad4
Implementing mscordbi on mono to support icordebug
thaystg Jan 27, 2021
086f051
Fix eval, fix step and cancel step, remove suspend status on mscordbi
thaystg Jan 28, 2021
7baa9d3
Using log from coreclr as mscordbi does
thaystg Jan 29, 2021
c6cd6e8
Missing files
thaystg Feb 2, 2021
c5ce818
Removing eglib from implementation
thaystg Feb 3, 2021
b3629ff
Compiling on Mac
thaystg Feb 5, 2021
d78b7a2
Compiling and Working on windows.
thaystg Feb 5, 2021
130866a
Generating libmscordbi on mac
thaystg Feb 5, 2021
759501d
Fix socket on mac
thaystg Feb 12, 2021
74524ab
Counting references, deleting objects, removing unecessary properties,
thaystg Feb 19, 2021
2dcaeb4
Merge branch 'thays_icordbg_pr2' of github.com:thaystg/runtime into t…
thaystg Feb 19, 2021
12127b9
Checking error when get reply from debugger
thaystg Feb 19, 2021
45dce7b
Merge branch 'master' into thays_icordbg_pr2
thaystg Feb 19, 2021
217dc72
Fix format, reuse regmeta from coreclr and try to fix compilation on
thaystg Feb 22, 2021
91e7497
Dont build when target is android or ios or wasm
thaystg Feb 22, 2021
d01a9ed
Fix error unused variable
thaystg Feb 22, 2021
be8f56d
Fixing error: 'CFUserNotificationDisplayAlert' is unavailable: not
thaystg Feb 22, 2021
11a62cb
Fix compilation error on Linux
thaystg Feb 22, 2021
ac1735f
Fix function not found in maccatalyst
thaystg Feb 22, 2021
d3cc3b4
Dont compile mscordbi on maccatalyst
thaystg Feb 22, 2021
2ff0e68
Fix compilation on linux
thaystg Feb 22, 2021
a1cb8e6
Fix compilation on android, ios and browser
thaystg Feb 22, 2021
9a98304
Fix compilation on linux
thaystg Feb 22, 2021
e3b74f4
Fix cross compiling
thaystg Feb 22, 2021
b7de3c8
Fix compilation x86 windows
thaystg Feb 23, 2021
6c3c0a2
Merge branch 'thays_icordbg_pr2' of github.com:thaystg/runtime into t…
thaystg Feb 23, 2021
8e54659
Fix x86 compilation
thaystg Feb 23, 2021
cc38142
Fix compilation on windows
thaystg Feb 23, 2021
9fdaaa2
Fix cmake error
thaystg Feb 23, 2021
6e53c1a
Fix cmake
thaystg Feb 23, 2021
a9d86ef
Fix arm64 mac
thaystg Feb 23, 2021
785d53c
Fix windows compilation
thaystg Feb 23, 2021
f1de772
Fix memory leak
thaystg Feb 23, 2021
5939ca5
Returning E_NOTIMPL where it was possible and it's not implemented as
thaystg Feb 23, 2021
8a6f3e9
Fix what @cshung
thaystg Feb 23, 2021
768b5e9
Use only one ref count
thaystg Feb 23, 2021
cdcbe2d
Adding ex_try everywhere that we could have an allocation failure.
thaystg Feb 23, 2021
7cc409c
Implementing get class
thaystg Feb 23, 2021
abc2aa3
Apply suggestions from code review
thaystg Feb 24, 2021
208d222
Fix what @noahfalk suggested in his review.
thaystg Feb 24, 2021
5363bce
Fixing what was suggested by @viniciusjarina
thaystg Feb 24, 2021
24bccdb
Fix what was suggested by @viniciusjarina
thaystg Feb 24, 2021
bafe8ee
Changing what @noahfalk suggested
thaystg Feb 25, 2021
5682529
Accept more than 1 stepper per thread
thaystg Mar 1, 2021
8627c1a
Changing what was suggested by @noahfalk
thaystg Mar 2, 2021
0248812
Using CORDB_ADDRESS to return addresses from debugged process
thaystg Mar 4, 2021
10389fd
Fix eval
thaystg Mar 4, 2021
3ebfdd8
Update src/mono/mono/mini/debugger-agent.c
thaystg Mar 4, 2021
c5b4d4c
Using 1:1 with runtime for cordbtype and cordbclass
thaystg Mar 5, 2021
02349c3
Rnaming methods as suggested by @lambdageek
thaystg Mar 5, 2021
c02a256
Update src/mono/dbi/cordb-process.h
thaystg Mar 8, 2021
a16ce8e
Update CMakeLists.txt
thaystg Mar 8, 2021
94d957c
Fix arm64 compilation
thaystg Mar 9, 2021
ae5f701
Fix arm64 compilation
thaystg Mar 9, 2021
2aae463
Fix error cmake
thaystg Mar 9, 2021
b5de06d
fix arm64 compilation
thaystg Mar 9, 2021
a8f0318
Fix arm64 compilation
thaystg Mar 9, 2021
25f24bc
Fix arm64 compilation
thaystg Mar 10, 2021
6032aa3
Fix arm64 compilation
thaystg Mar 10, 2021
8be3bfd
Revert "Fix arm64 compilation"
thaystg Mar 11, 2021
946f27a
Revert "Fix arm64 compilation"
thaystg Mar 11, 2021
df1ac59
Revert "fix arm64 compilation"
thaystg Mar 11, 2021
1886964
Revert "Fix error cmake"
thaystg Mar 11, 2021
5082ae4
Revert "Fix arm64 compilation"
thaystg Mar 11, 2021
eb47443
Revert "Fix arm64 compilation"
thaystg Mar 11, 2021
88f5021
Removing arm64 compilation
thaystg Mar 11, 2021
8278121
Remove arm64 compilation
thaystg Mar 12, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions eng/native/configureplatform.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ if(CLR_CMAKE_HOST_OS STREQUAL Linux)
set(CLR_CMAKE_HOST_UNIX_ARMV7L 1)
elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL arm OR CMAKE_SYSTEM_PROCESSOR STREQUAL armv7-a)
set(CLR_CMAKE_HOST_UNIX_ARM 1)
elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL aarch64)
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)
Comment on lines +44 to +46
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@thaystg thaystg Mar 8, 2021

Choose a reason for hiding this comment

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

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

set(CLR_CMAKE_HOST_UNIX_X86 1)
else()
clr_unknown_arch()
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/clrdefinitions.cmake
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
include(clrfeatures.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/clrfeatures.cmake)

add_compile_definitions($<$<BOOL:$<TARGET_PROPERTY:DAC_COMPONENT>>:DACCESS_COMPILE>)
add_compile_definitions($<$<BOOL:$<TARGET_PROPERTY:CROSSGEN_COMPONENT>>:CROSSGEN_COMPILE>)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/pal/src/configure.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -1402,4 +1402,4 @@ check_prototype_definition(
${STATFS_INCLUDES}
HAVE_NON_LEGACY_STATFS)

configure_file(${CMAKE_CURRENT_SOURCE_DIR}/config.h.in ${CMAKE_CURRENT_BINARY_DIR}/config.h)
configure_file(${CMAKE_CURRENT_LIST_DIR}/config.h.in ${CMAKE_CURRENT_BINARY_DIR}/config.h)
2 changes: 1 addition & 1 deletion src/coreclr/pal/src/libunwind/configure.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ int main(void)
return result;
}" HAVE_STDALIGN_ALIGNAS)

configure_file(${CMAKE_CURRENT_SOURCE_DIR}/config.h.in ${CMAKE_CURRENT_BINARY_DIR}/include/config.h)
configure_file(${CMAKE_CURRENT_LIST_DIR}/config.h.in ${CMAKE_CURRENT_BINARY_DIR}/include/config.h)
add_definitions(-DHAVE_CONFIG_H=1)

configure_file(include/libunwind-common.h.in ${CMAKE_CURRENT_BINARY_DIR}/include/libunwind-common.h)
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/pal/src/synchmgr/synchmanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4623,7 +4623,11 @@ namespace CorUnix
ptsAbsTmo->tv_nsec = tv.tv_usec * tccMicroSecondsToNanoSeconds;
}
#else
#error "Don't know how to get hi-res current time on this platform"
#ifdef DBI_COMPONENT_MONO
return ERROR_INTERNAL_ERROR;
#else
#error "Don't know how to get hi-res current time on this platform"
#endif
#endif // HAVE_WORKING_CLOCK_GETTIME, HAVE_WORKING_GETTIMEOFDAY
#if HAVE_CLOCK_MONOTONIC && HAVE_PTHREAD_CONDATTR_SETCLOCK
}
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/utilcode/ccomprc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,9 @@ HRESULT CCompRC::LoadString(ResourceCategory eCategory, UINT iResourceID, __out_

HRESULT CCompRC::LoadString(ResourceCategory eCategory, LocaleID langId, UINT iResourceID, __out_ecount(iMax) LPWSTR szBuffer, int iMax, int *pcwchUsed)
{
#ifdef DBI_COMPONENT_MONO
return E_NOTIMPL;
#else
CONTRACTL
{
GC_NOTRIGGER;
Expand Down Expand Up @@ -535,6 +538,7 @@ HRESULT CCompRC::LoadString(ResourceCategory eCategory, LocaleID langId, UINT iR
return LoadNativeStringResource(NATIVE_STRING_RESOURCE_TABLE(NATIVE_STRING_RESOURCE_NAME), iResourceID,
szBuffer, iMax, pcwchUsed);
#endif // HOST_WINDOWS
#endif
}

#ifndef DACCESS_COMPILE
Expand Down
15 changes: 15 additions & 0 deletions src/mono/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,18 @@ endif()

project(mono)

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 "")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


include(GNUInstallDirs)
include(CheckIncludeFile)
include(CheckFunctionExists)
Expand Down Expand Up @@ -699,6 +711,9 @@ endif()
### End of OS specific checks

add_subdirectory(mono)
if (NOT CMAKE_CROSSCOMPILING AND NOT TARGET_IOS AND NOT TARGET_ANDROID AND NOT TARGET_BROWSER AND NOT HOST_MACCATALYST)
add_subdirectory(dbi)
endif()

configure_file(cmake/config.h.in config.h)
configure_file(cmake/eglib-config.h.cmake.in mono/eglib/eglib-config.h) # TODO: eglib-config.h is not needed, we're using hardcoded eglib-config.hw
151 changes: 151 additions & 0 deletions src/mono/dbi/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
project(mscordbi)

set(CMAKE_INCLUDE_CURRENT_DIR ON)

set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

set(CLR_DIR ${PROJECT_SOURCE_DIR}/../../coreclr)
set(VM_DIR ${PROJECT_SOURCE_DIR}/../../coreclr/vm)
set(CMAKE_OSX_ARCHITECTURES ${CMAKE_SYSTEM_PROCESSOR})
set(CMAKE_EXE_LINKER_FLAGS_CHECKED "")
set(CMAKE_SHARED_LINKER_FLAGS_CHECKED "")
set(CLR_CMAKE_HOST_ARCH ${CMAKE_GENERATOR_PLATFORM})
set(FEATURE_EVENT_TRACE 0)

if(HOST_WIN32)
if(HOST_X86)
set(CLR_CMAKE_HOST_ARCH x86)
elseif(HOST_ARM64)
set(CLR_CMAKE_HOST_ARCH arm64)
elseif(HOST_ARM)
set(CLR_CMAKE_HOST_ARCH arm)
elseif(HOST_AMD64)
set(CLR_CMAKE_HOST_ARCH x64)
endif()
endif()

add_definitions(-DDBI_COMPONENT_MONO)

include_directories(
${CMAKE_CURRENT_SOURCE_DIR}/../..
${PROJECT_SOURCE_DIR}/../
${PROJECT_SOURCE_DIR}/../dbi
${PROJECT_SOURCE_DIR}/../dbi/socket-dbi
${PROJECT_SOURCE_DIR}/../../coreclr/md/enc
${PROJECT_SOURCE_DIR}/../../coreclr/inc
${PROJECT_SOURCE_DIR}/../../coreclr/md/inc
${PROJECT_SOURCE_DIR}/../../coreclr/md/compiler)

set(mscorbi_sources_base
cordb.cpp
cordb.h
cordb-appdomain.cpp
cordb-appdomain.h
cordb-assembly.cpp
cordb-assembly.h
cordb-blocking-obj.cpp
cordb-blocking-obj.h
cordb-breakpoint.cpp
cordb-breakpoint.h
cordb-chain.cpp
cordb-chain.h
cordb-class.cpp
cordb-class.h
cordb-code.cpp
cordb-code.h
cordb-eval.cpp
cordb-eval.h
cordb-frame.cpp
cordb-frame.h
cordb-function.cpp
cordb-function.h
cordb-process.cpp
cordb-process.h
cordb-register.cpp
cordb-register.h
cordb-stepper.cpp
cordb-stepper.h
cordb-thread.cpp
cordb-thread.h
cordb-type.cpp
cordb-type.h
cordb-value.cpp
cordb-value.h
)


if(HOST_DARWIN)
set(OS_LIBS "-framework CoreFoundation" "-framework Foundation")
elseif(HOST_LINUX)
set(OS_LIBS pthread m dl)
elseif(HOST_WIN32)
set(OS_LIBS bcrypt.lib Mswsock.lib ws2_32.lib psapi.lib version.lib advapi32.lib winmm.lib kernel32.lib)
endif()

addprefix(mscorbi_sources ../dbi/ "${mscorbi_sources_base}")
add_subdirectory(${PROJECT_SOURCE_DIR}/socket-dbi)

include(${PROJECT_SOURCE_DIR}/../../../eng/native/configuretools.cmake)
include(${PROJECT_SOURCE_DIR}/../../../eng/native/configurepaths.cmake)
include(${PROJECT_SOURCE_DIR}/../../../eng/native/configureplatform.cmake)
include(${PROJECT_SOURCE_DIR}/../../../eng/native/configurecompiler.cmake)

if (CLR_CMAKE_HOST_UNIX)
include_directories("${PROJECT_SOURCE_DIR}/../../coreclr/pal/inc")
include_directories("${PROJECT_SOURCE_DIR}/../../coreclr/pal/inc/rt")
include_directories("${PROJECT_SOURCE_DIR}/../../coreclr/pal/src/safecrt")

append("-Wno-missing-prototypes -Wno-pointer-arith -Wno-macro-redefined" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
add_subdirectory(${PROJECT_SOURCE_DIR}/../../coreclr/pal pal)

include_directories("../../coreclr/pal/inc/rt/cpp")
add_compile_options(-nostdinc)
endif (CLR_CMAKE_HOST_UNIX)

include_directories("../../coreclr/pal/prebuilt/inc")
include_directories("../../coreclr/nativeresources")

if (CLR_CMAKE_HOST_UNIX)
add_subdirectory(${PROJECT_SOURCE_DIR}/../../coreclr/nativeresources nativeresources)
endif()

add_subdirectory(${PROJECT_SOURCE_DIR}/../../coreclr/md/runtime md/runtime)
add_subdirectory(${PROJECT_SOURCE_DIR}/../../coreclr/md/compiler md/compiler)
Copy link
Member

Choose a reason for hiding this comment

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

Has there been any discussion in the past or a plan in place about these cross-runtime build links? I'm not sure if this is intended as a temporary step or something more permanent. The main example that I am aware of for native code shared between mono and coreclr is @lateralusX EventPipe work. I could imagine other components following the same trajectory, hopefully with vastly less labor involved assuming we are just moving them to another directory and not attempting to rewrite them from C++ to C. From my POV the main concern is setting expectations with developers about what pieces of code are shared cross-runtime so that everyone knows what they need to keep in mind when making changes or running tests. Moving directories isn't the only way to accomplish that, but it appears to be the pattern we've adopted so far.

Copy link
Member Author

Choose a reason for hiding this comment

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

In my opinion this would be permanent.
@lambdageek, do you think we should move directories that I'm using to "native" directory?

Copy link
Member

Choose a reason for hiding this comment

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

This is a permanent dependency.

I think it would be good to componentize this part of CoreCLR. I'm not sure whether moving this to src/native is worth it. It's still more closely tied to CoreCLR than to Mono. (Mono itself doesn't use it. Just mono's cordb).

Maybe moving some files around under src/coreclr/md to more clearly define what is a separable component here makes sense.

Certainly we should teach our CI to run debugger tests on mono (or at least build Mono's cordb when those files change).

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion as most of the developers I work with day to day rarely make changes in these areas of the code. Adding @davidwrighton or @jkotas who might be better positioned to represent devs that touch that code more frequently in case they have any concerns.

Copy link
Member

Choose a reason for hiding this comment

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

I expect that we will eventually end up in a place where majority of the unmanaged code is shared between CoreCLR and Mono, similar how vast majority of managed code is shared between CoreCLR and Mono today.

I think it is good idea to be moving the shared pieces under src/native to make it obvious what is shared.

Copy link
Member Author

@thaystg thaystg Feb 25, 2021

Choose a reason for hiding this comment

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

@lambdageek, if we decide to move, should move files in this PR or in another one?

Copy link
Member

Choose a reason for hiding this comment

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

My personal preference is to separate re-organization PRs from PRs that add functionality. Also this PR is already pretty big. I would do it in a follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it is a good idea to do the move in separate PR.


include(${PROJECT_SOURCE_DIR}/../../coreclr/clrdefinitions.cmake)
include_directories(${CMAKE_CURRENT_BINARY_DIR}/../)
include_directories(${CMAKE_CURRENT_BINARY_DIR}/../inc/)
add_subdirectory(${PROJECT_SOURCE_DIR}/../../coreclr/md/enc md/enc)
add_subdirectory(${PROJECT_SOURCE_DIR}/../../coreclr/utilcode utilcode)
if (CLR_CMAKE_HOST_UNIX)
add_subdirectory(${PROJECT_SOURCE_DIR}/../../coreclr/palrt palrt)
append("-Wno-strict-prototypes -Wno-deprecated -Wno-pointer-arith" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
endif (CLR_CMAKE_HOST_UNIX)

add_library(mscordbi SHARED "${mscorbi_sources};${PROJECT_SOURCE_DIR}/../mono/mini/debugger-protocol.c;${PROJECT_SOURCE_DIR}/../../coreclr/pal/prebuilt/idl/xcordebug_i.cpp;${PROJECT_SOURCE_DIR}/../../coreclr/pal/prebuilt/idl/cordebug_i.cpp")

#SET(CMAKE_C_COMPILER ${CMAKE_CXX_COMPILER})

set_source_files_properties(${PROJECT_SOURCE_DIR}/../mono/mini/debugger-protocol.c PROPERTIES LANGUAGE CXX)

set(COREDBI_LIBRARIES
utilcodestaticnohost
mdruntime-dbi
mdcompiler-dbi
mdruntimerw-dbi
socket-dbi
${OS_LIBS}
)

if(CLR_CMAKE_HOST_UNIX)
list(APPEND COREDBI_LIBRARIES
coreclrpal
palrt
nativeresourcestring
)
endif()

target_link_libraries(mscordbi ${COREDBI_LIBRARIES} )
install(TARGETS mscordbi DESTINATION lib)
Loading