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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
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);
----------------
dblaikie wrote:
> 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)
It seems like we could just use a pointer directly, good point. Also yes, made it a pointer to a `const XRaySledEntry`.


================
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) {
----------------
dblaikie wrote:
> Does the GCC implementation support partial patching failures/success/etc?
Actually now that I think about it, we only report errors and keep going anyway (this happens when we encounter a sled we don't know how to patch). It might happen for example when we add a new sled type and some object files have this new sled type but they're linked against an old version of the XRay library.

Updated it now to only fail if we didn't get to patch any sled for the function. Also added some diagnostics for debug-ability.


================
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);
----------------
dblaikie wrote:
> 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?
Good idea -- added a third case where we explicitly un-patch function id 1.


https://reviews.llvm.org/D32695