[libc-commits] [PATCH] D147054: [libc] Begin implementing a library for the RPC server

[Jon Chesterfield via Phabricator via libc-commits]

JonChesterfield added inline comments.


================
Comment at: libc/utils/gpu/loader/amdgpu/Loader.cpp:40
 
+static void handle_error(rpc_status_t) {
+  fprintf(stderr, "Failure in the RPC server\n");
----------------
Call into the other one, 
```
static void handle_error(rpc_status_t) {
  handle_error("Failure in the RPC server");
}
```


================
Comment at: libc/utils/gpu/loader/nvptx/Loader.cpp:51
+
 static void handle_error(const char *msg) {
   fprintf(stderr, "%s\n", msg);
----------------
i still really dislike the copy/paste going on here


================
Comment at: libc/utils/gpu/loader/nvptx/Loader.cpp:207
+      },
+      &memory_stream);
+  rpc_register_callback(
----------------
Does this work? It looks like the same stream running the kernel is being used to provide malloc/free, and I'd expect that to deadlock


================
Comment at: libc/utils/gpu/server/Server.cpp:35
+      : num_devices(num_devices),
+        devices(std::unique_ptr<Device[]>(new Device[num_devices])),
+        reference_count(0u) {}
----------------
Could we go with a vector of Device instead of the new+array construct?


================
Comment at: libc/utils/gpu/server/Server.cpp:44
+
+static State *state;
+
----------------
Why is this a heap allocated thing, as opposed to `static State state;` ?


================
Comment at: libc/utils/gpu/server/Server.cpp:51
+
+  if (state->reference_count == std::numeric_limits<uint32_t>::max())
+    return RPC_STATUS_ERROR;
----------------
Could make the counter 64 bit and delete the test against max as a counter >= address space size can't overflow

In general the DIY reference counting is a bit odd - is there a reason this isn't a shared_ptr?


================
Comment at: libc/utils/gpu/server/Server.cpp:107
+    case rpc::Opcode::WRITE_TO_STDOUT: {
+      uint64_t sizes[rpc::MAX_LANE_SIZE] = {0};
+      void *strs[rpc::MAX_LANE_SIZE] = {nullptr};
----------------
I'm still hopeful that we'll come up with a better idea than rpc::MAX_LANE_SIZE


================
Comment at: libc/utils/gpu/server/Server.h:57
+/// A function used to free the \p ptr previously allocated.
+typedef void(rpc_free_ty)(void *ptr, void *data);
+
----------------
This looks like the type of a function, not the type of a function pointer. It's used as an argument to functions where it'll decay to the pointer type. More conventionally written with an extra *

typedef void(*rpc_free_ty)(void *ptr, void *data);

Is there a benefit to declaring this as the function type as opposed to the function pointer type?


================
Comment at: libc/utils/gpu/server/Server.h:70
+/// Shut down the rpc interface.
+rpc_status_t rpc_shutdown();
+
----------------
Want `(void)` if this is meant to be usable from C (the guards about suggest it is)

C++ thinks foo() is a function of no arguments. C thinks it's some aberration from the past (though that might have been dropped in the last standard). 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147054/new/

https://reviews.llvm.org/D147054