aaron.ballman added a comment. In D139774#4040057 <https://reviews.llvm.org/D139774#4040057>, @vedgy wrote:
> In D139774#4039975 <https://reviews.llvm.org/D139774#4039975>, @aaron.ballman > wrote: > >> Oh that is a good point! Apologies for not noticing that earlier -- that >> makes me wonder if a better approach here is to add a `std::string` to the >> `CIndexer` class to represent the temp path to use. > > I have suggested the possibility in this review: > >> A copy of user-provided temporary directory path buffer can be stored in >> class `CIndexer`. But this API in //llvm/Support/Path.h// would stay fragile. > > So the question is whether the LLVM API or `CIndexer` should store the buffer. My goal is to not modify the LLVM path APIs at all. > The only possible caller of `system_temp_directory()` used by libclang is > `llvm::sys::fs::createUniquePath()`. This helper function is called by > `createUniqueEntity()`, which in turn is called by several other helper > functions, all in //llvm/lib/Support/Path.cpp//. Here is a backtrace from > KDevelop built against LLVM at this review's branch: > > .... > #6 0x00007fb3717cc1b7 in (anonymous namespace)::TempPCHFile::create () at > llvm-project/llvm/include/llvm/ADT/Twine.h:233 > #7 clang::PrecompiledPreamble::Build(clang::CompilerInvocation const&, > llvm::MemoryBuffer const*, clang::PreambleBounds, clang::DiagnosticsEngine&, > llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>, > std::shared_ptr<clang::PCHContainerOperations>, bool, > clang::PreambleCallbacks&) (Invocation=..., MainFileBuffer=0x7fb35000a360, > Bounds=Bounds@entry=..., Diagnostics=..., VFS=..., > PCHContainerOps=std::shared_ptr<clang::PCHContainerOperations> (use count 5, > weak count 0) = {...}, StoreInMemory=false, Callbacks=...) at > llvm-project/clang/lib/Frontend/PrecompiledPreamble.cpp:421 > #8 0x00007fb3717234a8 in > clang::ASTUnit::getMainBufferWithPrecompiledPreamble(std::shared_ptr<clang::PCHContainerOperations>, > clang::CompilerInvocation&, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>, > bool, unsigned int) (this=0x7fb3505c44d0, > PCHContainerOps=std::shared_ptr<clang::PCHContainerOperations> (use count 5, > weak count 0) = {...}, PreambleInvocationIn=..., VFS=..., > AllowRebuild=<optimized out>, MaxLines=0) at > /usr/include/c++/12.2.0/bits/unique_ptr.h:191 > #9 0x00007fb371729bd6 in > clang::ASTUnit::LoadFromCompilerInvocation(std::shared_ptr<clang::PCHContainerOperations>, > unsigned int, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>) > (this=0x7fb3505c44d0, > PCHContainerOps=std::shared_ptr<clang::PCHContainerOperations> (use count 5, > weak count 0) = {...}, PrecompilePreambleAfterNParses=<optimized out>, > VFS=...) at llvm-project/clang/lib/Frontend/ASTUnit.cpp:1685 > #10 0x00007fb37172e359 in clang::ASTUnit::LoadFromCommandLine(char const**, > char const**, std::shared_ptr<clang::PCHContainerOperations>, > llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine>, llvm::StringRef, bool, > clang::CaptureDiagsKind, > llvm::ArrayRef<std::pair<std::__cxx11::basic_string<char, > std::char_traits<char>, std::allocator<char> >, llvm::MemoryBuffer*> >, bool, > unsigned int, clang::TranslationUnitKind, bool, bool, bool, > clang::SkipFunctionBodiesScope, bool, bool, bool, bool, > llvm::Optional<llvm::StringRef>, std::unique_ptr<clang::ASTUnit, > std::default_delete<clang::ASTUnit> >*, > llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>) (ArgBegin=<optimized out>, > ArgEnd=<optimized out>, > PCHContainerOps=std::shared_ptr<clang::PCHContainerOperations> (empty) = > {...}, Diags=..., ResourceFilesPath=..., OnlyLocalDecls=false, > CaptureDiagnostics=clang::CaptureDiagsKind::All, RemappedFiles=..., > RemappedFilesKeepOriginalName=true, PrecompilePreambleAfterNParses=1, > TUKind=clang::TU_Complete, CacheCodeCompletionResults=true, > IncludeBriefCommentsInCodeCompletion=false, AllowPCHWithCompilerErrors=true, > SkipFunctionBodies=clang::SkipFunctionBodiesScope::None, > SingleFileParse=false, UserFilesAreVolatile=true, ForSerialization=false, > RetainExcludedConditionalBlocks=false, ModuleFormat=..., > ErrAST=0x7fb36effd2d8, VFS=...) at > llvm-project/clang/lib/Frontend/ASTUnit.cpp:1822 > #11 0x00007fb37104e462 in clang_parseTranslationUnit_Impl(CXIndex, char > const*, char const* const*, int, llvm::ArrayRef<CXUnsavedFile>, unsigned int, > CXTranslationUnit*) (CIdx=0x55dbe16c9360, source_filename=<optimized out>, > command_line_args=<optimized out>, num_command_line_args=<optimized out>, > unsaved_files=..., options=781, out_TU=0x7fb35ca7f630) at > /usr/include/c++/12.2.0/bits/stl_vector.h:987 > #12 0x00007fb37104f2b4 in operator() (__closure=0x7fb37a7b6f40) at > llvm-project/clang/tools/libclang/CIndex.cpp:3976 > #13 > llvm::function_ref<void()>::callback_fn<clang_parseTranslationUnit2FullArgv(CXIndex, > char const*, char const* const*, int, CXUnsavedFile*, unsigned int, unsigned > int, CXTranslationUnitImpl**)::<lambda()> >(intptr_t) > (callable=callable@entry=140408830783296) at > llvm-project/llvm/include/llvm/ADT/STLFunctionalExtras.h:45 > #14 0x00007fb3729cc6e0 in llvm::function_ref<void ()>::operator()() const > (this=<synthetic pointer>) at > llvm-project/llvm/include/llvm/ADT/STLFunctionalExtras.h:68 > #15 llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) > (this=<optimized out>, Fn=...) at > llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:433 > #16 0x00007fb3729cc724 in RunSafelyOnThread_Dispatch(void*) > (UserData=0x7fb37a7b6e80) at > llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:514 > #17 0x00007fb3729cc19a in llvm::thread::Apply<void (*)(void*), (anonymous > namespace)::RunSafelyOnThreadInfo*, 0> (Callee=std::tuple containing = {...}) > at llvm-project/llvm/include/llvm/Support/thread.h:42 > #18 llvm::thread::GenericThreadProxy<std::tuple<void (*)(void*), (anonymous > namespace)::RunSafelyOnThreadInfo*> > (Ptr=0x55dbe3921080) at > llvm-project/llvm/include/llvm/Support/thread.h:50 > #19 llvm::thread::ThreadProxy<std::tuple<void (*)(void*), (anonymous > namespace)::RunSafelyOnThreadInfo*> >(void*) (Ptr=0x55dbe3921080) at > llvm-project/llvm/include/llvm/Support/thread.h:60 > #20 0x00007fb3e909f8fd in () at /usr/lib/libc.so.6 > #21 0x00007fb3e9121a60 in () at /usr/lib/libc.so.6 Thank you for this trace, that helps identify where I think this functionality should live, which is `CIndexer` At a point where we have a `CIndexer` object, we eventually call `ASTUnit::LoadFromCommandLine()` and `ASTUnit` has a member `FileSystemOptions FileSystemOpts`, and `FileSystemOptions` has a `std::string` for the working directory to use. I think we should store the temp directory in here as well, and when `ASTUnit::getMainBufferWithPrecompiledPreamble()` build the preamble, it can pass that information along to `TempPCHFile` to avoid needing to ask the system for the temp directory. This does mean we store two copies of the string (one in `CIndexer` and one in `FileSystemOptions`, but I think the improved ownership semantics for the C API makes that duplication somewhat reasonable. Does this idea seem plausible to you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139774/new/ https://reviews.llvm.org/D139774 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
