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

[Evgeniy Stepanov via llvm-commits]

eugenis added inline comments.

================
Comment at: lib/cfi/cfi.cc:59
@@ +58,3 @@
+
+  if (cfi_check == (uptr)(-1) || cfi_check == 0) {
+    memset((void *)shadow_begin, cfi_check, shadow_end - shadow_begin);
----------------
pcc wrote:
> I think the code would be easier to read if this function were inlined into its caller, mainly because of the magic numbers you are using here.
Does it look better now?
I've split fill_shadow in 2: for function pointers, and for (0) and (-1) constants.
I've also refactored find_cfi_check_for_address to a class with, hopefully, cleaner interface.


================
Comment at: lib/cfi/cfi.cc:110
@@ +109,3 @@
+
+  for (const ElfW(Sym) *p = symtab; (ElfW(Addr))p < strtab; ++p) {
+    char *name = (char*)(strtab + p->st_name);
----------------
pcc wrote:
> This is reasonable if all major linkers place strtab immediately after symtab. Have you verified that this is the case?
Yes, looks that way.
Changed the code to bail out if symtab and strtab point to different ELF segments. Now, if they are inside one segment, nothing really bad can happen for over-scanning. The probability of misinterpreting a random sequence of bytes as __cfi_check symbol is super low.


================
Comment at: lib/cfi/cfi.cc:112
@@ +111,3 @@
+    char *name = (char*)(strtab + p->st_name);
+    if (strcmp(name, "CFI_Check") == 0) {
+      assert(p->st_info == ELF32_ST_INFO(STB_GLOBAL, STT_FUNC));
----------------
pcc wrote:
> We should probably not use this as the function name, as it intrudes on the user's namespace. Instead, please use a C/C++ implementation reserved identifier (e.g. `__CFI_Check`).
Renamed to __cfi_check.

================
Comment at: lib/cfi/cfi.cc:132
@@ +131,3 @@
+      // Need to fill shadow for both.
+      // FIXME: reject writable? There is no r/o segment, just rx and rw. Why?
+      uptr cur_beg = info->dlpi_addr + phdr->p_vaddr;
----------------
pcc wrote:
> I believe that this is controlled by the RELRO program header.
keep as FIXME?

================
Comment at: lib/cfi/cfi.cc:158
@@ +157,3 @@
+  }
+  if (cfi_check == 0xFFFFU) {
+    VReport(2, "CFI: unchecked call (shadow=FFFF): %p\n", Ptr);
----------------
pcc wrote:
> It's a little confusing that this function can return both function addresses and magic numbers. I would inline `find_cfi_check_for_address` into here.
PTAL

================
Comment at: lib/cfi/cfi.cc:175
@@ +174,3 @@
+
+  FlagParser ubsan_parser;
+  __ubsan::RegisterUbsanFlags(&ubsan_parser, uf);
----------------
pcc wrote:
> Can we link against ubsan dependent on whether diagnostics are enabled? That would save us from having to link ubsan into production builds.
So, we build libclang_rt.cfi and libclangrt.cfi_diag (the latter includes ubsan)?
And teach the driver to include the latter when needed.
Sounds doable.


Repository:
  rL LLVM

http://reviews.llvm.org/D15358