[PATCH] D144651: [Serialization] Place command line defines in the correct file
Michael Buch via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 24 10:23:03 PDT 2023
Michael137 added a comment.
FWIW here's some info on how we get to the assert. Maybe someone with more understanding of the `clang::SourceManager` and how it interacts with `.pcm`s will be able to spot something.
Here's a bit more of the stack trace:
frame #41: 0x000000013844c7e0 liblldb.17.0.0git.dylib`clang::Sema::LookupParsedName(this=0x0000000177def200, R=0x000000016fdea010, S=0x0000000164a6e300, SS=0x000000016fdeb1b8, AllowBuiltinCreation=true, EnteringContext=false) at SemaLookup.cpp:2741:10
frame #40: 0x000000013844afd0 liblldb.17.0.0git.dylib`clang::Sema::LookupName(this=0x0000000177def200, R=0x000000016fdea010, S=0x0000000164a6e300, AllowBuiltinCreation=true, ForceNoCPlusPlus=false) at SemaLookup.cpp:2267:9
frame #39: 0x00000001384468a0 liblldb.17.0.0git.dylib`clang::Sema::CppLookupName(this=0x0000000177def200, R=0x000000016fdea010, S=0x0000000164a6df70) at SemaLookup.cpp:1495:15
frame #38: 0x00000001384474e4 liblldb.17.0.0git.dylib`CppNamespaceLookup(S=0x0000000177def200, R=0x000000016fdea010, Context=0x0000000177de2c00, NS=0x0000000177deb830, UDirs=0x000000016fde9a78)::UnqualUsingDirectiveSet&) at SemaLookup.cpp:1207:16
frame #37: 0x000000013844b208 liblldb.17.0.0git.dylib`LookupDirect(S=0x0000000177def200, R=0x000000016fdea010, DC=0x0000000177deb830) at SemaLookup.cpp:1109:39
frame #36: 0x0000000139512644 liblldb.17.0.0git.dylib`clang::DeclContext::lookup(this=0x0000000177deb830, Name=(Ptr = 5968768496)) const at DeclBase.cpp:1743:17
frame #35: 0x0000000131c48b88 liblldb.17.0.0git.dylib`lldb_private::ClangASTSource::ClangASTSourceProxy::FindExternalVisibleDeclsByName(this=0x0000600000344060, DC=0x0000000177deb830, Name=(Ptr = 5968768496)) at ClangASTSource.h:216:25
frame #34: 0x0000000134f0ab40 liblldb.17.0.0git.dylib`lldb_private::ClangASTSource::FindExternalVisibleDeclsByName(this=0x0000600003b3e140, decl_ctx=0x0000000177deb830, clang_decl_name=(Ptr = 5968768496)) at ClangASTSource.cpp:180:3
frame #33: 0x0000000134f3b9d8 liblldb.17.0.0git.dylib`lldb_private::ClangExpressionDeclMap::FindExternalVisibleDecls(this=0x0000600003b3e140, context=0x000000016fde9008) at ClangExpressionDeclMap.cpp:727:5
frame #32: 0x0000000134f3c1d8 liblldb.17.0.0git.dylib`lldb_private::ClangExpressionDeclMap::FindExternalVisibleDecls(this=0x0000600003b3e140, context=0x000000016fde9008, module_sp=nullptr, namespace_decl=0x000000016fde8c90) at ClangExpressionDeclMap.cpp:1450:3
frame #31: 0x0000000134f3fb80 liblldb.17.0.0git.dylib`lldb_private::ClangExpressionDeclMap::LookupFunction(this=0x0000600003b3e140, context=0x000000016fde9008, module_sp=nullptr, name=(m_string = "CGImageGetRenderingIntent"), namespace_decl=0x000000016fde8c90) at ClangExpressionDeclMap.cpp:1333:48
frame #30: 0x0000000134f0e2c0 liblldb.17.0.0git.dylib`lldb_private::ClangASTSource::CopyDecl(this=0x0000600003b3e140, src_decl=0x000000016462a480) at ClangASTSource.cpp:1728:29
frame #29: 0x0000000134eedd58 liblldb.17.0.0git.dylib`lldb_private::ClangASTImporter::CopyDecl(this=0x00000001050683a8, dst_ast=0x0000000177de2c00, decl=0x000000016462a480) at ClangASTImporter.cpp:78:55
frame #28: 0x000000013924cbd4 liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromD=0x000000016462a480) at ASTImporter.cpp:9036:27
frame #27: 0x0000000134ef2a84 liblldb.17.0.0git.dylib`lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl(this=0x000000010b420000, From=0x000000016462a480) at ClangASTImporter.cpp:892:23
frame #26: 0x00000001392748dc liblldb.17.0.0git.dylib`clang::ASTImporter::ImportImpl(this=0x000000010b420000, FromD=0x000000016462a480) at ASTImporter.cpp:8633:19
frame #25: 0x0000000139274dc0 liblldb.17.0.0git.dylib`clang::declvisitor::Base<std::__1::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*>>::Visit(this=0x000000016fde7a40, D=0x000000016462a480) at DeclNodes.inc:433:1
frame #24: 0x000000013923e90c liblldb.17.0.0git.dylib`clang::ASTNodeImporter::VisitFunctionDecl(this=0x000000016fde7a40, D=0x000000016462a480) at ASTImporter.cpp:3603:44
frame #23: 0x000000013924026c liblldb.17.0.0git.dylib`std::__1::conditional<std::is_base_of_v<clang::Type, clang::ParmVarDecl>, llvm::Expected<clang::ParmVarDecl const*>, llvm::Expected<clang::ParmVarDecl*>>::type clang::ASTNodeImporter::import<clang::ParmVarDecl>(this=0x000000016fde7a40, From=0x00000001035d6e80) at AS
frame #22: 0x000000013924cbd4 liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromD=0x00000001035d6e80) at ASTImporter.cpp:9036:27
frame #21: 0x0000000134ef2a84 liblldb.17.0.0git.dylib`lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl(this=0x000000010b420000, From=0x00000001035d6e80) at ClangASTImporter.cpp:892:23
frame #20: 0x00000001392748dc liblldb.17.0.0git.dylib`clang::ASTImporter::ImportImpl(this=0x000000010b420000, FromD=0x00000001035d6e80) at ASTImporter.cpp:8633:19
frame #19: 0x0000000139274eb0 liblldb.17.0.0git.dylib`clang::declvisitor::Base<std::__1::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*>>::Visit(this=0x000000016fde6530, D=0x00000001035d6e80) at DeclNodes.inc:507:1
frame #18: 0x0000000139246aec liblldb.17.0.0git.dylib`clang::ASTNodeImporter::VisitParmVarDecl(this=0x000000016fde6530, D=0x00000001035d6e80) at ASTImporter.cpp:4394:26
frame #17: 0x0000000139235924 liblldb.17.0.0git.dylib`clang::SourceLocation clang::ASTNodeImporter::importChecked<clang::SourceLocation>(this=0x000000016fde6530, Err=0x000000016fde6480, From=0x000000016fde6458) at ASTImporter.cpp:696:30
frame #16: 0x0000000139221da0 liblldb.17.0.0git.dylib`llvm::Expected<clang::SourceLocation> clang::ASTNodeImporter::import<clang::SourceLocation>(this=0x000000016fde6530, From=0x000000016fde6458) at ASTImporter.cpp:218:23
frame #15: 0x00000001392764bc liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromLoc=(ID = 4217149690)) at ASTImporter.cpp:9511:36
frame #14: 0x0000000139280544 liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromID=(ID = -265130), IsBuiltin=false) at ASTImporter.cpp:9541:28
frame #13: 0x00000001392764bc liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromLoc=(ID = 2068864938)) at ASTImporter.cpp:9511:36
frame #12: 0x0000000139280884 liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromID=(ID = -276169), IsBuiltin=false) at ASTImporter.cpp:9564:35
frame #11: 0x0000000139276468 liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromLoc=(ID = 2356792)) at ASTImporter.cpp:9501:27
frame #10: 0x000000013928034c liblldb.17.0.0git.dylib`clang::SourceManager::isWrittenInBuiltinFile(this=0x0000000105070470, Loc=(ID = 2356792)) const at SourceManager.h:1480:28
frame #9: 0x000000013b6ef804 liblldb.17.0.0git.dylib`clang::SourceManager::getPresumedLoc(this=0x0000000105070470, Loc=(ID = 2356792), UseLineDirectives=true) const at SourceManager.cpp:1528:41
frame #8: 0x000000013b6e8140 liblldb.17.0.0git.dylib`clang::SourceManager::getDecomposedExpansionLoc(this=0x0000000105070470, Loc=(ID = 2356792)) const at SourceManager.h:1259:18
frame #7: 0x000000013910f0d4 liblldb.17.0.0git.dylib`clang::SourceManager::getFileID(this=0x0000000105070470, SpellingLoc=(ID = 2356792)) const at SourceManager.h:1119:12
frame #6: 0x000000013910f200 liblldb.17.0.0git.dylib`clang::SourceManager::getFileID(this=0x0000000105070470, SLocOffset=2356792) const at SourceManager.h:1834:12
frame #5: 0x000000013b6ed2d0 liblldb.17.0.0git.dylib`clang::SourceManager::getFileIDSlow(this=0x0000000105070470, SLocOffset=2356792) const at SourceManager.cpp:781:10
frame #4: 0x000000013b6ed620 liblldb.17.0.0git.dylib`clang::SourceManager::getFileIDLoaded(this=0x0000000105070470, SLocOffset=2356792) const at SourceManager.cpp:870:5
So this happens when we try `clang::ASTImport` a `SourceLocation`. The particular decl we're importing is:
ParmVarDecl 0x164d43880 </Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/System/Library/Frameworks/CoreGraphics.framework/<built-in>:408:20, /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/System/Library/Frameworks/CoreGraphics.framework/Headers/CGImage.h:275:83> col:83 imported in CoreGraphics.CGImage image 'CGImageRef _Nullable':'struct CGImage *'
The `<built-in>` in question is `__nullable`, which is only present on Darwin platforms.
Without the patch the decl looks like this:
ParmVarDecl 0x16a827a80 <<built-in>:409:20, /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/System/Library/Frameworks/CoreGraphics.framework/Headers/CGImage.h:275:83> col:83 imported in CoreGraphics.CGImage image 'CGImageRef _Nullable':'struct CGImage *'
(note how the location for the `<built-in>` doesn't have the directory prefix and the line number differs; though I don't think this is the cause of the crash)
The crashing import gets started as such:
// frame #12
Expected<FileID> ASTImporter::Import(FileID FromID, bool IsBuiltin) {
....
if (!IsBuiltin && !Cache->BufferOverridden) {
// Include location of this file.
ExpectedSLoc ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc()); <<< Crashes
if (!ToIncludeLoc)
return ToIncludeLoc.takeError();
....
In `frame #11` the `IsBuiltin` gets derived as such:
bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
...which uses the `PresumedLoc` filename to determine whether the source location is a built-in. But after this patch, the filename is set to:
(lldb) p FromSM.getPresumedLoc(FromLoc, true)
(clang::PresumedLoc) $6 = {
Filename = 0x0000000107f8d168 "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/System/Library/Frameworks/CoreGraphics.framework/<built-in>"
ID = (ID = 0)
Line = 408
Col = 20
IncludeLoc = (ID = 2356792)
}
Since this patch started setting the `hasLineDirectives`, `getPresumedLoc` now goes down following codepath:
...
PresumedLoc SourceManager::getPresumedLoc(SourceLocation Loc,
bool UseLineDirectives) const {
if (UseLineDirectives && FI.hasLineDirectives()) {
....
if (const LineEntry *Entry =
LineTable->FindNearestLineEntry(LocInfo.first, LocInfo.second)) {
// If the LineEntry indicates a filename, use it.
if (Entry->FilenameID != -1) {
Filename = LineTable->getFilename(Entry->FilenameID);
// The contents of files referenced by #line are not in the
// SourceManager
FID = FileID::get(0);
}
...
I can confirm that the filename stored in the LineTable does have that prefix.
The prefixes in the line-table get written in:
void ASTReader::ParseLineTable(ModuleFile &F, const RecordData &Record) {
...
for (unsigned I = 0; Record[Idx]; ++I) {
// Extract the file name
FileIDs[I] = LineTable.getLineTableFilenameID(ReadPath(F, Record, Idx));
}
...
}
Inside `ReadPath` we call into:
/// If we are loading a relocatable PCH or module file, and the filename
/// is not an absolute path, add the system or module root to the beginning of
/// the file name.
void ASTReader::ResolveImportedPath(ModuleFile &M, std::string &Filename) {
If you dump the filenames that we create here you can see that even before the patch, we would add filenames into the line table of the form: `/some/path/<built-in>` and `/some/path/<command-line>`
But now that `hasLineDirectives` is being set, we end up reading these out. I wonder if we should just not add the root paths to `<built-in>` filenames.
I've not spent much time around this code so I don't know what the implications are here
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144651/new/
https://reviews.llvm.org/D144651
More information about the cfe-commits
mailing list