[PATCH] D89749: SourceManager: Don't allocate an SLocEntry until it's loaded

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a subscriber: v.g.vassilev.
teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Maybe arc didn't correctly apply the patch, but this causes a few LLDB tests to fail on my machine:

  lldb-api :: commands/expression/import-std-module/queue/TestQueueFromStdModule.py
  lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py

>From the backtrace this seems like the ASTImporter needs to be updated:

  * frame #0: 0x00007ffff25f1bb6 _lldb.so`clang::ASTImporter::Import(clang::FileID, bool) + 566
    frame #1: 0x00007ffff25ecc56 _lldb.so`clang::ASTImporter::Import(clang::SourceLocation) + 182
    frame #2: 0x00007ffff25bbaed _lldb.so`clang::SourceLocation clang::ASTNodeImporter::importChecked<clang::SourceLocation>(llvm::Error&, clang::SourceLocation const&) + 61
    frame #3: 0x00007ffff25c34e8 _lldb.so`clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*) + 2968
    frame #4: 0x00007ffff25ebb31 _lldb.so`clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*) + 81
    frame #5: 0x00007ffff25ebac2 _lldb.so`clang::ASTImporter::ImportImpl(clang::Decl*) + 34
    frame #6: 0x00007ffff0df9cb1 _lldb.so`lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl(clang::Decl*) + 1345
    frame #7: 0x00007ffff25ed3f4 _lldb.so`clang::ASTImporter::Import(clang::Decl*) + 532
    frame #8: 0x00007ffff25aeb93 _lldb.so`llvm::Expected<clang::ValueDecl*> clang::ASTNodeImport

The tests are just loading a Clang module and then trying to import some Decls into another ASTContext, so I don't think the error here is specific to LLDB. We just don't have any ASTImporter tests that exercise modules.

FWIW, if you quickly want to see if your SourceManager changes break LLDB, then you can just set the LIT_FILTER to "Modules" and run `check-lldb` as LLDB's use of Clang modules are the only place where we have any valid SourceLocations within LLDB.

Regarding the patch itself:

1. The `I - 1` everywhere when translating `SLocEntryLoaded` values to a `LoadedSLocEntryTable` index is a bit cryptic. We already do similar magic in other places, but here it's really just about expressing an optional integer. Could we maybe have a wrapper function that turns this into a `llvm::Optional<unsigned>` (that is `llvm::None` when it's not loaded). Something like this maybe (which hopefully optimizes to something similar to the original code):

  llvm::Optional<unsigned> getLoadedSLocEntryTableIndex(size_t Index) const {
    unsigned result = SLocEntryLoaded[Index];
    if (result == /*sentinel value*/0)
      return llvm::None;
    return result - 1U;
  }



2. If we have this as an Optional, we might as well make the sentinel value `std::numeric_limits<unsigned>::max()` instead of 0 in `SLocEntryLoaded`. Then we can just all values as-is without translating them to an index.

3. Now that the primary purpose of `SLocEntryLoaded` is storing indices and not the loaded status, maybe something like `SLocEntryIndices` would be a better name?

4. Do you still have your benchmarking values (or some estimates) for this? I'm just curious how much memory this actually saves.

Otherwise I think this looks good.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89749/new/

https://reviews.llvm.org/D89749