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