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

[Alex Brachet via Phabricator via libc-commits]

abrachet 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>
   }];
 }
 
----------------
Why are all these in a linux specific directory?


================
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">
----------------
Is there no linux header for these?


================
Comment at: libc/config/linux/platfrom_defs.h.inc:15
+
+#define PAGE_SIZE 4096
----------------
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.


================
Comment at: libc/config/linux/x86_64/syscall_test.cpp:1
+//===---------------------- Unittests for errno --------------------------===//
+//
----------------
Change the name.


================
Comment at: libc/src/sys/mman/mmap_test.cpp:19
+TEST(MMapTest, NoError) {
+  size_t alloc_size = 128;
+  llvmlibc_errno = 0;
----------------
Make this pagesize, if it calls mmap2, then this will be an error because it's not a multiple of pagesize.


================
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;
+
----------------
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.


================
Comment at: libc/src/sys/mman/mmap_test.cpp:41
+
+TEST(MMapTest, Error_InvalidSize) {
+  llvmlibc_errno = 0;
----------------
Maybe also a test for invalid `munmap`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71634