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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
