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

Thu Dec 19 14:02:18 PST 2019

sivachandra added inline comments.

================
Comment at: libc/config/linux/api.td:7-47
 def SizeT : TypeDecl<"size_t"> {
   let Decl = [{
     #define __need_size_t
     #include <stddef.h>
   }];
 }

----------------
abrachet wrote:
> Why are all these in a linux specific directory?
Right. They can live in a common config file, but they are here as we only have one config right now. When we add a new config, we can move them to a common location as suitable.

================
Comment at: libc/config/linux/api.td:119-121
+    // TODO: The value of 0x20 is good for x86_64, but has to be extended
+    // in some manner to accommodate other machine architectures.
+    SimpleMacroDef<"MAP_ANONYMOUS", "0x20">
----------------
abrachet wrote:
> Is there no linux header for these?
Surprisingly, no. AFAICT, the arch specific values are not exposed via any public linux header.  Both glibc and musl also define these macros in their tree.

================
Comment at: libc/config/linux/platfrom_defs.h.inc:15
+
+#define PAGE_SIZE 4096
----------------
MaskRay wrote:
> 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).
Let me start by saying that it was my mistake to not have added a long comment explaining why I added `PAGE_SIZE` here.

So, page size should ideally be got from the aux vector[`*`]. We do not have that yet so I had to rely on some other way to obtain the page size. Looking around, what I found was glibc's use of `EXEC_PAGESIZE` from `linux/param.h`. But, glibc is actually mixed about this: in some places it uses `EXEC_PAGESIZE`, and in some other places uses the value it read from the aux vector. There was a discussion around this long time ago on the glibc list: https://lists.gnu.org/archive/html/bug-hurd/2011-10/msg00035.html.

This hard coding is only an interim step (I have added a comment now), and as @MaskRay points out, for x86 it is actually correct. I plan to move the implementations of mmap and munmap to the `config/linux` directory (we cannot do it now as the CMake targets will also have to be moved under linux/config. I have a follow up change which helps us avoid it and then we can move.) After moving them, I plan to use `EXEC_PAGESIZE` from `linux/param.h`. Though it is not perfect, it will also be an interim step, but a better interim: `EXEC_PAGESIZE` works for most architectures, but would fail for those on which the page size is not fixed.

`*` Using page size from the aux vector brings in its own set of problems. It would require the libc to maintain global state with parts of the libc implementation using this state. This causes problems wrt mixing LLVM-libc with another libc. I have not put enough thought into it, so may be it can be overcome cleanly.

================
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
----------------
MaskRay wrote:
> 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.
In a way, I have paralleled what musl does. But, I think you are right so I have now put everything under #ifdef, which is what glibc does.

================
Comment at: libc/src/sys/mman/mmap_test.cpp:19
+TEST(MMapTest, NoError) {
+  size_t alloc_size = 128;
+  llvmlibc_errno = 0;
----------------
abrachet wrote:
> Make this pagesize, if it calls mmap2, then this will be an error because it's not a multiple of pagesize.
Hmm, I am not sure. AFAIU, mmap2 only changes the interpretation of the offset.

================
Comment at: libc/src/sys/mman/mmap_test.cpp:34
+  // PROT_READ: if you uncomment the line below, the test should crash.
+  // array[0] = 123;
+
----------------
abrachet wrote:
> You could do `ASSERT_DEATH(array[0] = 123, ::testing::KilledBySignal(SIGSEGV)` or something like that. I don't think we need to though, that's just testing the OS at that point. But if you think it is worth the comment it should be worth the test as well.
Removed now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71634