refactor: Use strong typedef instead of struct for Socket.

[iphydf]

Sparse checks it. This is neater than using a struct, which has some
slightly weird syntax at times. This also reduces the risk of someone adding
another struct member.

---

This change is 

[codecov]

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (9fe18b1) 73.75% compared to head (969e3a2) 73.76%.

Files	Patch %	Lines
testing/fuzzing/fuzz_support.cc	50.00%	23 Missing ⚠️
toxcore/network_test_util.cc	0.00%	3 Missing ⚠️
toxcore/network.c	93.93%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2640      +/-   ##
==========================================
+ Coverage   73.75%   73.76%   +0.01%     
==========================================
  Files         148      148              
  Lines       30481    30479       -2     
==========================================
+ Hits        22480    22482       +2     
+ Misses       8001     7997       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nurupo]

This is slightly better than the typedef struct Socket. Are we going to use Sparse for anything else in the future?

Reviewed 11 of 12 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)

---

other/docker/sparse/sparse.Dockerfile line 34 at r1 (raw file):

#RUN sparsec $(pkg-config --cflags --libs libsodium opus vpx) crash.c

COPY other/docker/sparse/Makefile /work/c-toxcore/

I don't see other/docker/sparse/Makefile as one of committed files, and it doesn't seem like anything in here is supposed to generate it.

[iphydf]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo)

---

other/docker/sparse/sparse.Dockerfile line 34 at r1 (raw file):

Previously, nurupo wrote…

I don't see other/docker/sparse/Makefile as one of committed files, and it doesn't seem like anything in here is supposed to generate it.

It was missing (.gitignore). Added.

[Green-Sky]

Reviewed 8 of 12 files at r1, 2 of 2 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo)

[Green-Sky]

toxcore/network.h line 32 at r2 (raw file):

Previously, iphydf wrote…

Yes, I like it better. Done.

LGTM

[nurupo]

Are we going to use Sparse for anything else in the future?

[iphydf]

Are we going to use Sparse for anything else in the future?

Can you elaborate on "anything else"? If you mean "any other types than socket", then yes, as you can see from the other 2 pull requests. If you mean "other than the currently open PRs", then yes, I plan to do more bitwise types. If you mean "other sparse features than bitwise", then maybe.

[nurupo]

I meant other features of Sparse. Not very sold on the value of brining an entire tool and using non-standard keywords/macros/attributes for what is a slight improvement over typedef struct Socket { int sock; } Socket;, which works for everyone with a C compiler, without any extra tools required or needing to learn any extra keywords/macros/attributes.

In this PR, it looks like int net_socket_to_native(Socket sock) could be changed to return sock.sock and Socket net_socket_from_native(int sock) to return (Socket) {sock} and I would expect it to work. The only difference of using struct instead of bitwise is that anyone at any point can directly access sock.sock without using net_socket_to_native(), but I'm not sure if this is really a con, because if all interfaces require Socket type then there is no reason for anyone to access sock.sock directly aside from interfacing with native OS functions, which is what we want.

Now, if sparse provided more value than that, then it would be easier to justify its use, which is why I'm asking if you have any plans on using it for anything else aside from struct as a unique type replacement / aside from what can be achieved in plain C with arguably the same success.

[iphydf]

I'd like to use strong typedefs for pretty much every int we have in toxcore. Doing all the struct wrapping/unwrapping everywhere is going to be a bit of a pain. That pain is the main reason I haven't done it or recommended it in PR reviews, yet. Also, operator== doesn't work on structs, so you always have to unwrap for equality checks, which is worse for things that aren't sockets, but e.g. GC_Peer_Id.

But yes, I'm also planning to use the address_space feature to enforce distinctions between stack and heap allocated objects, and require e.g. that only heap allocated data can be stored in structs (with minor exceptions, but then those structs can only be stack-allocated themselves).

[nurupo]

Also, operator== doesn't work on structs, so you always have to unwrap for equality checks, which is worse for things that aren't sockets, but e.g. GC_Peer_Id.

These are one member structs, peer_id1 == peer_id2 vs peer_id1.value == peer_id2.value is not that much of a difference. Also no idea what do you mean by GC_Peer_Id being worse than Socket in that regard, they seem equivalent to me.

[nurupo]

I want to point out that most of the changes in this PR are not for adding bitwise, they are just to change function signatures from taking int sock to Socket sock, which should have really been done long time ago, when Socket was still a struct.

Just to show how many lines in here are for the bitwise feature, let me make a patch on top of this PR reverting it back to use struct Socket again:

diff --git a/toxcore/network.c b/toxcore/network.c
index d8594098..9f2f08cb 100644
--- a/toxcore/network.c
+++ b/toxcore/network.c
@@ -370,12 +370,12 @@ IP6 get_ip6_loopback(void)
 
 int net_socket_to_native(Socket sock)
 {
-    return (force int)sock;
+    return sock.value;
 }
 
 Socket net_socket_from_native(int sock)
 {
-    return (force Socket)sock;
+    return (Socket) {sock};
 }
 
 Socket net_invalid_socket(void)
@@ -475,7 +475,7 @@ bool net_family_is_tox_tcp_ipv6(Family family)
 
 bool sock_valid(Socket sock)
 {
-    return sock != net_invalid_socket();
+    return sock.value != net_invalid_socket().value;
 }
 
 struct Network_Addr {
diff --git a/toxcore/network.h b/toxcore/network.h
index 2e951162..015e214b 100644
--- a/toxcore/network.h
+++ b/toxcore/network.h
@@ -27,7 +27,9 @@ extern "C" {
  */
 typedef struct Network_Addr Network_Addr;
 
-typedef bitwise int Socket;
+typedef struct Socket {
+    int value;
+} Socket;
 
 int net_socket_to_native(Socket sock);
 Socket net_socket_from_native(int sock);

Such a small patch and we got our unique Socket type back without needing any extra tools. I don't really see anything in this diff that is so inconvenient that is worth adding an extra tool for. Does anyone else see anything in this diff worth adding an extra tool for?

---

Also, using struct Socket has its own advantages. Using struct Socket is better than using Sparse's bitwise because Sparse is limited to parsing only C code. Sparse doesn't parse C++, it can't check C++ code, while struct Socket is understood by a C++ compiler and will get checked by it.

In fact, if you try to apply my from patch above and build toxcore, you will notice that network_test_util.[hh|cc]

is failing to build.

Scanning dependencies of target test_util
[ 89%] Building CXX object CMakeFiles/test_util.dir/toxcore/DHT_test_util.cc.o
In file included from ./toxcore/DHT_test_util.cc:10:
./toxcore/network_test_util.hh:47:9: error: ‘int Test_Network::close(void*, int)’ marked ‘override’, but does not override
   47 |     int close(void *obj, int sock) override;
      |         ^~~~~
./toxcore/network_test_util.hh:48:9: error: ‘int Test_Network::accept(void*, int)’ marked ‘override’, but does not override
   48 |     int accept(void *obj, int sock) override;
      |         ^~~~~~
./toxcore/network_test_util.hh:49:9: error: ‘int Test_Network::bind(void*, int, const Network_Addr*)’ marked ‘override’, but does not override
   49 |     int bind(void *obj, int sock, const Network_Addr *addr) override;
      |         ^~~~
./toxcore/network_test_util.hh:50:9: error: ‘int Test_Network::listen(void*, int, int)’ marked ‘override’, but does not override
   50 |     int listen(void *obj, int sock, int backlog) override;
      |         ^~~~~~
./toxcore/network_test_util.hh:51:9: error: ‘int Test_Network::recvbuf(void*, int)’ marked ‘override’, but does not override
   51 |     int recvbuf(void *obj, int sock) override;
      |         ^~~~~~~
./toxcore/network_test_util.hh:52:9: error: ‘int Test_Network::recv(void*, int, uint8_t*, size_t)’ marked ‘override’, but does not override
   52 |     int recv(void *obj, int sock, uint8_t *buf, size_t len) override;
      |         ^~~~
./toxcore/network_test_util.hh:53:9: error: ‘int Test_Network::recvfrom(void*, int, uint8_t*, size_t, Network_Addr*)’ marked ‘override’, but does not override
   53 |     int recvfrom(void *obj, int sock, uint8_t *buf, size_t len, Network_Addr *addr) override;
      |         ^~~~~~~~
./toxcore/network_test_util.hh:54:9: error: ‘int Test_Network::send(void*, int, const uint8_t*, size_t)’ marked ‘override’, but does not override
   54 |     int send(void *obj, int sock, const uint8_t *buf, size_t len) override;
      |         ^~~~
./toxcore/network_test_util.hh:55:9: error: ‘int Test_Network::sendto(void*, int, const uint8_t*, size_t, const Network_Addr*)’ marked ‘override’, but does not override
   55 |     int sendto(
      |         ^~~~~~
./toxcore/network_test_util.hh:57:9: error: conflicting return type specified for ‘virtual int Test_Network::socket(void*, int, int, int)’
   57 |     int socket(void *obj, int domain, int type, int proto) override;
      |         ^~~~~~
./toxcore/network_test_util.hh:32:27: note: overridden function is ‘virtual Socket Network_Class::socket(void*, int, int, int)’
   32 |     virtual net_socket_cb socket = 0;
      |                           ^~~~~~
./toxcore/network_test_util.hh:58:9: error: ‘int Test_Network::socket_nonblock(void*, int, bool)’ marked ‘override’, but does not override
   58 |     int socket_nonblock(void *obj, int sock, bool nonblock) override;
      |         ^~~~~~~~~~~~~~~
./toxcore/network_test_util.hh:59:9: error: ‘int Test_Network::getsockopt(void*, int, int, int, void*, size_t*)’ marked ‘override’, but does not override
   59 |     int getsockopt(
      |         ^~~~~~~~~~
./toxcore/network_test_util.hh:61:9: error: ‘int Test_Network::setsockopt(void*, int, int, int, const void*, size_t)’ marked ‘override’, but does not override
   61 |     int setsockopt(
      |         ^~~~~~~~~~
gmake[2]: *** [CMakeFiles/test_util.dir/build.make:82: CMakeFiles/test_util.dir/toxcore/DHT_test_util.cc.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:304: CMakeFiles/test_util.dir/all] Error 2
gmake: *** [Makefile:160: all] Error 2

It looks like @iphydf forgot to update the signatures from int sock to Socket sock in there, because Sparse didn't check C++ files and didn't remind him. But if you use struct Socket then even if you forget to change the types, the type-checking is enforced by the compiler and the build would simply fail, pointing you at what you forgot to do.

Here is the patch to fix it. It should also be applied to this PR.

diff --git a/toxcore/network_test_util.cc b/toxcore/network_test_util.cc
index 8db0136b..fdf55f2a 100644
--- a/toxcore/network_test_util.cc
+++ b/toxcore/network_test_util.cc
@@ -24,49 +24,49 @@ Network_Funcs const Network_Class::vtable = {
     Method<net_freeaddrinfo_cb, Network_Class>::invoke<&Network_Class::freeaddrinfo>,
 };
 
-int Test_Network::close(void *obj, int sock) { return net->funcs->close(net->obj, sock); }
-int Test_Network::accept(void *obj, int sock) { return net->funcs->accept(net->obj, sock); }
-int Test_Network::bind(void *obj, int sock, const Network_Addr *addr)
+int Test_Network::close(void *obj, Socket sock) { return net->funcs->close(net->obj, sock); }
+Socket Test_Network::accept(void *obj, Socket sock) { return net->funcs->accept(net->obj, sock); }
+int Test_Network::bind(void *obj, Socket sock, const Network_Addr *addr)
 {
     return net->funcs->bind(net->obj, sock, addr);
 }
-int Test_Network::listen(void *obj, int sock, int backlog)
+int Test_Network::listen(void *obj, Socket sock, int backlog)
 {
     return net->funcs->listen(net->obj, sock, backlog);
 }
-int Test_Network::recvbuf(void *obj, int sock) { return net->funcs->recvbuf(net->obj, sock); }
-int Test_Network::recv(void *obj, int sock, uint8_t *buf, size_t len)
+int Test_Network::recvbuf(void *obj, Socket sock) { return net->funcs->recvbuf(net->obj, sock); }
+int Test_Network::recv(void *obj, Socket sock, uint8_t *buf, size_t len)
 {
     return net->funcs->recv(net->obj, sock, buf, len);
 }
-int Test_Network::recvfrom(void *obj, int sock, uint8_t *buf, size_t len, Network_Addr *addr)
+int Test_Network::recvfrom(void *obj, Socket sock, uint8_t *buf, size_t len, Network_Addr *addr)
 {
     return net->funcs->recvfrom(net->obj, sock, buf, len, addr);
 }
-int Test_Network::send(void *obj, int sock, const uint8_t *buf, size_t len)
+int Test_Network::send(void *obj, Socket sock, const uint8_t *buf, size_t len)
 {
     return net->funcs->send(net->obj, sock, buf, len);
 }
 int Test_Network::sendto(
-    void *obj, int sock, const uint8_t *buf, size_t len, const Network_Addr *addr)
+    void *obj, Socket sock, const uint8_t *buf, size_t len, const Network_Addr *addr)
 {
     return net->funcs->sendto(net->obj, sock, buf, len, addr);
 }
-int Test_Network::socket(void *obj, int domain, int type, int proto)
+Socket Test_Network::socket(void *obj, int domain, int type, int proto)
 {
     return net->funcs->socket(net->obj, domain, type, proto);
 }
-int Test_Network::socket_nonblock(void *obj, int sock, bool nonblock)
+int Test_Network::socket_nonblock(void *obj, Socket sock, bool nonblock)
 {
     return net->funcs->socket_nonblock(net->obj, sock, nonblock);
 }
 int Test_Network::getsockopt(
-    void *obj, int sock, int level, int optname, void *optval, size_t *optlen)
+    void *obj, Socket sock, int level, int optname, void *optval, size_t *optlen)
 {
     return net->funcs->getsockopt(net->obj, sock, level, optname, optval, optlen);
 }
 int Test_Network::setsockopt(
-    void *obj, int sock, int level, int optname, const void *optval, size_t optlen)
+    void *obj, Socket sock, int level, int optname, const void *optval, size_t optlen)
 {
     return net->funcs->setsockopt(net->obj, sock, level, optname, optval, optlen);
 }
diff --git a/toxcore/network_test_util.hh b/toxcore/network_test_util.hh
index 3fe91441..1f437d79 100644
--- a/toxcore/network_test_util.hh
+++ b/toxcore/network_test_util.hh
@@ -44,22 +44,22 @@ struct Network_Class {
 class Test_Network : public Network_Class {
     const Network *net = REQUIRE_NOT_NULL(os_network());
 
-    int close(void *obj, int sock) override;
-    int accept(void *obj, int sock) override;
-    int bind(void *obj, int sock, const Network_Addr *addr) override;
-    int listen(void *obj, int sock, int backlog) override;
-    int recvbuf(void *obj, int sock) override;
-    int recv(void *obj, int sock, uint8_t *buf, size_t len) override;
-    int recvfrom(void *obj, int sock, uint8_t *buf, size_t len, Network_Addr *addr) override;
-    int send(void *obj, int sock, const uint8_t *buf, size_t len) override;
+    int close(void *obj, Socket sock) override;
+    Socket accept(void *obj, Socket sock) override;
+    int bind(void *obj, Socket sock, const Network_Addr *addr) override;
+    int listen(void *obj, Socket sock, int backlog) override;
+    int recvbuf(void *obj, Socket sock) override;
+    int recv(void *obj, Socket sock, uint8_t *buf, size_t len) override;
+    int recvfrom(void *obj, Socket sock, uint8_t *buf, size_t len, Network_Addr *addr) override;
+    int send(void *obj, Socket sock, const uint8_t *buf, size_t len) override;
     int sendto(
-        void *obj, int sock, const uint8_t *buf, size_t len, const Network_Addr *addr) override;
-    int socket(void *obj, int domain, int type, int proto) override;
-    int socket_nonblock(void *obj, int sock, bool nonblock) override;
+        void *obj, Socket sock, const uint8_t *buf, size_t len, const Network_Addr *addr) override;
+    Socket socket(void *obj, int domain, int type, int proto) override;
+    int socket_nonblock(void *obj, Socket sock, bool nonblock) override;
     int getsockopt(
-        void *obj, int sock, int level, int optname, void *optval, size_t *optlen) override;
+        void *obj, Socket sock, int level, int optname, void *optval, size_t *optlen) override;
     int setsockopt(
-        void *obj, int sock, int level, int optname, const void *optval, size_t optlen) override;
+        void *obj, Socket sock, int level, int optname, const void *optval, size_t optlen) override;
     int getaddrinfo(void *obj, int family, Network_Addr **addrs) override;
     int freeaddrinfo(void *obj, Network_Addr *addrs) override;
 };

I guess I will add it as a commit to iphy's branch.

---

Anyway, I still not sold on the value of bringing an entire tool and using non-standard keywords/macros/attributes for, as I just discovered, arguably worse version of typedef struct Socket { int value; } Socket;, when typedef struct Socket { int value; } Socket; already works for everyone with a C and C++ compilers, without any extra tools required or needing to learn any extra keywords/macros/attributes.

I haven't looked into the other 2 PRs, maybe they shed some light on why would we want to use an extra tool for this?

---

But yes, I'm also planning to use the address_space feature to enforce distinctions between stack and heap allocated objects, and require e.g. that only heap allocated data can be stored in structs (with minor exceptions, but then those structs can only be stack-allocated themselves).

So, basically, to make sure we don't accidentally store things would get destroyed at the end of the current scope in a heap-allocated struct that is supposed to outlive the current scope? Sounds interesting, but I would imagine the tools we already use cover this already, no? Well, we can discuss it more in-depth when you open a PR for it.

[iphydf]

Bitwise works in the public API and doesn't break clients. Bitwise is used in the Linux kernel, structs are a somewhat uncommon way of doing things in C (but I like it).

I'm not aware of any tools that can currently do the kind of cross function boundary checks in a sound way (no false negatives) that could be achieved with sparse address space annotations. I could implement it in cimple but it's not easy.

[iphydf]

sock.value == family.value no error. Sparse prevents that. I guess the most complete protection would be to do both: bitwise int inside a struct.

[iphydf]

I've made that change. It's now both a struct and a bitwise int.

[nurupo]

Bitwise works in the public API and doesn't break clients.

Hm, that's a good point. I recall we have slightly touched on using structs for distinct types in the API (and even opaque pointers / handles for things the client shouldn't really read the value of or rely on, like friend numbers, though it would cost an extra heap allocation to do) in #2518 (comment). I'm not too sure which types in the public API we would want to make distinct, so depending on the type, a struct might be too much and bitwise might be preferred. Would need to see an actual PR or a list of public API function arguments that we want to make distinct first to really tell.

(I will take look at the changes a bit later, don't have time now as of writing this.)

[iphydf]

I think all the typedef int ones we recently did in the public API can be distinct types. The problem with structs there becomes ABI compatibility (depending on the platform, not on x86) and complexity to make bindings to struct passing functions in FFI, so I'd probably not do that. I'll need to look into how much of a problem that actually is.

[nurupo]

Reviewed 1 of 12 files at r1, 2 of 2 files at r2, 4 of 4 files at r3, 2 of 2 files at r4, 3 of 5 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: 0 of 2 approvals obtained (waiting on @JFreegman and @robinlinden)

[nurupo]

0 of 2 approvals obtained (waiting on @JFreegman and @robinlinden)

Ah, sorry about that. Wanted to have them look over the PR and provide some input on whether this bitwise changes makes any sense as I couldn't decide if bitwise is really that necessary as it looed like struct could do just fine, but then I did some code edits and #2640 (comment) happened, which made me decide that that a struct is better, at least in this case. (I also decided that it makes sense to keep Sparse, e.g. the API seems like a valid reason, and #2640 (comment) shows what bitwise can do but struct can't.) So I don't need them reviewing the PR anymore to decide. Obviously they are welcome to review it if they want to, but the original reason why I wanted them to review is no more.