Start switching System.Native from C++ to C by luhenry · Pull Request #25032 · dotnet/corefx

luhenry · 2017-11-03T01:58:57Z

Motivation: Sharing code between .NET Core and Mono.

SunnyWar · 2017-11-03T03:17:10Z

@luhenry perhaps a few words in the description describing the motivation behind this work?

jkotas

LGTM otherwise. Thanks!

jkotas · 2017-11-03T05:24:56Z

+        if ((vflag & INP_IPV4) == INP_IPV4)
        {
-            memcpy_s(iepi->AddressBytes, sizeof(IPEndPointInfo::AddressBytes), &in_pcb.inp_laddr.s_addr, 4);
+            memcpy(iepi->AddressBytes, &in_pcb.inp_laddr.s_addr, 4);


Would it be possible to keep memcpy_s here? It is mandated by SDL rules...

jkotas · 2017-11-03T05:27:45Z

+
+// These defines are temporary until all files have been migrated from C++ to C
+#ifdef __cplusplus
+#define BEGIN_DECLS extern "C" {


Nit: I am wondering whether it would be better to use EXTERN_C macro. It is the more standard way to deal with this problem - in the codebases that I live in at least.

jkotas · 2017-11-03T06:15:04Z

    {
-        delete[] buffer;
-        size_t tmpEstimatedSize;
-        if (!multiply_s(estimatedSize, static_cast<size_t>(2), &tmpEstimatedSize) ||


Can we keep multiply_s here? Another SDL requirement.

(Multiple places)

jkotas · 2017-11-03T06:16:56Z

-    PAL_ESHUTDOWN = 0x1006C,       // Socket shutdown.
-    PAL_EHOSTDOWN = 0x10070,       // Host is down.
-    PAL_ENODATA = 0x10071,         // No data available.
+    ERROR_E2BIG = 0x10001,           // Argument list too long.


Nit: Maybe this should Error_E2BIG to follow the <enum name>_<field name> convention.

jkotas · 2017-11-03T06:20:29Z

            if (err != 0)
            {
-                return SystemNative_ConvertErrorPlatformToPal(errno);
+                return static_cast<Error>(SystemNative_ConvertErrorPlatformToPal(errno));


These static_cast<Error> casts seem to be pretty frequent. Would it make sense to have ConvertErrorPlatformToPal local non-exporter variant (without the SystemNative_ prefix) that just returns Error?

They will all go away as soon as the transition is done, as we won't be able to return Error to managed anymore, and just int32_t

jkotas · 2017-11-03T06:23:02Z

Linux builds fail with 2:27: error: implicit declaration of function 'strerror_r' is invalid in C99 [

stephentoub

Other than Jan's comments, LGTM.

akoeplinger · 2017-11-06T17:28:26Z

 set(CMAKE_INCLUDE_CURRENT_DIR ON)
-set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=c11")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=c99")
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")


I guess this can be removed now too. There are also a few more CXX variables which are used and need replacement too:

CMAKE_CXX_COMPILER_VERSION

CMAKE_CXX_FLAGS_DEBUG

CLR_SANITIZE_CXX_FLAGS

jkotas · 2017-11-09T23:47:35Z

Linux build failing with errors like:

/mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/Native/Unix/System.Native/pal_io.c:66:1: error: _Static_assert is a C11-specific feature [-Werror,-Wc11-extensions]
static_assert(PAL_S_ISGID == S_ISGID, "");
^
/usr/include/assert.h:119:24: note: expanded from macro 'static_assert'
# define static_assert _Static_assert

jkotas · 2017-11-09T23:49:48Z

+
+// Multiplies a and b into result.
+// Returns true if safe, false if overflows.
+inline static int8_t multiply_s(size_t a, size_t b, size_t* result)


Should these rather return bool ?

bool doesn't exist with C99, so the usual replacement is int8_t. Maybe we could add a typedef int8_t bool.

#include <stdbool.h> ?

luhenry · 2017-11-14T17:13:36Z

@jkotas @stephentoub I updated the PR and it looks much greener now (still waiting for OSX). Could you please review again, so once we have the result for OSX we can get it merged? Thank you

jkotas

LGTM - just a few nits

jkotas · 2017-11-14T18:43:55Z

+        assert(src != NULL);
        assert(sizeInBytes >= count);
+#ifdef __cplusplus
        assert( // should be using memmove if this fails


Do we need these sort of ifdefs? I would expect the C version should work for C++ fine too. (Multiple places.)

C++ warns of using "old style" casting (with (const char*)), while static_cast produces no warning.

Disable the warning instead? Everything will get converted to the old style casts eventually, so we are not getting anything for being warned about using them.

jkotas · 2017-11-14T21:14:58Z

 #define lstat_ lstat
 #endif

+#ifdef static_assert


If there is no standard way to do static_assert in C, can we define our own?

E.g. this is the classic definition used by Windows SDK that should work in C too:

#define C_ASSERT(e) typedef char __C_ASSERT__[(e)?1:-1]

With newer Clang and GCC, static_assert is defined so the only case where we wouldn't compile these static asserts would be if you compile with an older version of Clang or GCC that do not support those.

So should be define the static_assert in the common header if it is not defined? It would be better than ifdefing each use.

15e76fb defines c_static_assert which fixes your comment. It forces us to define -Wno-typedef-redefinition, which might be quite detrimental. IMO since static_assert is not defined only on older compilers, just gating for its presence brings the best tradeoff.

jkotas · 2017-11-14T22:33:02Z

                    rawAddress,
                    unchecked((uint)addressBuffer.Length),
-                    addr.AddressFamily == AddressFamily.InterNetworkV6,
+                    unchecked((byte)(addr.AddressFamily == AddressFamily.InterNetworkV6 ? 1 : 0)),


Unchecked should not be necessary

jkotas · 2017-11-14T22:34:48Z

    /// Specifies the states of a Transmission Control Protocol (TCP) connection.
    /// </summary>
-    public enum TcpState
+    public enum TcpState : int


Not really necessary. This is a public type, so nobody can change it anyway.

Shouldn't we make sure of the size, so we map properly the structure MibTcpRow? It uses a TcpState field, and having a different field size between managed and native would produce hard to debug bugs.

The default size of enum in C# is always int. It is not hurting anything to add : int explicitly, but it is not obvious from looking at it why it is there.

jkotas · 2017-11-14T23:19:06Z

 install (TARGETS System.Native-Static DESTINATION .)
+
+# We have to cast struct to uint8_t and back
+set_source_files_properties(pal_networkstatistics.c PROPERTIES COMPILE_FLAGS -Wno-cast-align)


Disable it globally? Seems like a pretty useless warning for the kind of code that System.Native is.

luhenry · 2017-11-16T16:07:12Z

@jkotas @stephentoub I updated according to latest review.

jkotas · 2017-11-16T16:45:48Z

@luhenry Thank you!

tmds · 2017-11-16T18:55:53Z

@luhenry, just to understand, why doesn't mono use c++?

luhenry · 2017-11-16T20:11:59Z

@tmds one of Mono's strength resides on its ease to embed into other projects. Switching to C++ would make it harder for multiple technical reasons, with the main one being the C++ runtime, as long as we didn't figure out a story for that, we will not be able to do the switch. Also, C is available on much more platforms than C++ (let alone C++11), and Mono needs to be able to go to all these places where a modern C++ compiler might not have gone yet.

ghost · 2018-03-03T20:29:53Z

@luhenry, for the remaining C++ files in this repository, is there a plan to convert them as well?

* Move System.Native/pal_maphardwaretype.cpp to C * Switch C compiler from C11 to C99 * Move System.Native/pal_errno.cpp to C * Move System.Native/pal_tcpstate.cpp to C * Move System.Native/pal_networkstatistics.cpp to C * Move System.Native/pal_memory.cpp to C * Move System.Native/pal_io.cpp to C * Move System.Native/pal_networking.cpp to C * Define c_static_assert for compiler that don't support static_assert Commit migrated from dotnet/corefx@6a9f579

luhenry force-pushed the switch-to-c branch from 31b8b37 to bf41e3c Compare November 3, 2017 01:58

jkotas reviewed Nov 3, 2017

View reviewed changes

stephentoub reviewed Nov 3, 2017

View reviewed changes

karelz added the area-Meta label Nov 4, 2017

karelz assigned luhenry, jkotas and janvorli Nov 4, 2017

luhenry force-pushed the switch-to-c branch 3 times, most recently from 64a2bb1 to 7ca77f5 Compare November 6, 2017 17:13

akoeplinger reviewed Nov 6, 2017

View reviewed changes

luhenry force-pushed the switch-to-c branch 2 times, most recently from 4f8f60a to 09854f4 Compare November 9, 2017 18:01

jkotas reviewed Nov 9, 2017

View reviewed changes

luhenry force-pushed the switch-to-c branch 5 times, most recently from 7d170bd to b79c347 Compare November 14, 2017 15:25

jkotas reviewed Nov 14, 2017

View reviewed changes

luhenry force-pushed the switch-to-c branch from b79c347 to 15e76fb Compare November 15, 2017 16:00

luhenry added 2 commits November 15, 2017 11:21

Switch C compiler from C11 to C99

2cbdd31

Move System.Native/pal_maphardwaretype.cpp to C

e2851e5

luhenry force-pushed the switch-to-c branch from 15e76fb to b2ebc89 Compare November 15, 2017 16:41

luhenry added 2 commits November 15, 2017 11:42

Move System.Native/pal_errno.cpp to C

008fcae

Move System.Native/pal_tcpstate.cpp to C

cfc887b

luhenry added 5 commits November 15, 2017 11:42

Move System.Native/pal_networkstatistics.cpp to C

a31a367

Move System.Native/pal_memory.cpp to C

74a3396

Move System.Native/pal_io.cpp to C

6d90664

Move System.Native/pal_networking.cpp to C

ee5070b

Define c_static_assert for compiler that don't support static_assert

f13351a

luhenry force-pushed the switch-to-c branch from b2ebc89 to f13351a Compare November 15, 2017 16:44

jkotas approved these changes Nov 16, 2017

View reviewed changes

jkotas merged commit 6a9f579 into dotnet:master Nov 16, 2017

karelz added this to the 2.1.0 milestone Nov 18, 2017

jkotas mentioned this pull request Jan 16, 2018

Use SIGCHLD to trigger Process waitpid check #26291

Merged

ghost mentioned this pull request Mar 23, 2018

WIP - initial serial port support for Linux #28415

Closed

stephentoub mentioned this pull request Apr 17, 2018

bottom part of serial port support on Unix #29033

Merged

Conversation

luhenry commented Nov 3, 2017 • edited by karelz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SunnyWar commented Nov 3, 2017

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Nov 3, 2017

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Nov 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luhenry commented Nov 14, 2017

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luhenry commented Nov 16, 2017

Uh oh!

jkotas commented Nov 16, 2017

Uh oh!

tmds commented Nov 16, 2017

Uh oh!

luhenry commented Nov 16, 2017

Uh oh!

ghost commented Mar 3, 2018

Uh oh!

Reviewers

luhenry commented Nov 3, 2017 •

edited by karelz

Loading

jkotas Nov 3, 2017 •

edited

Loading