Good catch, thanks for the fix.

Makes sense to me

lgtm

Apply the fix in XRayInstrumentation, add test cases.

Add static function to the test case, update documentation.

I can update the docs @MaskRay, not a problem. I'll tweak the test a little too per your comment.

Thanks @MaskRay - I tried to answer that question in https://reviews.llvm.org/D87953#2286430. At present we are deploying instrumentation to an arbitrary subset of our application using the instruction threshold. I would like to make the selection of how many and which functions more deterministic, and be able to instrument different subsets over time. The overhead we are concerned with is purely binary size as we are deploying to Android devices. We are using features from my previous XRay patches to omit the function index already, but the sheer number of sleds and size of the associated xray_instr_map are the limiting factor of how much we can instrument in any given app release. For our use case it is fine to gradually over a period of weeks work our way across the entire app group by group.

Remove extraneous parameter validations.

This is ready for another review, I think I addressed everything.

const

In D87953#2284071, @MaskRay wrote:

How large the overhead is? This is somewhat surprising to me.

Address code review feedback

Ugh, cut a bit too deep when removing formatting. Thanks for fixing forward on the test @MaskRay

Can I get a re-review on this?

@MaskRay I think this should cover it now. I cut away all the sections I could from the test and eliminated the unused branch and all the ELFObject modifications.

Remove unnecessary fallback branch.

Remove even more sections from the test object. Special case ARM entirely instead of extending the ELFObject classes.

I'll keep looking at this but @MaskRay if you could point me to something in LLVM that checks whether an addend is present that I can actually access from this module that would be helpful. I added getOptionalAddend as a last resort :(

Simplify and combine tests per @MaskRay

@MaskRay for what it's worth i'm currently trying to make xray_fn_idx optional - we tend to patch an entire DSO at a time so we don't need it to find individual sleds (I think we could support the entire compiler-rt interface, just slower, without it). Perhaps if someone cares about avoiding the relocations in xray_fn_idx they could just use the option to omit it as well.

Is "llvm-xray extract" still able to symbolize these correctly? I don't know offhand what test coverage is there for that. The change looks great to me otherwise.

In D73842#1869890, @smeenai wrote:

Is it worth adding a test that a function with an explicit xray-instrument attribute also has these other attributes applied?

Address code review feedback on the tests.

Now with 100% more tests.

@hiraditya or @smeenai can i get a merge?

Address code review nits

Add x86 tests

fix clang-format

In D72659#1818816, @dberris wrote:

I'm wondering how you're imbuing these attributes to the functions from the front-ends -- is this something you intend to support as a flag in Clang for example?

Remove extraneous changes

@smeenai or @hiraditya we could use this too, maybe one of you can merge?

I don't have commit access, can someone land it?

rename Supports