Page MenuHomePhabricator

[xray][clang] Always add xray-skip-entry/exit and xray-ignore-loops attrs
ClosedPublic

Authored by ianlevesque on Feb 1 2020, 2:56 PM.

Details

Summary

The function attributes xray-skip-entry, xray-skip-exit, and
xray-ignore-loops were only being applied if a function had an
xray-instrument attribute, but they should apply if xray is enabled
globally too.

Diff Detail

Event Timeline

ianlevesque created this revision.Feb 1 2020, 2:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2020, 2:56 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dberris accepted this revision.Feb 1 2020, 6:11 PM
This revision is now accepted and ready to land.Feb 1 2020, 6:11 PM
MaskRay added a subscriber: MaskRay.Feb 1 2020, 7:20 PM
MaskRay added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1102

Args.hasArg(OPT_fxray_ignore_loops);

There is no need to check a fno option in cc1, if the option is always enabled/disabled by default.

ianlevesque marked 2 inline comments as done.Feb 3 2020, 11:26 AM
ianlevesque added inline comments.
clang/lib/CodeGen/CodeGenFunction.cpp
832

@hiraditya I realize on a previous diff you suggested dropping the brackets. I'll do that here too.

clang/lib/Frontend/CompilerInvocation.cpp
1102

Will update.

Address code review nits

MaskRay accepted this revision.Feb 5 2020, 7:13 PM

I can commit this for you, but is it possible to write a test case?

Now with 100% more tests.

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

clang/test/CodeGen/xray-attributes-skip-entry-exit.cpp
9

I don't see the NOCUSTOM or NOTYPED prefixes defined?

13

Same with the FUNCTION prefix.

clang/test/CodeGen/xray-ignore-loops.cpp
2

From what I've seen, CodeGen tests don't usually invoke the driver directly. They have a test that the driver passes the correct flags to cc1 (as you do below), and then they just test that cc1 does the right thing with that flag.

ianlevesque marked 4 inline comments as done.Feb 11 2020, 1:28 PM

Address code review feedback on the tests.

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

The existing tests from when the feature was added covered that case well. It was the driver passing the flag and the case without the xray-instrument attribute that weren't working.

smeenai accepted this revision.Feb 11 2020, 1:53 PM

LGTM. I'll commit this for you.

This revision was automatically updated to reflect the committed changes.