Page MenuHomePhabricator

Support for instrumenting only selected files or functions
ClosedPublic

Authored by phosek on Jan 15 2021, 12:53 PM.

Details

Summary

This change implements support for applying profile instrumentation
only to selected files or functions. The implementation uses the
sanitizer special case list format to select which files and functions
to instrument, and relies on the new noprofile IR attribute to exclude
functions from instrumentation.

Diff Detail

Event Timeline

phosek created this revision.Jan 15 2021, 12:53 PM
phosek requested review of this revision.Jan 15 2021, 12:53 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 15 2021, 12:53 PM
shayba added a subscriber: shayba.Jan 15 2021, 1:42 PM
davidxl added inline comments.Jan 19 2021, 1:22 PM
clang/docs/SourceBasedCodeCoverage.rst
77 ↗(On Diff #317052)

perhaps documenting how compiler generated functions are handled?

86 ↗(On Diff #317052)

what is the default section name?

clang/lib/CodeGen/CodeGenModule.cpp
2571

Should the option to turn on instrumentation also be checked?

2593

If the profile list contains only one line of exclude list, it seems that all functions will be rejected as the function returns 'false' by default for all other functions?

clang/lib/CodeGen/CodeGenModule.h
1280

Document the method and params.

phosek updated this revision to Diff 317796.Jan 20 2021, 12:44 AM
phosek marked 3 inline comments as done.
phosek added inline comments.Jan 20 2021, 12:44 AM
clang/docs/SourceBasedCodeCoverage.rst
86 ↗(On Diff #317052)

It's * which always matches.

clang/lib/CodeGen/CodeGenModule.cpp
2571

Currently, this method is only invoked from CodeGenFunction which does its own check, but I'm happy to include a check here in case there are ever other callers.

2593

That's correct, would you expect a different behavior and if so what should that behavior be?

Currently, the profile list is used to build up a list of functions and files that should be instrumented. Any function or file that's not covered by the profile list is automatically excluded (not included may be more correct). Having just excludes without any includes means that nothing will be instrumented.

davidxl added inline comments.Jan 20 2021, 5:25 PM
clang/lib/CodeGen/CodeGenModule.cpp
2593

It is natural and more useful to let user simply specify exclude lists so they don't need to worry about patterns of other functions to be instrumented.

This means that when only exclude list is specified, the default should be flipped to be true (instrument everything else).

If there only include list, the default is false.

If there are both, the include list should be used to specify a super set while the exclude list (taking precedence) used to specify a subset from the super set.

phosek updated this revision to Diff 318132.Jan 21 2021, 1:37 AM
phosek marked an inline comment as not done.
phosek added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
2593

That makes sense, I've implemented that in the latest patch.

vsk added inline comments.Jan 21 2021, 9:23 AM
clang/lib/CodeGen/CodeGenFunction.cpp
851

If we add an instrumentation kind, it may be more convenient to trigger a -Wcovered-switch warning (by removing this default case) vs. seeing a crash at runtime.

phosek updated this revision to Diff 318257.Jan 21 2021, 11:02 AM
phosek marked an inline comment as done.
phosek added inline comments.
clang/lib/CodeGen/CodeGenFunction.cpp
851

Done, I've also decided to move the helper function inside the ProfileList class to hide the detail of how ProfileInstrKind translates to section name.

phosek marked an inline comment as done.Jan 25 2021, 10:31 AM

@davidxl @vsk do you have any further comments?

davidxl added inline comments.Jan 25 2021, 12:21 PM
clang/test/CodeGen/profile-filter.c
18

Can you add a test case with both include and exclude list (the intersection of INCLUDE_SET & !EXCLUDE_SET)?

phosek added inline comments.Jan 25 2021, 1:58 PM
clang/test/CodeGen/profile-filter.c
18

See the case above this one which has:

fun:test*
!fun:test1

Is that what you have in mind?

davidxl added inline comments.Jan 25 2021, 2:00 PM
clang/test/CodeGen/profile-filter.c
18

Right

phosek added inline comments.Jan 25 2021, 2:41 PM
clang/test/CodeGen/profile-filter.c
18

Do we need to do anything then since this test case already exists?

davidxl accepted this revision.Jan 25 2021, 2:55 PM

my bad -- I missed the test case. LGTM

This revision is now accepted and ready to land.Jan 25 2021, 2:55 PM
This revision was automatically updated to reflect the committed changes.

Looks like this breaks tests on windows: http://45.33.8.238/win/32075/step_7.txt

Please take a look, and revert for now if it takes a while to fix.

Looks like this breaks tests on windows: http://45.33.8.238/win/32075/step_7.txt

Please take a look, and revert for now if it takes a while to fix.

Thanks for the heads up, I suspect it might be because of the Windows paths in the list file. I've reverted the change so I can test the fix on Windows before relanding.

phosek reopened this revision.Jan 26 2021, 12:27 PM
This revision is now accepted and ready to land.Jan 26 2021, 12:27 PM
MaskRay added inline comments.
clang/docs/UsersManual.rst
2271

This can be added below -fprofile-exclude-files=

phosek added inline comments.Jan 26 2021, 2:25 PM
clang/docs/UsersManual.rst
2271

I considered it but that's in the GCOV section and this flag only applies to the LLVM instrumentation.

This revision was automatically updated to reflect the committed changes.