[PATCH] D41153: [XRay][compiler-rt] Coalesce calls to mprotect to reduce patching overhead

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Suggestion for future changes:

1. We could decouple mprotect and __xray_patch_function so that callers that want to mprotect once and then patch individual functions have their cake and eat it too.
2. Should xray make an effort to restore the mprotect status of pages after it's done? Probably not worth the complexity.
3. If these regions never lose write permissions, why even call mprotect for unpatching?



================
Comment at: compiler-rt/lib/xray/xray_flags.inc:22-23
 XRAY_FLAG(const char *, xray_mode, "", "Mode to install by default.")
-
+XRAY_FLAG(uptr, xray_page_size_override, 0,
+          "Override the default page size for the system, in bytes.")
 
----------------
It's a bit unfortunate that there is now an undocumented requirement that code #including this file must have already imported sanitizer_internal_defs.h.

Perhaps you could add an #ifdef check for SANITIZER_DEFS_H or the uptr typedef itself with an #error assertion.

Is uptr even the right type? Page size doesn't represent a pointer, it's more like a size_t (although we might have to use it as an offset and add it to an address).


================
Comment at: compiler-rt/lib/xray/xray_interface.cc:211-213
+  // and try to get as few calls to mprotect(...) as possible. We're assuming
+  // that all the sleds for the instrumentation map are contiguous as a single
+  // set of pages. When we do support dynamic shared object instrumentation,
----------------
What evidence or reasoning leads us to conclude that this assumption holds (ignoring DSO)?


================
Comment at: compiler-rt/lib/xray/xray_interface.cc:221
+  for (std::size_t I = 0; I < InstrMap.Entries; I++) {
+    auto &Sled = InstrMap.Sleds[I];
+    if (Sled.Address < MinSled.Address)
----------------
const auto &Sled?


================
Comment at: compiler-rt/lib/xray/xray_interface.cc:228
+
+  const size_t PageSize = flags()->xray_page_size_override > 0
+                              ? flags()->xray_page_size_override
----------------
Usage suggests defining the flag as size_t.


================
Comment at: compiler-rt/lib/xray/xray_interface.cc:232
   if ((PageSize == 0) || ((PageSize & (PageSize - 1)) != 0)) {
     Report("System page size is not a power of two: %lld\n", PageSize);
     return XRayPatchingStatus::FAILED;
----------------
Consider listing this requirement in the flag description.


================
Comment at: compiler-rt/lib/xray/xray_interface.cc:320
 
+static XRayPatchingStatus __xray_patch_helper(int32_t FuncId, bool Enable) {
+  XRaySledMap InstrMap;
----------------
This function name is very non-descriptive.

__mprotect_and_patch_function()?


https://reviews.llvm.org/D41153