This is an archive of the discontinued LLVM Phabricator instance.

[Lex] Bring back the magic number 50 in updateConsecutiveMacroArgTokens.
ClosedPublic

Authored by hokein on Oct 22 2022, 1:46 PM.

Details

Summary

The magic number 50 was removed in D134942, as a behavior change for
performance reason.

While it reduces the number of SLocEntry, it increases the usage of
SourceLocation address space usage, which is critical for compiling
large TU.

This fixes a regression caused in D134942 -- clang failed to compile one of
our internal files, complaining the file is too large to process because clang
runs out of source location space (we spend 40% more address space!)

Diff Detail

Event Timeline

hokein created this revision.Oct 22 2022, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2022, 1:46 PM
hokein requested review of this revision.Oct 22 2022, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2022, 1:46 PM
alexfh accepted this revision.Oct 22 2022, 3:51 PM

LG, thanks!

This revision is now accepted and ready to land.Oct 22 2022, 3:51 PM

This is a subtle change that needs careful review, and honestly should have a test.

I realize there's a breakage that needs to be fixed with some urgency and you believe you're just restoring the old behavior, however in that case the right course of action is to revert the patch.
As it turns out I think this introduces a bug when offset space is nearly full.

clang/lib/Lex/TokenLexer.cpp
1017

distance -> Distance

1018

This seems to be missing the same-sloc-address-space check: it can create a single file ID spanning a local and loaded sloc entry, which will be corrupted by saving+loading

1018

(getOffset() would be much clearly correct than getRawEncoding())

1024

having worked quite hard to make this code readable, copy/pasting the same (buggy) algorithm in both places seems like an unfortunate choice.

I'd suggest:

auto NearLast = [&, Last{BeginLoc}](SourceLocation Loc) mutable {
  constexpr int Limit = 50;
  SourceLocation IntTy  RelOffs;
  if (!SM.isInSameSLocAddrSpace(Last, Loc, &RelOffs) || RelOffs < 0 || RelOffs > Limit)
    return false;
  Last = Loc;
  return true;
};

...
Partition = All.take_while([&]{
  return T.getLocation() >= BeginLoc && T.getLocation() < Limit && NearLast(T.getLocation());
});

Here NearLast can be shared between both branches.

This is a subtle change that needs careful review, and honestly should have a test.

Um, sorry for the grumpy tone. I thought this had already been quickly landed! Apologies...

This is a subtle change that needs careful review, and honestly should have a test.

I realize there's a breakage that needs to be fixed with some urgency and you believe you're just restoring the old behavior, however in that case the right course of action is to revert the patch.

You're right. Let's first revert the change that caused the problem and then figure out what amount of testing is necessary here.

alexfh requested changes to this revision.Oct 23 2022, 2:17 PM

Undo LGTM, since we decided to go with a revert.

This revision now requires changes to proceed.Oct 23 2022, 2:17 PM
hokein updated this revision to Diff 470457.Oct 25 2022, 6:10 AM
hokein marked 4 inline comments as done.

address comments and add a stress test.

This is a subtle change that needs careful review, and honestly should have a test.

I agree, and thanks for the nice review comments!

I'd like to add a unittest, but I don't see a straight way (I manually test it by comparing the number of allocated SLocEntries and the used SourceLocation address space in clang --print-stats with/out this patch), some options:

  1. construct a large TU that will make clang fails to compile without the 50 trick patch;
  2. create a small test, and verify the the source location is split when the two consecutive tokens distance > 50, and verify the number of in clang print-stats;

1 feels better to reflect the real behavior, the only concern is that this is a stress test, running it is expensive (we also have a similar SourceLocationsOverlow.c, I guess it is ok to add it).

clang/lib/Lex/TokenLexer.cpp
1018

missing the same-sloc-address-space check

oops, good spot. Indeed this is another behavior change in the previous-rewriting patch :( -- the original behavior was that if two consecutive tokens are not in the same address space, it split to two expansion SLocEntries.

Fixed.

This is a subtle change that needs careful review, and honestly should have a test.

I agree, and thanks for the nice review comments!

I'd like to add a unittest, but I don't see a straight way (I manually test it by comparing the number of allocated SLocEntries and the used SourceLocation address space in clang --print-stats with/out this patch), some options:

  1. construct a large TU that will make clang fails to compile without the 50 trick patch;
  2. create a small test, and verify the the source location is split when the two consecutive tokens distance > 50, and verify the number of in clang print-stats;

1 feels better to reflect the real behavior, the only concern is that this is a stress test, running it is expensive (we also have a similar SourceLocationsOverlow.c, I guess it is ok to add it).

I was all ready to say that option 2 is the only practical one, and was surprised that you had an actual testcase.
I'm a bit nervous (it must be slow and eat tons of ram) but if we already have a similar test...

clang/lib/Lex/TokenLexer.cpp
1018

oops, good spot. Indeed this is another behavior change in the previous-rewriting patch :(

Argh, I'm so sorry, this prompted me to take another look and there is no bug/need for the check at all.

Before we do the NearLast check we've already established that the FileID is the same, which means the locations are necessarily in the same address space.

We can replace the isInSameSLocAddrSpace with a raw offset comparison again...

hokein updated this revision to Diff 470599.Oct 25 2022, 12:54 PM
hokein marked an inline comment as not done.

refine the test.

This is a subtle change that needs careful review, and honestly should have a test.

I agree, and thanks for the nice review comments!

I'd like to add a unittest, but I don't see a straight way (I manually test it by comparing the number of allocated SLocEntries and the used SourceLocation address space in clang --print-stats with/out this patch), some options:

  1. construct a large TU that will make clang fails to compile without the 50 trick patch;
  2. create a small test, and verify the the source location is split when the two consecutive tokens distance > 50, and verify the number of in clang print-stats;

1 feels better to reflect the real behavior, the only concern is that this is a stress test, running it is expensive (we also have a similar SourceLocationsOverlow.c, I guess it is ok to add it).

I was all ready to say that option 2 is the only practical one, and was surprised that you had an actual testcase.
I'm a bit nervous (it must be slow and eat tons of ram) but if we already have a similar test...

now I change my mind -- this test is very expensive, taking ~8s to run (which is 4x slower than the SourceLocationsOverlow.c), adding it is not a good idea. Switched to 2.

clang/lib/Lex/TokenLexer.cpp
1018

hmm, I think there was some misunderstanding here -- I thought you meant a corner case where Last and T are in the same file, but Last.offset < CurrentLoadedOffset, and T.offset > CurrentLoadedOffset. For this case, there is a behavior change between both versions.

I'm not sure whether this case is possible or it is reasonable to handle it, since it is a running-out-of-sourcelocation-space case (clang's behavior is somewhat inconsistent, in CreateFileID, we emit a too-large diagnostic, in createExpansionLocImpl, it is just an assertion), maybe we should not worry about this case, treat it as UB.

sammccall accepted this revision.Oct 25 2022, 1:34 PM

LG other than removing the redundant same-sloc-address check again (sorry!)

clang/lib/Lex/TokenLexer.cpp
1018

hmm, I think there was some misunderstanding here -- I thought you meant a corner case where Last and T are in the same file, but Last.offset < CurrentLoadedOffset and T.offset > CurrentLoadedOffset

That's not possible, loaded vs local is a property of the FileID. I was missing that we'd established they're in the same FileID before checking deltas.

clang/test/Lexer/update_consecutive_macro_address_space.c
3

it's a shame there's no -cc1 flag for SourceManager::dump() or this could be much more direct...

hokein updated this revision to Diff 470764.Oct 26 2022, 3:02 AM

upload the full reland version.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 26 2022, 3:07 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
clang/lib/Lex/TokenLexer.cpp