This is an archive of the discontinued LLVM Phabricator instance.

[clang][index] NFCI: Make `CXFile` a `FileEntryRef`
ClosedPublic

Authored by jansvoboda11 on Jun 1 2023, 2:09 PM.

Details

Summary

This patch swaps out the void * behind CXFile from FileEntry * to FileEntryRef::MapEntry *. This allows us to remove some deprecated uses of FileEntry::getName().

Depends on D151854.

Diff Detail

Event Timeline

jansvoboda11 created this revision.Jun 1 2023, 2:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2023, 2:09 PM
jansvoboda11 requested review of this revision.Jun 1 2023, 2:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2023, 2:09 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
benlangmuir added inline comments.Jun 1 2023, 3:01 PM
clang/test/Modules/crash-vfs-umbrella-frameworks.m
13 ↗(On Diff #527613)

This looks suspicious; what's going on here?

clang/tools/libclang/CXFile.h
2

Missing filename and C++ language tag from header comment.

clang/tools/libclang/CXLoadedDiagnostic.cpp
278–279

Does this preserve behaviour in call cases? operator[] will construct nullptr if the key is missing, but the new code will crash dereferencing an invalid iterator.

jansvoboda11 marked 3 inline comments as done.

Remove accidental diff with --crash in test, add file header, continue being defensive in CXLoadedDiagnostic.cpp.

clang/test/Modules/crash-vfs-umbrella-frameworks.m
13 ↗(On Diff #527613)

Bad cherry-pick, removed.

clang/tools/libclang/CXLoadedDiagnostic.cpp
278–279

You're right, let's continue being defensive here.

benlangmuir accepted this revision.Jun 14 2023, 9:16 AM
This revision is now accepted and ready to land.Jun 14 2023, 9:16 AM
This revision was landed with ongoing or failed builds.Jun 15 2023, 3:35 AM
This revision was automatically updated to reflect the committed changes.

Thanks for notifying me. Interestingly, I didn't receive any notification from that bot. Is it possible for me to reproduce this locally on my Mac? If not, could you please provide me with the stack trace for the failing call to cantFail()?

uabelho added inline comments.
clang/tools/libclang/CXFile.h
19

Gcc warns here:

../../clang/tools/libclang/CXFile.h:18:50: warning: cast from type 'const MapEntry*' {aka 'const llvm::StringMapEntry<llvm::ErrorOr<clang::FileEntryRef::MapValue> >*'} to type 'CXFile' {aka 'void*'} casts away qualifiers [-Wcast-qual]

Anything that should be fixed?

jansvoboda11 added inline comments.Jun 20 2023, 3:22 AM
clang/tools/libclang/CXFile.h
19

Thanks, should be fixed in 1c64c414.

Sorry for the late response @jansvoboda11, when lookupModuleFile is called, the file build/tools/clang/test/Index/Core/Output/index-with-module.m.tmp.mcp/A03A61VI43WA/ModA-21USRMHJNU3PG.pcm doesn't exist. So it seems there's some issue with the creation of the file.

The backtrace is:

#0  0x09000000005cefe4 in pthread_kill () from /usr/lib/libpthreads.a(shr_xpg5_64.o)
#1  0x09000000005ce808 in _p_raise () from /usr/lib/libpthreads.a(shr_xpg5_64.o)
#2  0x090000000004130c in raise () from /usr/lib/libc.a(shr_64.o)
#3  0x090000000005f49c in abort () from /usr/lib/libc.a(shr_64.o)
#4  0x0900000006cb4e30 in llvm::llvm_unreachable_internal (msg=<optimized out>, 
    file=0x8001000a0014bee <L.._MergedGlobals.658+12910> "/llvm-project/llvm/include/llvm/Support/Error.h", line=755) at /llvm-project/llvm/lib/Support/ErrorHandling.cpp:212
#5  0x0900000006ccc5ec in llvm::cantFail (Err=..., Msg=
    0x0 <clang::driver::toolchains::AMDGPUToolChain::getDefaultDenormsAreZeroForTarget(llvm::AMDGPU::GPUKind)>)
    at /llvm-project/llvm/include/llvm/Support/Error.h:755
#6  llvm::handleAllErrors<llvm::consumeError(llvm::Error)::{lambda(llvm::ErrorInfoBase const&)#1}>(llvm::Error, llvm::consumeError(llvm::Error)::{lambda(llvm::ErrorInfoBase const&)#1}&&) (E=..., Handlers=...)
    at /llvm-project/llvm/include/llvm/Support/Error.h:967
#7  0x0900000006d0ef30 in llvm::consumeError (Err=...)
    at /llvm-project/llvm/include/llvm/Support/Error.h:1044
#8  llvm::expectedToOptional<clang::FileEntryRef> (E=...)
    at /llvm-project/llvm/include/llvm/Support/Error.h:1057
#9  0x0000000102dc03a0 in clang::serialization::ModuleManager::lookupModuleFile (this=<optimized out>, FileName=..., 
    ExpectedSize=576478347173894528, ExpectedModTime=0, File=...)
    at /llvm-project/clang/lib/Serialization/ModuleManager.cpp:451
#10 0x0000000102dbf380 in clang::serialization::ModuleManager::addModule (this=0x110950d40, FileName=..., 
    Type=clang::serialization::MK_ExplicitModule, ImportLoc=..., 
    ImportedBy=0x0 <clang::driver::toolchains::AMDGPUToolChain::getDefaultDenormsAreZeroForTarget(llvm::AMDGPU::GPUKind)>, 
    Generation=67, ExpectedSize=-1, ExpectedModTime=-1, ExpectedSignature=..., 
    ReadSignature=@0x11071e1a8: 0x102dbd514 <readASTFileSignature(llvm::StringRef)>, 
    Module=@0xfffffffffffd678: 0x0 <clang::driver::toolchains::AMDGPUToolChain::getDefaultDenormsAreZeroForTarget(llvm::AMDGPU::GPUKind)>, ErrorStr=...) at /llvm-project/clang/lib/Serialization/ModuleManager.cpp:121
#11 0x0000000102dbc2a8 in clang::ASTReader::ReadASTCore (this=0x110950cb0, FileName=..., 
    Type=clang::serialization::MK_ImplicitModule, ImportLoc=..., 
    ImportedBy=0x0 <clang::driver::toolchains::AMDGPUToolChain::getDefaultDenormsAreZeroForTarget(llvm::AMDGPU::GPUKind)>, 
    Loaded=..., ExpectedSize=-1, ExpectedModTime=0, ExpectedSignature=..., ClientLoadCapabilities=19)
    at /llvm-project/clang/lib/Serialization/ASTReader.cpp:4584
#12 0x0000000102dbad14 in clang::ASTReader::ReadAST(llvm::StringRef, clang::serialization::ModuleKind, clang::SourceLocation, unsigned int, llvm::SmallVectorImpl<clang::ASTReader::ImportedSubmodule>*) ()
#13 0x0000000102c826a8 in clang::CompilerInstance::findOrCompileModuleAndReadAST(llvm::StringRef, clang::SourceLocation, clang::SourceLocation, bool) ()
#14 0x0000000102c7e75c in clang::CompilerInstance::loadModule (this=0x110950cc8, ImportLoc=..., Path=..., 
    Visibility=(clang::Module::AllVisible | unknown: 0x12), IsInclusionDirective=<optimized out>)
    at /llvm-project/clang/lib/Frontend/CompilerInstance.cpp:1995
#15 0x00000001001143ac in clang::Preprocessor::LexAfterModuleImport (this=0x1108ee6c8, Result=...)
    at /llvm-project/clang/lib/Lex/Preprocessor.cpp:1323
#16 0x00000001001132e4 in clang::Preprocessor::Lex (
this=0x110981100, Result=...) at /llvm-project/clang/lib/Lex/Preprocessor.cpp:899
#17 0x000000010271eb1c in clang::Parser::ConsumeToken (this=0x1109810f0)
    at /llvm-project/clang/include/clang/Parse/Parser.h:504
#18 clang::Parser::ParseModuleName (this=0x1109810f0, UseLoc=..., Path=..., IsImport=<optimized out>)
    at /llvm-project/clang/lib/Parse/Parser.cpp:2638
#19 0x000000010271de04 in clang::Parser::ParseModuleImport (this=0x1109810f0, AtLoc=..., 
    ImportState=@0x1: clang::Sema::FirstDecl) at /llvm-project/clang/lib/Parse/Parser.cpp:2526
#20 0x000000010296637c in clang::Parser::ParseObjCAtDirectives (this=0x1109810f0, DeclAttrs=..., DeclSpecAttrs=...)
    at /llvm-project/clang/lib/Parse/ParseObjc.cpp:99
#21 0x0000000102842e58 in clang::Parser::ParseExternalDeclaration (this=0x1109810f0, Attrs=..., DeclSpecAttrs=..., 
    DS=0x0 <clang::driver::toolchains::AMDGPUToolChain::getDefaultDenormsAreZeroForTarget(llvm::AMDGPU::GPUKind)>)
    at /llvm-project/clang/lib/Parse/Parser.cpp:908
#22 0x000000010271a934 in clang::Parser::ParseTopLevelDecl (this=0x1109810f0, Result=..., 
    ImportState=@0xfffffffffffeef8: clang::Sema::FirstDecl)
    at /llvm-project/clang/lib/Parse/Parser.cpp:742
#23 0x0000000102719524 in clang::Parser::ParseFirstTopLevelDecl (this=0xfffffffffffef90, Result=..., 
    ImportState=@0xffffffffffff1e8: clang::Sema::FirstDecl)
    at /llvm-project/clang/lib/Parse/Parser.cpp:594
#24 0x0000000101693200 in clang::ParseAST (S=..., PrintStats=<optimized out>, SkipFunctionBodies=<optimized out>)
    at /llvm-project/clang/lib/Parse/ParseAST.cpp:162
#25 0x00000001015f4d80 in clang::ASTFrontendAction::ExecuteAction() ()
#26 0x0000000102f96d18 in clang::FrontendAction::Execute() ()
    at /opt/IBM/openxlC/17.1.1/bin/../include/c++/v1/optional:957
#27 0x0000000102c77838 in clang::ASTUnit::LoadFromCompilerInvocationAction (CI=..., PCHContainerOps=..., Diags=..., 
    Action=<optimized out>, Unit=<optimized out>, Persistent=<optimized out>, ResourceFilesPath=..., 
    OnlyLocalDecls=<optimized out>, CaptureDiagnostics=<optimized out>, PrecompilePreambleAfterNParses=<optimized out>, 
    CacheCodeCompletionResults=<optimized out>, UserFilesAreVolatile=<optimized out>, ErrAST=<optimized out>)
    at /llvm-project/clang/lib/Frontend/ASTUnit.cpp:1651
#28 0x000000010002c1c4 in printSourceSymbols (Executable=<optimized out>, Args=..., indexLocals=<optimized out>, 
    dumpModuleImports=<optimized out>, ignoreMacros=<optimized out>)
    at /llvm-project/clang/tools/c-index-test/core_main.cpp:241
#29 indextest_core_main (argc=<optimized out>, argv=<optimized out>)
    at /llvm-project/clang/tools/c-index-test/core_main.cpp:370
#30 0x0000000100000a1c in main (argc=276155128, argv=0xffffffffffff8c0)
    at /llvm-project/clang/tools/c-index-test/c-index-test.c:5142

Sorry for the late response @jansvoboda11, when lookupModuleFile is called, the file build/tools/clang/test/Index/Core/Output/index-with-module.m.tmp.mcp/A03A61VI43WA/ModA-21USRMHJNU3PG.pcm doesn't exist. So it seems there's some issue with the creation of the file.

The backtrace is:

...

@Jake-Egan Thank you for the stack trace and XFAILing the test! Other tests started failing after converting more FileEntry usages to FileEntryRef: https://lab.llvm.org/buildbot/#/builders/214/builds/9416

There are definitely some behavioral changes these patches introduce, but I don't see anything obviously wrong. What's more concerning, though, is that llvm::expectedToOptional()/llvm::cantFail() end up in llvm_unreachable(). Are you able to investigate? If not, could you provide me a way to reproduce this? These failures only occur on the AIX bots AFAIK.

@jansvoboda11 Thanks for taking care of the other tests. I'm not sure how this could be reproduced not on AIX. I'll have to investigate.

@jansvoboda11 Actually, we could give you access to an AIX machine to debug this. Would that work for you?

@jansvoboda11 Actually, we could give you access to an AIX machine to debug this. Would that work for you?

Hi, I'd be happy to try that!