[PATCH] D15358: CFI runtime library (cross-DSO support)

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/cfi/cfi.cc:41
@@ +40,3 @@
+
+static uptr mem_to_shadow(uptr x) {
+  return __cfi_shadow + ((x >> kShadowGranularity) << 1);
----------------
Looks like most callers of this function would be better off with a `uint16_t *` so maybe should return that?

================
Comment at: lib/cfi/cfi.cc:57
@@ +56,3 @@
+
+  CFICheckFn get_cfi_check() const {
+    uptr aligned_addr = addr & ~(kShadowAlign - 1);
----------------
`assert(!is_invalid() && !is_unchecked());` ? 

================
Comment at: lib/cfi/cfi.cc:60
@@ +59,3 @@
+    uptr p = aligned_addr - (((uptr)v - 1) << kShadowGranularity);
+    return reinterpret_cast<CFICheckFn>(p);
+  }
----------------
Yes, this looks nicer.

================
Comment at: lib/cfi/cfi.cc:71-72
@@ +70,4 @@
+
+  begin = begin & ~(kShadowAlign - 1);
+  end = (end + kShadowAlign - 1) & ~(kShadowAlign - 1);
+
----------------
Is this necessary? Looks like you could just pass these directly to `mem_to_shadow`.

================
Comment at: lib/cfi/cfi.cc:75
@@ +74,3 @@
+  uptr shadow_begin = mem_to_shadow(begin);
+  uptr shadow_end = mem_to_shadow(end);
+  assert((shadow_begin & 1) == 0);
----------------
This would become `mem_to_shadow(end - 1) + 1`.

================
Comment at: lib/cfi/cfi.cc:83-84
@@ +82,4 @@
+static void fill_shadow(uptr begin, uptr end, uptr cfi_check) {
+  begin = begin & ~(kShadowAlign - 1);
+  end = (end + kShadowAlign - 1) & ~(kShadowAlign - 1);
+
----------------
Same as above.

================
Comment at: lib/cfi/cfi.cc:133
@@ +132,3 @@
+  // Verify that strtab and symtab are inside of the same LOAD segment.
+  // This excludes VDSO, which has (very high) bogus strtab and symtab pointers.
+  int phdr_idx;
----------------
Yes.

================
Comment at: lib/cfi/cfi.cc:159
@@ +158,3 @@
+    }
+  }
+  return 0;
----------------
Looks good.

================
Comment at: lib/cfi/cfi.cc:176
@@ +175,3 @@
+      // PT_RELRO?
+      uptr cur_beg = info->dlpi_addr + phdr->p_vaddr;
+      uptr cur_end = cur_beg + phdr->p_memsz;
----------------
Maybe.

Why don't we build a libclang_rt.sanitizer_common library and have the sanitizers use that instead of each having its own copy of sanitizer_common? Then we could just have a cfi library and link the regular ubsan library conditionally.

================
Comment at: lib/cfi/cfi.cc:199
@@ +198,3 @@
+  ShadowValue sv = ShadowValue::load(Addr);
+  // uptr cfi_check = find_cfi_check_for_address(Addr);
+  if (sv.is_invalid()) {
----------------
Remove commented line.

================
Comment at: lib/cfi/cfi.cc:210
@@ +209,3 @@
+  VReport(2, "__cfi_check at %p\n", cfi_check);
+  ((CFICheckFn)cfi_check)(CallSiteTypeId, Ptr);
+}
----------------
You don't need this cast.


Repository:
  rL LLVM

http://reviews.llvm.org/D15358