This is an archive of the discontinued LLVM Phabricator instance.

[XRay][compiler-rt] Support patching/unpatching specific functions
ClosedPublic

Authored by dberris on May 1 2017, 1:15 AM.

Details

Summary

This change allows us to patch/unpatch specific functions using the
function ID. This is useful in cases where implementations might want to
do coverage-style, or more fine-grained control of which functions to
patch or un-patch at runtime.

Depends on D32693.

Event Timeline

dberris created this revision.May 1 2017, 1:15 AM
dberris updated this revision to Diff 97375.May 1 2017, 6:05 PM

Remember to use the mutex/atomic that guards the patching from clobbering each other.

dblaikie accepted this revision.May 3 2017, 11:41 AM

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

lib/xray/xray_interface.cc
251

'unpatch' or should this be 'patch' (since the function is called 'patchSpecificFunction')?

264–266

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)

268–269

Does the GCC implementation support partial patching failures/success/etc?

test/xray/TestCases/Linux/coverage-sample.cc
56–58

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?

This revision is now accepted and ready to land.May 3 2017, 11:41 AM
dberris updated this revision to Diff 97770.May 3 2017, 9:41 PM
dberris marked 2 inline comments as done.
  • fixup: address comments, make better tests/semantics
  • fixup: in 32-bit platforms, we need to pad the struct to be 16-bytes
dberris added inline comments.May 3 2017, 9:42 PM
lib/xray/xray_interface.cc
264–266

It seems like we could just use a pointer directly, good point. Also yes, made it a pointer to a const XRaySledEntry.

268–269

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.

test/xray/TestCases/Linux/coverage-sample.cc
56–58

Good idea -- added a third case where we explicitly un-patch function id 1.

This revision was automatically updated to reflect the committed changes.