[monodroid] Migrate to C++ (#2089)

The purpose of this conversion is to start using a language that is
stricter and yet more powerful than C.  Stricter in type safety, in
separation of private and public functions, in data encapsulation.
Taking advantage of the C++ *language* features (as opposed to its
standard library)  allows us to write better structured,
safer and less fragile code.  C++ also allows us to implement safer
memory management (not part of this commit, see below).

This commit implements the basic, skeletal work on straightforward
C to C++ conversion:

  * Introduces classes to encompass a number of APIs that can be
    grouped together

  * fixes a number of issues with the old C code (void pointer
    arithmetic, incorrect type casts, comparisons of integers with
    different signs or of different sizes)

  * introduces strict type checking in the Mono interface API (it
    used to use `void*` pointers which could lead to confusion and
    hard to detect runtime/build time errors, see below)

The Mono API (found in the `DylibMono` class) was error prone, as
mentioned above, because it allowed code similar to the following
fragment to build without any warning or error from the compilers:

	void my_code (MonoDomain *domain) { }
	// ...
	MonoDomain *domain = get_domain ();
	my_code (&domain);

`MonoDomain` (and other Mono types) were `typedef`ed to be `void`
which made the above call "valid" (as a `void**` ISA `void*`).
A developer working with code wouldn't immediately know what was the
issue because of the `typedef` hiding the actual `MonoDomain` type.
During test execution, code in `monodroid-glue.cc` would mistakenly
pass a pointer to `MonoDomain` pointer (a.k.a `void**`) to the Mono
runtime caused the runtime to block while attempting to obtain a lock
on the domain where the domain did not exist and, obviously, not
initialized, so and the call couldn't succeed.

Thus the idea to change the typedefs to `struct XY` which introduces
a unique type, making all mistakes of this kind (as well as invalid
casts) obviously wrong and caught by the compiler.

This commit does *not* implement much in the are of memory management
because such changes would make porting code from master to the
branch increasingly complex and error prone.  For the same reason
`monodroid-glue.cc` is not split up into separate files (which is the
eventual plan) but merely introduces a handful of classes in that file.
After the commit is merged into master both of those changes will be
implemented to further take advantage of C++ capabilities.

Author: grendello

src/monodroid/CMakeLists.txt

@@ -147,12 +147,14 @@ if(ENABLE_NDK)
   add_definitions("-DANDROID")
   add_definitions("-DLINUX -Dlinux -D__linux__")
 else()
+  # MinGW/mxe need it for {v,a}sprintf
+  add_definitions("-D_GNU_SOURCE")
   if(APPLE)
     add_definitions("-DAPPLE_OS_X")
     set(HOST_BUILD_NAME "host-Darwin")
   elseif(CMAKE_HOST_SYSTEM_NAME STREQUAL Linux)
     if(NOT MINGW AND NOT WIN32)
-      add_definitions("-D_GNU_SOURCE -DLINUX -Dlinux -D__linux__")
+      add_definitions("-DLINUX -Dlinux -D__linux__")
       set(HOST_BUILD_NAME "host-Linux")
 
       if(EXISTS "/.flatpak-info")
@@ -205,9 +207,9 @@ endif()
 
 add_definitions("-DHAVE_CONFIG_H")
 add_definitions("-D_REENTRANT")
+add_definitions("-DDYLIB_MONO")
 add_definitions("-DJI_DLL_EXPORT")
 add_definitions("-DMONO_DLL_EXPORT")
-add_definitions("-DDYLIB_MONO")
 add_definitions("-DSGEN_BRIDGE_VERSION=${SGEN_BRIDGE_VERSION}")
 
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${EXTRA_COMPILER_FLAGS} ${EXTRA_C_FLAGS} -std=c99")
@@ -232,13 +234,15 @@ set(SOURCES_DIR ${TOP_DIR}/jni)
 set(MONODROID_SOURCES
   ${MONODROID_SOURCES}
   ${MONO_PATH}/support/zlib-helper.c
-  ${SOURCES_DIR}/cpu-arch-detect.c
-  ${SOURCES_DIR}/dylib-mono.c
-  ${SOURCES_DIR}/embedded-assemblies.c
+  ${SOURCES_DIR}/cpu-arch-detect.cc
+  ${SOURCES_DIR}/debug-constants.cc
+  ${SOURCES_DIR}/dylib-mono.cc
+  ${SOURCES_DIR}/embedded-assemblies.cc
+  ${SOURCES_DIR}/globals.cc
   ${SOURCES_DIR}/jni.c
-  ${SOURCES_DIR}/monodroid-glue.c
-  ${SOURCES_DIR}/timezones.c
-  ${SOURCES_DIR}/util.c
+  ${SOURCES_DIR}/monodroid-glue.cc
+  ${SOURCES_DIR}/timezones.cc
+  ${SOURCES_DIR}/util.cc
   ${SOURCES_DIR}/zip/ioapi.c
   ${SOURCES_DIR}/zip/unzip.c
   ${JAVA_INTEROP_SRC_PATH}/java-interop.c
@@ -251,9 +255,9 @@ if(UNIX)
   set(MONODROID_SOURCES
     ${MONODROID_SOURCES}
     ${MONO_PATH}/support/nl.c
-    ${SOURCES_DIR}/debug.c
-    ${SOURCES_DIR}/monodroid-networkinfo.c
-    ${SOURCES_DIR}/xamarin_getifaddrs.c
+    ${SOURCES_DIR}/debug.cc
+    ${SOURCES_DIR}/monodroid-networkinfo.cc
+    ${SOURCES_DIR}/xamarin_getifaddrs.cc
     )
 endif()
 

src/monodroid/jni/TODO.md

@@ -0,0 +1,11 @@
+= Memory management
+
+  * Don't use malloc and friends on pointers which aren't passed to Mono
+
+= Strings
+
+  * Implement a simple class to perform all string manipulations
+
+= Warnings
+
+  * Fix all the C++ warnings regarding constant strings

src/monodroid/jni/cppcompat.h

@@ -0,0 +1,24 @@
+// This is a -*- C++ -*- header
+#ifndef __CPP_COMPAT_H
+#define __CPP_COMPAT_H
+
+// Since Android doesn't currently have any standard C++ library
+// and we don't want to use any implementation of it shipped in
+// source form with the NDK (for space reasons), this header will
+// contain implementations of certain C++ standard functions, classes
+// etc we want to use despite lack of the STL.
+//
+// When/if we have any STL implementation available on standard Android
+// we can remove this file.
+namespace std
+{
+	template <typename T> struct remove_reference      { using type = T; };
+	template <typename T> struct remove_reference<T&>  { using type = T; };
+	template <typename T> struct remove_reference<T&&> { using type = T; };
+
+	template<typename T> typename remove_reference<T>::type&& move (T&& arg) noexcept
+	{
+		return static_cast<typename remove_reference<decltype(arg)>::type&&>(arg);
+	}
+}
+#endif

src/monodroid/jni/cpu-arch-detect.c renamed to src/monodroid/jni/cpu-arch-detect.cc

@@ -0,0 +1,26 @@
+#include "debug.h"
+
+using namespace xamarin::android;
+
+// These are moved here so that the Windows build works and we don't need to
+// #ifdef references to these out in this case. Debugging code generally works
+// only on Unix atm
+const char Debug::DEBUG_MONO_CONNECT_PROPERTY[]      = "debug.mono.connect";
+const char Debug::DEBUG_MONO_DEBUG_PROPERTY[]        = "debug.mono.debug";
+const char Debug::DEBUG_MONO_ENV_PROPERTY[]          = "debug.mono.env";
+const char Debug::DEBUG_MONO_EXTRA_PROPERTY[]        = "debug.mono.extra";
+const char Debug::DEBUG_MONO_GC_PROPERTY[]           = "debug.mono.gc";
+const char Debug::DEBUG_MONO_GDB_PROPERTY[]          = "debug.mono.gdb";
+const char Debug::DEBUG_MONO_GDBPORT_PROPERTY[]      = "debug.mono.gdbport";
+const char Debug::DEBUG_MONO_LOG_PROPERTY[]          = "debug.mono.log";
+const char Debug::DEBUG_MONO_MAX_GREFC[]             = "debug.mono.max_grefc";
+const char Debug::DEBUG_MONO_PROFILE_PROPERTY[]      = "debug.mono.profile";
+const char Debug::DEBUG_MONO_RUNTIME_ARGS_PROPERTY[] = "debug.mono.runtime_args";
+const char Debug::DEBUG_MONO_SOFT_BREAKPOINTS[]      = "debug.mono.soft_breakpoints";
+const char Debug::DEBUG_MONO_TRACE_PROPERTY[]        = "debug.mono.trace";
+const char Debug::DEBUG_MONO_WREF_PROPERTY[]         = "debug.mono.wref";
+
+extern "C" const char *__get_debug_mono_log_property (void)
+{
+	return static_cast<const char*> (Debug::DEBUG_MONO_LOG_PROPERTY);
+}

src/monodroid/jni/debug-constants.cc

@@ -21,17 +21,19 @@
 #include <errno.h>
 #include <ctype.h>
 #include <assert.h>
-#include <pthread.h>
 
 #ifdef ANDROID
 #include <android/log.h>
 #endif
 
+extern "C" {
 #include "java-interop-util.h"
+}
 
 #include "monodroid.h"
 #include "debug.h"
 #include "util.h"
+#include "globals.h"
 
 //
 // The communication between xs and the app works as follows:
@@ -46,28 +48,27 @@
 //
 //
 
-#ifdef DEBUG
-
 // monodroid-glue.c
-int process_cmd (int fd, char *cmd);
-
-static int conn_port;
-static pthread_t conn_thread_id;
+extern int process_cmd (int fd, char *cmd);
+namespace xamarin { namespace android
+{
+	void* conn_thread (void *arg);
+}}
 
-static void* conn_thread (void *arg);
+#ifdef DEBUG
+using namespace xamarin::android;
 
-typedef struct {
-	int64_t timeout_time;
-} ConnOptions;
+int Debug::conn_port = 0;
+pthread_t Debug::conn_thread_id = 0;
 
-static void
-parse_options (char *options, ConnOptions *opts)
+inline void
+Debug::parse_options (char *options, ConnOptions *opts)
 {
 	char **args, **ptr;
 
 	log_info (LOG_DEFAULT, "Connection options: '%s'", options);
 
-	args = monodroid_strsplit (options, ",", -1);
+	args = utils.monodroid_strsplit (options, ",", -1);
 
 	for (ptr = args; ptr && *ptr; ptr++) {
 		const char *arg = *ptr;
@@ -99,7 +100,7 @@ parse_options (char *options, ConnOptions *opts)
  * - 2 if no connection is neccessary
  */
 int
-start_connection (char *options)
+Debug::start_connection (char *options)
 {
 	int res;
 	ConnOptions opts;
@@ -119,7 +120,7 @@ start_connection (char *options)
 	if (!conn_port)
 		return 0;
 
-	res = pthread_create (&conn_thread_id, NULL, conn_thread, NULL);
+	res = pthread_create (&conn_thread_id, NULL, xamarin::android::conn_thread, this);
 	if (res) {
 		log_error (LOG_DEFAULT, "Failed to create connection thread: %s", strerror (errno));
 		return 1;
@@ -134,8 +135,8 @@ start_connection (char *options)
  * Handle communication on the socket FD. Return TRUE if its neccessary to create more connections to handle more data.
  * Call process_cmd () with each command received.
  */
-static int
-process_connection (int fd)
+inline int
+Debug::process_connection (int fd)
 {
 	// make sure the fd/socket blocks on reads/writes
 	fcntl (fd, F_SETFL, fcntl (fd, F_GETFL, NULL) & ~O_NONBLOCK);
@@ -145,7 +146,7 @@ process_connection (int fd)
 		int rv;
 		unsigned char cmd_len;
 
-		rv = recv_uninterrupted (fd, &cmd_len, 1);
+		rv = utils.recv_uninterrupted (fd, &cmd_len, 1);
 		if (rv == 0) {
 			log_info (LOG_DEFAULT, "EOF on socket.\n");
 			return FALSE;
@@ -155,7 +156,7 @@ process_connection (int fd)
 			return FALSE;
 		}
 
-		rv = recv_uninterrupted (fd, command, cmd_len);
+		rv = utils.recv_uninterrupted (fd, command, cmd_len);
 		if (rv <= 0) {
 			log_info (LOG_DEFAULT, "Error while receiving command from XS (%s)\n", strerror (errno));
 			return FALSE;
@@ -172,8 +173,8 @@ process_connection (int fd)
 	}
 }
 
-static int
-handle_server_connection (void)
+inline int
+Debug::handle_server_connection (void)
 {
 	int listen_port = conn_port;
 	struct sockaddr_in listen_addr;
@@ -295,18 +296,23 @@ handle_server_connection (void)
 	return rv;
 }
 
+// TODO: this is less than ideal. We can't use std::function or std::bind beause we
+// don't have the C++ stdlib on Android (well, we do but including it would make the
+// app huge so we don't want to involve it). To better solve it we need our own equivalent
+// to std::function
 void*
-conn_thread (void *arg)
+xamarin::android::conn_thread (void *arg)
 {
-	int res;
+	assert (arg != nullptr);
 
-	res = handle_server_connection ();
+	int res;
+	Debug *instance = static_cast<Debug*> (arg);
+	res = instance->handle_server_connection ();
 	if (res && res != 3) {
 		log_fatal (LOG_DEBUGGER, "Error communicating with the IDE, exiting...");
 		exit (FATAL_EXIT_DEBUGGER_CONNECT);
 	}
 
 	return NULL;
 }
-
 #endif /* DEBUG */

src/monodroid/jni/debug.c renamed to src/monodroid/jni/debug.cc

@@ -1,24 +1,65 @@
+// This is a -*- C++ -*- header
 #ifndef __MONODROID_DEBUG_H__
 #define __MONODROID_DEBUG_H__
 
-/* Android property containing connection information, set by XS */
-#define DEBUG_MONO_CONNECT_PROPERTY      "debug.mono.connect"
-#define DEBUG_MONO_DEBUG_PROPERTY        "debug.mono.debug"
-#define DEBUG_MONO_ENV_PROPERTY          "debug.mono.env"
-#define DEBUG_MONO_EXTRA_PROPERTY        "debug.mono.extra"
-#define DEBUG_MONO_GC_PROPERTY           "debug.mono.gc"
-#define DEBUG_MONO_GDB_PROPERTY          "debug.mono.gdb"
-#define DEBUG_MONO_GDBPORT_PROPERTY      "debug.mono.gdbport"
-#define DEBUG_MONO_LOG_PROPERTY          "debug.mono.log"
-#define DEBUG_MONO_MAX_GREFC             "debug.mono.max_grefc"
-#define DEBUG_MONO_PROFILE_PROPERTY      "debug.mono.profile"
-#define DEBUG_MONO_RUNTIME_ARGS_PROPERTY "debug.mono.runtime_args"
-#define DEBUG_MONO_SOFT_BREAKPOINTS      "debug.mono.soft_breakpoints"
-#define DEBUG_MONO_TRACE_PROPERTY        "debug.mono.trace"
-#define DEBUG_MONO_WREF_PROPERTY         "debug.mono.wref"
-
-#ifndef WINDOWS
-int start_connection (char *options);
+#include <stdint.h>
+#include <pthread.h>
+
+#ifdef __cplusplus
+namespace xamarin { namespace android
+{
+	class Debug
+	{
+	private:
+		struct ConnOptions
+		{
+			int64_t timeout_time;
+		};
+
+	public:
+		/* Android property containing connection information, set by XS */
+		static const char DEBUG_MONO_CONNECT_PROPERTY[];
+		static const char DEBUG_MONO_DEBUG_PROPERTY[];
+		static const char DEBUG_MONO_ENV_PROPERTY[];
+		static const char DEBUG_MONO_EXTRA_PROPERTY[];
+		static const char DEBUG_MONO_GC_PROPERTY[];
+		static const char DEBUG_MONO_GDB_PROPERTY[];
+		static const char DEBUG_MONO_GDBPORT_PROPERTY[];
+		static const char DEBUG_MONO_LOG_PROPERTY[];
+		static const char DEBUG_MONO_MAX_GREFC[];
+		static const char DEBUG_MONO_PROFILE_PROPERTY[];
+		static const char DEBUG_MONO_RUNTIME_ARGS_PROPERTY[];
+		static const char DEBUG_MONO_SOFT_BREAKPOINTS[];
+		static const char DEBUG_MONO_TRACE_PROPERTY[];
+		static const char DEBUG_MONO_WREF_PROPERTY[];
+
+	public:
+		explicit Debug ()
+		{}
+
+#if !defined (WINDOWS) && defined (DEBUG)
+		int start_connection (char *options);
+
+	private:
+		void parse_options (char *options, ConnOptions *opts);
+		int process_connection (int fd);
+		int handle_server_connection (void);
+		friend void* conn_thread (void *arg);
+
+	private:
+		static int conn_port;
+		static pthread_t conn_thread_id;
+
 #endif
+	};
+} }
+#else // __cplusplus
+const char *__get_debug_mono_log_property (void);
+
+#ifndef DEBUG_MONO_LOG_PROPERTY
+#define DEBUG_MONO_LOG_PROPERTY __get_debug_mono_log_property ()
+#endif
+
+#endif // __cplusplus
 
 #endif /* __MONODROID_DEBUG_H__ */