[libc-commits] [PATCH] D145913: [libc] Add initial support for an RPC mechanism for the GPU

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added a comment.

I stumbled upon this code while looking at `write_to_stderr` and I think that the `PRINT_TO_STDERR` opcode is buggy (see comments). Also since we're exchanging messages of 64B, why waste 8B on the Opcode? Do we need such a huge opcode space ?
I'm working on a patch to try to fix these issues, I'll ping back when it's ready for discussion.



================
Comment at: libc/src/__support/OSUtil/gpu/io.cpp:24
+          inline_memcpy(reinterpret_cast<char *>(&buffer->data[1]), &msg[i],
+                        (length > buffer_len ? buffer_len : length));
+        },
----------------
The number of bytes copied at each round are either `buffer_len` (i.e. 56B) or the **total** length of the string, it should be the remainder.


================
Comment at: libc/utils/gpu/loader/amdgpu/Loader.cpp:50
+        case __llvm_libc::rpc::Opcode::PRINT_TO_STDERR: {
+          fputs(reinterpret_cast<const char *>(&buffer->data[1]), stderr);
+          break;
----------------
If the message's length is greater than 56B, `&buffer->data[1]` contains only a fragment of the message and is not null-terminated, leading `fputs` to read past it's allocated buffer.


================
Comment at: libc/utils/gpu/loader/amdgpu/Loader.cpp:314
+  void *buffer;
+  if (hsa_status_t err = hsa_amd_memory_pool_allocate(
+          finegrained_pool, sizeof(__llvm_libc::cpp::Atomic<int>),
----------------
Are there any guarantees that this piece of memory has the same alignment that `__llvm_libc::cpp::Atomic<int>`.
Same for the Cuda loader.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145913