[PATCH] D32695: [XRay][compiler-rt] Support patching/unpatching specific functions

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good - maybe the test case could be improved. Provided some ideas.



================
Comment at: lib/xray/xray_interface.cc:251
+
+  // If we don't have an index, we can't unpatch individual functions.
+  if (InstrMap.Functions == 0)
----------------
'unpatch' or should this be 'patch' (since the function is called 'patchSpecificFunction')?


================
Comment at: lib/xray/xray_interface.cc:264-266
+  auto SledRange = InstrMap.SledsIndex[FuncId - 1];
+  auto *f = reinterpret_cast<XRaySledEntry *>(SledRange.Begin);
+  auto *e = reinterpret_cast<XRaySledEntry *>(SledRange.End);
----------------
any chance the InstrMap data structure could be typed in this way to begin with, to avoid the need for casts here? (also - should const be added here? I guess maybe not, since these pointers point to the sleds which will be modified)


================
Comment at: lib/xray/xray_interface.cc:268-269
+
+  // TODO: Figure out how we can undo partial patching, or signal partial
+  // success.
+  while (f != e) {
----------------
Does the GCC implementation support partial patching failures/success/etc?


================
Comment at: test/xray/TestCases/Linux/coverage-sample.cc:55-57
+  // patch the functions we've called before.
+  for (const auto id : called_fns)
+    __xray_patch_function(id);
----------------
I'd consider changing the test to patch a specific function - as it stands it patches them all, which doesn't look all that different to calling the __xray_patch() function.

Also - what's the purpose of unpatching each function as its called in the handler? Would the handler behave differently/fail the test, etc, if the __xray_unpatch_function wasn't called there.

Maybe start with them all patched so you can record the function IDs, then unpatch one, test that it wasn't visited. Unpatch all+repatch one, test that only that one was visited?


https://reviews.llvm.org/D32695