This is an archive of the discontinued LLVM Phabricator instance.

Extend SpecialCaseList to allow users to blame matches on entries in the file.
ClosedPublic

Authored by hctim on Oct 31 2017, 5:35 PM.

Details

Summary

Extends SCL functionality to allow users to find the line number in the file the SCL is built from through SpecialCaseList::inSectionBlame(...).

Also removes the need to compile the SCL before use. As the matcher now contains a list of regexes to test against instead of a single regex, the regexes can be individually built on each insertion rather than one large compilation at the end of construction.

This change also fixes a bug where blank lines would cause the parser to become out-of-sync with the line number. An error on line k was being reported as being on line k - num_blank_lines_before_k.

Note: This change has a cyclical dependency on D39486. Both these changes must be submitted at the same time to avoid a build breakage.

Event Timeline

hctim created this revision.Oct 31 2017, 5:35 PM
hctim edited reviewers, added: vlad.tsyrklevich; removed: kcc.Oct 31 2017, 6:02 PM
hctim edited subscribers, added: kcc; removed: vlad.tsyrklevich.
pcc added a comment.Oct 31 2017, 6:13 PM

I'd be curious about what sort of perf impact (positive or negative) comes from splitting the special case list into multiple regexes, as this part of the compiler has historically been perf sensitive. It may be that the trigram filter (which is relatively new) makes the impact relatively insignificant.

Perhaps a good way to measure would be to compile a relatively large Chromium TU with/without this patch.

I share the same concern as Peter, the TrigramIndex might save us from the performance hit but we are potentially blowing up the amount of regex comparisons we have to perform. With the very wide new cfi-icall blacklist in Chrome we will pass the TrigramIndex for significant parts of Chrome and all those entries are at the bottom of the list so we have to iteratively go through the entire blacklist before hitting those entries. I'd be curious to measure this for a Chrome build with use_cfi_icall=true specifically.

lib/Support/SpecialCaseList.cpp
139

Doesn't the loop already handle this?

hctim updated this revision to Diff 121173.Nov 1 2017, 1:18 PM
hctim marked an inline comment as done.

Fixed line parsing to not ignore blank lines, line number is correctly generated.

lib/Support/SpecialCaseList.cpp
139

Too right. There is still a latent bug however - llvm::SplitString culls blank lines from the output vector, so it was generating the wrong line numbers for anything following a blank line.

I've fixed this up and made sure that a unit test is there that catches both these cases.

hctim updated this revision to Diff 121174.Nov 1 2017, 1:22 PM

Accidently made last modifications on an old patch, merged the changes across onto the most recent patch and fixed up the regressions.

vlad.tsyrklevich accepted this revision.Nov 7 2017, 12:55 PM
vlad.tsyrklevich added inline comments.
include/llvm/Support/SpecialCaseList.h
114–115

This is no longer true.

115–116

s/Set/Map/

This revision is now accepted and ready to land.Nov 7 2017, 12:55 PM
hctim updated this revision to Diff 121967.Nov 7 2017, 1:13 PM
hctim marked 2 inline comments as done.

Updated with Vlad's comments and rebased over master in preparation for submission.