Page MenuHomePhabricator

[xray] Option to omit the function index
ClosedPublic

Authored by ianlevesque on Jun 17 2020, 12:18 AM.

Details

Summary

Add a flag to omit the xray_fn_idx to cut size overhead and relocations
roughly in half at the cost of reduced performance for single function
patching. Minor additions to compiler-rt support per-function patching
without the index.

Diff Detail

Event Timeline

ianlevesque created this revision.Jun 17 2020, 12:18 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJun 17 2020, 12:18 AM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits and 2 others. · View Herald Transcript
dberris accepted this revision.Jun 17 2020, 12:46 AM

LGTM

Thanks!

This revision is now accepted and ready to land.Jun 17 2020, 12:46 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Jun 17 2020, 12:40 PM
clang/include/clang/Basic/CodeGenOptions.def
114

Nit: a variable name with a positive meaning may be easier to understand.

clang/test/Driver/XRay/xray-function-index-flags.cpp
6

If c++11 is not relevant, drop it.

20

I know some tests may be inconsistent but the prevailing style is to place REQUIRES: at the top.

compiler-rt/lib/xray/xray_init.cpp
94

std:: is uncommon.

LLVM style is ++I

95

spell out the concrete type if non-obvious.

llvm/test/CodeGen/AArch64/xray-omit-function-index.ll
6

CHECK-COUNT

34

No newline at end of file

ianlevesque marked 3 inline comments as done.Jun 17 2020, 3:32 PM
ianlevesque added inline comments.
clang/include/clang/Basic/CodeGenOptions.def
114

I had the entire patch that way at first, making the index something you would enable (defaulting to true) but it was much worse.

clang/test/Driver/XRay/xray-function-index-flags.cpp
20

I already committed this, but I can do a quick follow up for these nits.

MaskRay added inline comments.Jun 17 2020, 5:54 PM
clang/include/clang/Basic/CodeGenOptions.def
114

The variable naming should be easily inferrable from the option name. The option does not say omit. Doesn't XRayFunctionIndex work? How can it be much worse?

clang/include/clang/Driver/Options.td
1284

This should use OptOutFFlag. I fixed it.

clang/test/Driver/XRay/xray-function-index-flags.cpp
6

Already dropped and cleaned a bit.

20

Fixed.

ianlevesque marked 4 inline comments as done.Jun 17 2020, 7:08 PM
ianlevesque added inline comments.
clang/include/clang/Basic/CodeGenOptions.def
114

Sure just changing the variable name is fine of course. I meant it was worse (many tests needing to be changed) when I tried to make the index opt-in not opt-out and just have the Driver set it by default.

clang/include/clang/Driver/Options.td
1284

Thanks for the fixes!

MaskRay added inline comments.Jun 17 2020, 7:37 PM
clang/include/clang/Basic/CodeGenOptions.def
114

I mean you can use a variable named XRayFunctionIndex which defaults to true. This is similar to an XRayOmitFunctionIndex which defaults to false.

ianlevesque marked 2 inline comments as done.Jun 17 2020, 8:56 PM
ianlevesque added inline comments.
clang/include/clang/Basic/CodeGenOptions.def
114

Yep, saw your change - works for me. Thanks!