[libc-commits] [PATCH] D147738: [libc] Enable linux directory entries syscalls in riscv64

[Mikhail Ramalho via Phabricator via libc-commits]

mikhail.ramalho marked 4 inline comments as done.
mikhail.ramalho added inline comments.


================
Comment at: libc/include/llvm-libc-types/ino64_t.h:12
+
+typedef __UINTPTR_TYPE__ ino64_t;
+
----------------
sivachandra wrote:
> As far as I can tell, this is exactly the same as `ino_t`. Why do we need this?
I'm following glibc here, as it defines both types separately, despite them being the same. Do you think it's better to remove it and just typedef ino64_t to be ino_t?


================
Comment at: libc/include/llvm-libc-types/struct_dirent.h:12
 
+#include <sys/syscall.h> // For syscall definitions.
+
----------------
sivachandra wrote:
> This will pollute user namespace.
Do you have any suggestions on how to have `SYS_getdents64` available here?


================
Comment at: libc/include/llvm-libc-types/struct_dirent.h:33
 #endif
+  unsigned char d_type;
   // The user code should use strlen to determine actual the size of d_name.
----------------
sivachandra wrote:
> Have you checked to see if this breaks x86_64?
No regressions in x86_64.


================
Comment at: libc/src/__support/File/dir.cpp:45
 
+#ifdef SYS_getdents64
   struct ::dirent *d = reinterpret_cast<struct ::dirent *>(buffer + readptr);
----------------
sivachandra wrote:
> This file is supposed to contain the platform independent implementation. Can we improve the platform abstraction to push variance within Linux into the Linux implementation?
do you mean creating a linux/ dir with the `Dir::read()` implementation?


================
Comment at: libc/src/__support/File/dir.cpp:86
+  size_t d_name_offset = offsetof(linux_dirent, d_name);
+  memcpy(buffer + readptr + d_name_offset + 1, buffer + readptr + d_name_offset,
+         strlen(ld->d_name));
----------------
sivachandra wrote:
> Calling `memcpy` on overlapping source and destination memory is undefined.
changed to memmove.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147738