[libc] Enable linux directory entries syscalls in riscv64

[mikhail.ramalho]

Summary

This patch updates the struct dirent to be on par with glibc (by adding
a missing d_type member) and update the readdir call to use SYS_getdents64
instead of SYS_getdents.

The change to SYS_getdents64 was necessary because SYS_getdents returns
a slightly different struct (without the d_type member), so when
returning a pointer to our internal buffer holding the directory entries,
the contents of the buffer would be off-by-one, resulting in garbage.

To support SYS_getdents, buffer is rewritten everytime the syscall is
called: as per the documentation, d_type is always the last byte in the
structure, so the rewrite:

1. gets d_type from the last byte in the structure
2. shifts d_name by one
3. stores d_type where d_name was previously
4. clears previous d_type location

[mikhail.ramalho]

Add support for SYS_getdents

[mikhail.ramalho]

Fix error message

[michaelrj]

libc/src/__support/File/dir.cpp

45

this struct should be moved to its own file in include/llvm-libc-types/ to match struct dirent

51

you should probably add a comment clarifying that the 1 here represents the minimum width of this string, and that this struct has a variable length.

[mikhail.ramalho]

libc/src/__support/File/dir.cpp

45

struct linux_dirent is kinda an oddball... glibc never defines it, see https://man7.org/linux/man-pages/man2/getdents.2.html:
Note: There is no definition of struct linux_dirent in glibc; see NOTES.

The notes mention:

Library support for getdents64() was added in glibc 2.30; Glibc
does not provide a wrapper for getdents(); call getdents() (or
getdents64() on earlier glibc versions) using syscall(2).  In
that case you will need to define the linux_dirent or
linux_dirent64 structure yourself.

Maybe I can add a comment just explaining that?

I also doesn't seem to be exposed by the kernel: https://elixir.bootlin.com/linux/v4.7/source/fs/readdir.c#L150

51

will do!

[sivachandra]

libc/include/llvm-libc-types/ino64_t.h

12 ↗

(On Diff #511741)

As far as I can tell, this is exactly the same as ino_t. Why do we need this?

libc/include/llvm-libc-types/struct_dirent.h

12

This will pollute user namespace.

17

This type definition should live in llvm-libc-types/ino_t.h. Similar structuring for off_t;

21

Have you checked to see if this breaks x86_64?

libc/src/__support/File/dir.cpp

41

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?

81

Calling memcpy on overlapping source and destination memory is undefined.

[mikhail.ramalho]

libc/include/llvm-libc-types/ino64_t.h

12 ↗

(On Diff #511741)

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?

libc/include/llvm-libc-types/struct_dirent.h

12

Do you have any suggestions on how to have SYS_getdents64 available here?

21

No regressions in x86_64.

libc/src/__support/File/dir.cpp

41

do you mean creating a linux/ dir with the Dir::read() implementation?

81

changed to memmove.

[mikhail.ramalho]

• Added comment about string lenght
• changed memcopy to memmove to prevent UB

[jrtc27]

libc/include/llvm-libc-types/ino64_t.h

12 ↗

(On Diff #514209)

uintptr_t is most certainly not the same thing as ino64_t. I remember complaining about ino_t and time_t(?) being typedef'ed to that a while back but nobody fixed it. NAK on this though; use proper types.

[mikhail.ramalho]

libc/include/llvm-libc-types/ino64_t.h

12 ↗

(On Diff #514209)

I just double-checked and in glibc it's defined as __uint64_t in 32 bits and unsigned long int in 64 bits.

We can use __UINT64_TYPE__ then (long long unsigned int in 32 bits and long unsigned int in 64 bits).

[mikhail.ramalho]

Changed ino_t and ino64_t type to UINT64_TYPE

[jrtc27]

libc/include/llvm-libc-types/ino64_t.h

12 ↗

(On Diff #514209)

FYI Darwin (unnecessarily? for consistency?) uses unsigned long long even on 64-bit for uint64_t but that's fine, it does still define ino_t as uint64_t.

[michaelrj]

libc/include/llvm-libc-types/struct_dirent.h

17

since ino64_t and ino_t are both typedef'd to uint64_t now, this should be unnecessary. Same for off64_t and off_t.

libc/src/__support/File/dir.cpp

45

glibc not providing the struct publicly doesn't mean that we can't. I think it's cleaner to have the type defined in llvm-libc-types and then use #ifdef to include the appropriate struct.

[mikhail.ramalho]

• Added new struct linux_dirent to llvm-libc-types
• Removed both ino64_t and off64_t

[mikhail.ramalho]

libc/src/__support/File/dir.cpp

45

got it

[mikhail.ramalho]

Add linux_dirent to dirent.h only if unix is defined

[michaelrj]

libc/include/dirent.h.def

14 ↗

(On Diff #515327)

if this is a linux type then it probably makes more sense for this to be __linux__

libc/include/llvm-libc-types/off64_t.h

12 ↗

(On Diff #515327)

I didn't mean you should delete off64_t completely, only that you didn't need it in that specific place.

libc/src/__support/File/dir.cpp

14–15

now that we've moved to cpp/new.h for allocation, you should be able to remove the stdlib include as well as the string include.

47

We generally avoid calling public names of libc function in our internal code, if you want to call strlen you should use the internal implementation in string/string_utils.h called string_length.

65

You should also use the internal memmove which is in src/string/memory_utils/memmove_implementations.h and memset (same pattern)

[mikhail.ramalho]

libc/include/llvm-libc-types/off64_t.h

12 ↗

(On Diff #515327)

We don't actually need it since, as you mentioned, off_t and off64_t are the same types (the same for ino_t and ino64_t). Do we want to keep them both in the code base?

[mikhail.ramalho]

• Changed ifdef unix to linux
• Changed strlen, memset and memmove to use libc's internal implementation

[mikhail.ramalho]

ping.

@sivachandra can you clarify what you mean by "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?"?

[sivachandra]

libc/src/__support/File/dir.cpp

41

This file is a platform independent abstraction for the dirent API. So, all references to Linux pieces and variances should move to linux_dir.cpp. Which means that we should not have the #ifdef for Linux syscalls here. I have not looked in to it in detail, but what I was trying to say that, if are unable to fit these variances in the current platform abstraction, we should enhance the abstraction instead of leaking platform specific details in to this file.

[mikhail.ramalho]

Moved linux specific implementation to linux_dir.cpp

[mikhail.ramalho]

@sivachandra does my last chance address your comment?

The Linux-specific handling was moved to the linux_dir.cpp file.

[sivachandra]

In D147738#4306199, @mikhail.ramalho wrote:

@sivachandra does my last chance address your comment?

That particular comment was addressed I believe. I want to do some homework wrt the types used in this patch. I will get back shortly.

libc/include/llvm-libc-types/struct_linux_dirent.h

1 ↗

(On Diff #517365)

This is an internal type so should not be define in a public header file.

[mikhail.ramalho]

libc/include/llvm-libc-types/struct_linux_dirent.h

1 ↗

(On Diff #517365)

This sounds contradictory to what @michaelrj suggested in one of his comments:

glibc not providing the struct publicly doesn't mean that we can't. I think it's cleaner to have the type defined in llvm-libc-types and then use #ifdef to include the appropriate struct.

[michaelrj]

libc/include/llvm-libc-types/struct_linux_dirent.h

1 ↗

(On Diff #517365)

From what I can tell from the man page, the linux_dirent struct is somewhat standard, but also often not defined by the C library. Given that, I personally prefer defining it like the other dirent struct as a public type. If we decide not to define it publicly, I think we should still define it in a separate header and use ifdef to include it as needed. That header might end up in __support/File/linux.

[sivachandra]

libc/include/llvm-libc-types/struct_linux_dirent.h

1 ↗

(On Diff #517365)

Sorry, I missed @michaelrj's comment - AFAICT, linux_dirent is only relevant for the syscall, so we should not expose it publicly. We have examples of other such types in the libc: https://github.com/llvm/llvm-project/blob/main/libc/src/signal/linux/signal_utils.h#L29. And since it would be an internal type, you should name it LinuxDirent.

[sivachandra]

libc/include/llvm-libc-types/struct_linux_dirent.h

1 ↗

(On Diff #517365)

From what I can tell from the man page, the linux_dirent struct is somewhat standard, but also often not defined by the C library.

You are perhaps looking at https://man7.org/linux/man-pages/man2/getdents.2.html, which says:

These are not the interfaces you are interested in.  Look at readdir(3) for the POSIX-conforming C library interface.  This page documents the bare kernel system call interfaces.

[mikhail.ramalho]

• make struct linux_dirent a private header
• rebased

[mikhail.ramalho]

ping

[sivachandra]

We can land this after my comments from this round are addressed.

libc/include/llvm-libc-types/ino_t.h

12 ↗

(On Diff #518727)

Is this required for this change? If not, remove it. If yes, can explain why?

Defining ino_t to be a typedef of __UINTPTR_TYPE__ might not be the best but it was done because the size of ino_t matches the pointer size on most platforms. If have ways to do this better, feel free to send a separate patch.

libc/include/llvm-libc-types/off64_t.h

12 ↗

(On Diff #515327)

Linux man pages for fopencookie explicitly specify off64_t so we should keep it.

libc/src/__support/File/linux_dir.cpp

39

Do we need this alternate path at all for the platforms we care about now? For example, if none of the builders exercise this path, then it is dead code. The four platforms I have checked and care for at this time, x86_64, aarch64, arm32 and riscv64, all have SYS_getdents64. If this alternate path is not an immediate necessity, we should skip it.

[mikhail.ramalho]

libc/src/__support/File/linux_dir.cpp

39

@sivachandra, SYS_getdents doesn't seem to be defined in riscv64, so we need to add the patch to support SYS_getdents64 instead. Please, check: https://github.com/riscv-collab/riscv-gnu-toolchain/blob/master/linux-headers/include/asm-generic/unistd.h

May I ask how you are testing this? I tested cross-compiling libc with clang, and building it natively in qemu, and SYS_getdents was not defined in either case.

[sivachandra]

libc/src/__support/File/linux_dir.cpp

39

I think you understood my comment in the other way - I was suggesting the removal of the SYS_getdents path and to keep only the new SYS_getdents64 path.

[mikhail.ramalho]

libc/src/__support/File/linux_dir.cpp

39

Oh, I see; riscv32 doesn't seem to have SYS_getdents defined either.

I would only argue to keep this for completeness' sake, but I can remove it if you prefer.

[sivachandra]

libc/src/__support/File/linux_dir.cpp

39

It would be dead code not tested anywhere. So, I would suggest to just remove it.

[mikhail.ramalho]

addressed comments

libc/config/linux/riscv64/entrypoints.txt

set(TARGET_LIBM_ENTRYPOINTS
     libc.src.math.truncl
 )
 
 if(LLVM_LIBC_FULL_BUILD)
   list(APPEND TARGET_LIBC_ENTRYPOINTS
     # assert.h entrypoints
     libc.src.assert.__assert_fail
 
+    # dirent.h entrypoints
+    libc.src.dirent.closedir
+    libc.src.dirent.dirfd
+    libc.src.dirent.opendir
+    libc.src.dirent.readdir
+
     # network.h entrypoints
     libc.src.network.htonl
     libc.src.network.htons
     libc.src.network.ntohl
     libc.src.network.ntohs
 
     # pthread.h entrypoints
     libc.src.pthread.pthread_atfork

libc/config/linux/riscv64/headers.txt

 set(TARGET_PUBLIC_HEADERS
     libc.include.assert
     libc.include.ctype
+    libc.include.dirent
     libc.include.errno
     libc.include.fcntl
     libc.include.fenv
     libc.include.inttypes
     libc.include.math
     libc.include.pthread
     libc.include.sched
     libc.include.signal

libc/include/llvm-libc-types/struct_dirent.h

 //===-- Definition of type struct dirent ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef __LLVM_LIBC_TYPES_STRUCT_DIRENT_H__
 #define __LLVM_LIBC_TYPES_STRUCT_DIRENT_H__
 
 #include <llvm-libc-types/ino_t.h>
 #include <llvm-libc-types/off_t.h>
 
 struct dirent {
   ino_t d_ino;
 #ifdef __unix__
   off_t d_off;
   unsigned short d_reclen;
 #endif
+  unsigned char d_type;
   // The user code should use strlen to determine actual the size of d_name.
   // Likewise, it is incorrect and prohibited by the POSIX standard to detemine
   // the size of struct dirent type using sizeof. The size should be got using
   // a different method, for example, from the d_reclen field on Linux.
   char d_name[1];
 };
 
 #endif // __LLVM_LIBC_TYPES_STRUCT_DIRENT_H__

libc/src/__support/File/CMakeLists.txt

 if(NOT (TARGET libc.src.__support.threads.mutex))
   # Not all platforms have a mutex implementation. If mutex is unvailable,
   # we just skip everything about files.
   return()
 endif()
 
 add_object_library(
   file
   SRCS
     file.cpp
   HDRS
     file.h
   DEPENDS
-    
     libc.src.__support.CPP.new
     libc.src.__support.CPP.span
     libc.src.__support.threads.mutex
     libc.src.__support.error_or
 )
 
 add_object_library(
   dir

 
 if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS}_file.cpp)
   add_object_library(
     platform_file
     SRCS
       ${LIBC_TARGET_OS}_file.cpp
     DEPENDS
       .file
-      
       libc.include.fcntl
       libc.include.stdio
       libc.include.sys_syscall
       libc.src.__support.CPP.new
       libc.src.__support.OSUtil.osutil
       libc.src.errno.errno
       libc.src.__support.error_or
   )

libc/src/__support/File/dir.cpp

 //===--- Implementation of a platform independent Dir data structure ------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "dir.h"
 
 #include "src/__support/CPP/new.h"
 #include "src/__support/error_or.h"
 #include "src/errno/libc_errno.h" // For error macros
 
-#include <stdlib.h>
-
 namespace __llvm_libc {
 
 ErrorOr<Dir *> Dir::open(const char *path) {
   auto fd = platform_opendir(path);
   if (!fd)
     return __llvm_libc::Error(fd.error());
 
   __llvm_libc::AllocChecker ac;
   Dir *dir = new (ac) Dir(fd);
if (readptr >= fillsize) {
     if (!readsize)
       return __llvm_libc::Error(readsize.error());
     fillsize = readsize;
     readptr = 0;
   }
   if (fillsize == 0)
     return nullptr;
 
   struct ::dirent *d = reinterpret_cast<struct ::dirent *>(buffer + readptr);
 #ifdef __unix__
   // The d_reclen field is available on Linux but not required by POSIX.
   readptr += d->d_reclen;
 #else
   // Other platforms have to implement how the read pointer is to be updated.
 #error "DIR read pointer update is missing."
 #endif
   return d;
 }
 
 int Dir::close() {
   {
     MutexLock lock(&mutex);
     int retval = platform_closedir(fd);
     if (retval != 0)
       return retval;
   }
   delete this;
   return 0;
 }
 
 } // namespace __llvm_libc

libc/src/__support/File/linux_dir.cpp

#endif
 
   if (fd < 0) {
     return __llvm_libc::Error(-fd);
   }
   return fd;
 }
 
 ErrorOr<size_t> platform_fetch_dirents(int fd, cpp::span<uint8_t> buffer) {
-  long size =
-      __llvm_libc::syscall_impl(SYS_getdents, fd, buffer.data(), buffer.size());
+#ifdef SYS_getdents64
+  long size = __llvm_libc::syscall_impl(SYS_getdents64, fd, buffer.data(),
+                                        buffer.size());
+#else
+#error "getdents64 syscalls not available to perform a fetch dirents operation."
+#endif
+
   if (size < 0) {
     return __llvm_libc::Error(static_cast<int>(-size));
   }
   return size;
 }
 
 int platform_closedir(int fd) {
   long ret = __llvm_libc::syscall_impl(SYS_close, fd);
   if (ret < 0) {
     return static_cast<int>(-ret);
   }
   return 0;
 }
 
 } // namespace __llvm_libc