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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/include/llvm-libc-types/ino64_t.h:12
+
+typedef __UINTPTR_TYPE__ ino64_t;
+
----------------
As far as I can tell, this is exactly the same as `ino_t`. Why do we need this?


================
Comment at: libc/include/llvm-libc-types/struct_dirent.h:12
 
+#include <sys/syscall.h> // For syscall definitions.
+
----------------
This will pollute user namespace.


================
Comment at: libc/include/llvm-libc-types/struct_dirent.h:23
+#ifdef SYS_getdents64
+typedef ino64_t ino_t;
+typedef off64_t off_t;
----------------
This type definition should live in `llvm-libc-types/ino_t.h`. Similar structuring for `off_t`;


================
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.
----------------
Have you checked to see if this breaks x86_64?


================
Comment at: libc/src/__support/File/dir.cpp:45
 
+#ifdef SYS_getdents64
   struct ::dirent *d = reinterpret_cast<struct ::dirent *>(buffer + readptr);
----------------
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?


================
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));
----------------
Calling `memcpy` on overlapping source and destination memory is undefined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147738