Page MenuHomePhabricator

[Lexer] Don't merge macro args from different macro files
ClosedPublic

Authored by vsk on May 18 2016, 4:41 PM.

Details

Summary

The lexer sets the end location of macro arguments incorrectly *if*,
while merging consecutive args to fit into a single SLocEntry, it finds
args which come from different macro files.

Fix the issue by using separate SLocEntries in this situation.

This fixes a code coverage crasher (rdar://problem/26181005). Because
the lexer reported end locations for certain macro args incorrectly, we
would generate bogus coverage mappings with negative line offsets.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk updated this revision to Diff 57710.May 18 2016, 4:41 PM
vsk retitled this revision from to [Lexer] Don't merge macro args from different macro files.
vsk updated this object.
vsk added reviewers: akyrtzi, doug.gregor.
vsk added a subscriber: cfe-commits.
vsk updated this revision to Diff 57711.May 18 2016, 4:47 PM
  • Add some comments to the unit test.
vsk updated this revision to Diff 57810.May 19 2016, 9:17 AM
  • Fix explanation of the test case in test/CoverageMapping.
vsk added a comment.May 19 2016, 1:47 PM

I discussed this bug with Argyrios off-list, who lgtm'd on the condition that it doesn't introduce a performance regression. He suggested preprocessing Cocoa.h to stress the patch. After running a stabilization script, I used this command to stress RelNoAsserts builds of clang both with and without this patch.

for I in $(seq 1 100); do time $CC -F /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks -E Cocoa.h -o /dev/null; done

The results are basically in the noise (link to raw data: https://ghostbin.com/paste/r6cyh):

CompilerUnpatched TOTPatched TOT
Avg. wall time (s)0.217090.21608
Std. deviation0.021010.02219

I also made sure that the preprocessed sources emitted by the two compilers are the same.

This revision was automatically updated to reflect the committed changes.

I know this was sped up slightly in 3339c568c43e4644f02289e5edfc78c860f19c9f, but this change makes updateConsecutiveMacroArgTokens the hottest function in clang in a bottom up profile of an entire build of the Linux kernel. It thrashes the one entry LastFileIDLookup cache, and we end up looking up the same FileID again and again and again for each token when we expand nested function like macros.

Is there anything we can do to speed this up? Is there any way to record which FileID corresponds to a given Token so that we don't have to keep rematerializing that? Is it possible to find whether two SourceLocations correspond to the same FileID without having to figure out which FileID in particular each belongs to?

I discussed this bug with Argyrios off-list, who lgtm'd on the condition that it doesn't introduce a performance regression.

Well, I'd say it's a performance regression, though perhaps reported 5 years too late.

vsk added a comment.May 20 2021, 10:19 AM

I know this was sped up slightly in 3339c568c43e4644f02289e5edfc78c860f19c9f, but this change makes updateConsecutiveMacroArgTokens the hottest function in clang in a bottom up profile of an entire build of the Linux kernel. It thrashes the one entry LastFileIDLookup cache, and we end up looking up the same FileID again and again and again for each token when we expand nested function like macros.

Is there anything we can do to speed this up? Is there any way to record which FileID corresponds to a given Token so that we don't have to keep rematerializing that? Is it possible to find whether two SourceLocations correspond to the same FileID without having to figure out which FileID in particular each belongs to?

Perhaps you could try:

  • using SourceManager::isInFileID(NextLoc, getFileID(CurLoc), ...) (to halve the number of necessary getFileID lookups), or
  • using a 2-element cache in getFileID?

I discussed this bug with Argyrios off-list, who lgtm'd on the condition that it doesn't introduce a performance regression.

Well, I'd say it's a performance regression, though perhaps reported 5 years too late.

If the performance issue manifests on Linux kernel sources from 5 years ago, then sure, I'd agree :).