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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

In https://reviews.llvm.org/D41153#954307, @kpw wrote:

> 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.


Yeah, I'd like to think about that a little more. I suspect the call to mprotect is an implementation detail of patching/unpatching.

> 2. Should xray make an effort to restore the mprotect status of pages after it's done? Probably not worth the complexity.

I think we do that already, no? `MProtectHelper` already does that in the destructor, to make the pages read+execute only as opposed to read+write+execute.

> 3. If these regions never lose write permissions, why even call mprotect for unpatching?

They do, `MProtectHelper` does RAII on the mprotect status of pages. It does make the assumption that the pages being mprotected are r+x to begin with.



================
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.")
 
----------------
kpw wrote:
> 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).
Yeah, this file could only really be included from `xray_flags.h` and `xray_flags.cc` which should include the types. Fixed it from that side instead.

Unfortunately the flag parser doesn't support an overload for `size_t` reliably, and of the options we have there, `uptr` seems to fit the best.


================
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,
----------------
kpw wrote:
> What evidence or reasoning leads us to conclude that this assumption holds (ignoring DSO)?
This is the convention used by ELF and thus the Linux kernel to map the `.text` segment as contiguous virtual memory. If this is somehow not true, then our calls to `mprotect` will fail (i.e. if `.text` was somehow further segmented or non-contiguous). Is there a way to somehow improve this comment to make that clearer?


================
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
----------------
kpw wrote:
> Usage suggests defining the flag as size_t.
Unfortunately the flag parser may not know how to parse `size_t` but does know how to parse `uptr`.


================
Comment at: compiler-rt/lib/xray/xray_interface.cc:320
 
+static XRayPatchingStatus __xray_patch_helper(int32_t FuncId, bool Enable) {
+  XRaySledMap InstrMap;
----------------
kpw wrote:
> This function name is very non-descriptive.
> 
> __mprotect_and_patch_function()?
Good point. I've moved this into the `__xray` namespace, put it in an anonymous namespace in there, and used a more descriptive name `mprotectAndPatchFunction`.


https://reviews.llvm.org/D41153