[PATCH] D15728: [cfi] Support for dlopen and dlclose

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/cfi/cfi.cc:35
@@ -33,2 +34,3 @@
 
-static uptr __cfi_shadow;
+static constexpr uptr kCfiShadowStorageSize = 4096; // 1 page
+// Lets hope that the data segment is mapped with 4K pages.
----------------
Maybe assert somewhere that this is equal to `GetPageSize()`?

================
Comment at: lib/cfi/cfi.cc:39
@@ +38,3 @@
+// The rest of the page is unused, re-mapped read-only and updated via mremap.
+static char __cfi_shadow_storage[kCfiShadowStorageSize]
+    __attribute__((aligned(kCfiShadowStorageSize)));
----------------
This name is unclear. It should reflect that the shadow pointer is stored here, not the shadow itself.

================
Comment at: lib/cfi/cfi.cc:54
@@ +53,3 @@
+// Set the start address of the CFI shadow region.
+void SetShadow(uptr shadow) {
+  CHECK_EQ(0, GetShadow());
----------------
Please inline into `ShadowBuilder::Install`, as the pre-conditions of this function aren't clear without context.

================
Comment at: lib/cfi/cfi.cc:140
@@ +139,3 @@
+  if (main_shadow) {
+    void *res = mremap((void *)shadow_, __cfi_shadow_size, __cfi_shadow_size,
+                       MREMAP_MAYMOVE | MREMAP_FIXED, main_shadow);
----------------
Do you need to set the memory protection for `shadow_` first?

================
Comment at: lib/cfi/cfi.cc:299
@@ -197,3 +298,3 @@
     VReport(2, "CFI: invalid memory region for a function pointer (shadow==0): %p\n", Ptr);
-    Die();
+    __builtin_trap(); // FIXME: use Trap().
   }
----------------
You can use it now I guess.

================
Comment at: lib/sanitizer_common/sanitizer_posix.cc:178
@@ +177,3 @@
+
+bool MprotectReadWrite(uptr addr, uptr size) {
+  return 0 == internal_mprotect((void *)addr, size, PROT_READ | PROT_WRITE);
----------------
This function is unused.


Repository:
  rL LLVM

http://reviews.llvm.org/D15728