[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/src/__support/File/linux_dir.cpp:45
+                                        buffer.size());
+#elif defined(SYS_getdents)
+  size_t size =
----------------
mikhail.ramalho wrote:
> sivachandra wrote:
> > mikhail.ramalho wrote:
> > > sivachandra wrote:
> > > > 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.
> > > @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.
> > 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.
> 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.
It would be dead code not tested anywhere. So, I would suggest to just remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147738