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

Duncan P. N. Exon Smith via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 26 11:54:53 PDT 2020


dexonsmith added a comment.

In D89749#2353602 <https://reviews.llvm.org/D89749#2353602>, @teemperor wrote:

> 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.

I haven't reproduced yet (still building), but I think I found the problem by inspection:

  Expected<FileID> ASTImporter::Import(FileID FromID, bool IsBuiltin) {
    llvm::DenseMap<FileID, FileID>::iterator Pos = ImportedFileIDs.find(FromID);
    if (Pos != ImportedFileIDs.end())
      return Pos->second;
  
    SourceManager &FromSM = FromContext.getSourceManager();
    SourceManager &ToSM = ToContext.getSourceManager();
    const SrcMgr::SLocEntry &FromSLoc = FromSM.getSLocEntry(FromID);
  
    // Various recursive calls to Import.

After this patch, the address of `SLocEntry` is potentially invalidated by subsequent calls to `getSLocEntry`, since a load can increase the capacity of `LoadedSLocEntryTable` and move its allocation.

That means it's not safe to store an address from `getSLocEntry` when there will be another call. I can update `ASTImporter` to use a copy, but I think this is too much of a gotcha... I'll think more about what to do, but here are the ideas I have:

1. Return an `SLocEntry` by-value. After https://reviews.llvm.org/D89580 that's just 24B (down from 40B), so maybe this is reasonable.
2. Return an `SLocEntryRef` by-value, a new type that is (say) a pointer to the correct `SLocEntryTable` and an index into it. This would be 16B.

Let me know if you have thoughts.


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

https://reviews.llvm.org/D89749



More information about the cfe-commits mailing list