[libc-commits] [PATCH] D71634: Add implementations of POSIX mmap and munmap functions.

[Fangrui Song via Phabricator via libc-commits]

MaskRay added inline comments.


================
Comment at: libc/config/linux/platfrom_defs.h.inc:15
+
+#define PAGE_SIZE 4096
----------------
phosek wrote:
> abrachet wrote:
> > Shouldn't this be defined in some linux header that we can include? Either way I don't think we should just be defining it like this, as this is platform specific (although what platforms use a different page size) it should go in the config files I reckon.
> I'm not sure whether this should be defined at all, the man page for `getpagesize` says:
> 
>        Whether  getpagesize()  is  present as a Linux system call depends on the architecture.  If it is, it returns the kernel symbol PAGE_SIZE, whose value depends on the architecture and machine model.  Generally, one uses binaries that are dependent on the architecture but not on the machine model, in order to have a single binary distribution per architecture.  This means that a user program should not find PAGE_SIZE at compile time from a header file, but use an actual system call, at least for those architectures (like sun4) where this dependency exists.  Here glibc 2.0 fails because its getpagesize() returns a statically derived value, and does not use a system call.  Things are OK in glibc 2.1.
4096 is good for x86. Linux uapi does not define `PAGE_SIZE` as a constant on arm/aarch64/powerpc/etc.

If you are going to make `PAGE_SIZE` arch-specific, it probably makes more sense to use `PAGESIZE` (in POSIX base), and add `#define PAGE_SIZE PAGESIZE` (XSI extension).


================
Comment at: libc/config/linux/syscall_numbers.h.inc:110
+#define SYS_fstatfs __NR_fstatfs
+#define SYS_fstatfs64 __NR_fstatfs64
+#define SYS_fsync __NR_fsync
----------------
Some __NR_*64 macros do not exist on some architectures. In such cases, such SYS_*64 should not be defined as user programs may use `#ifdef __SYS_fstatfs64` and the definition here will break them.

It probably also makes sense to define `SYS_*` to `SYS_*64` if the latter is defined, if you don't want users of the new libc also fight with the Y2038 problem.


================
Comment at: libc/include/__posix-types.h:14
+#if defined(__need_off_t) && !defined(__llvm_libc_off_t_defined)
+typedef __PTRDIFF_TYPE__ off_t;
+#define __llvm_libc_off_t_defined
----------------
It probably makes more sense to define off_t as `long long` (64-bit) or `long` (32-bit) to not have to deal with LFS64.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71634