[PATCH] D139774: [libclang] Add API to set temporary directory location

Tue Jan 10 10:39:33 PST 2023

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 at 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 at 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