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

Thu Jan 12 00:51:19 PST 2023

vedgy added a subscriber: milianw.
vedgy added a comment.

In D139774#4045361 <https://reviews.llvm.org/D139774#4045361>, @dblaikie wrote:

> 1. Should clang be doing something better with these temp files anyway, no matter the directory they go in? (setting them for cleanup at process exit or the like - I think we have such a filesystem abstraction, maybe that only works on Windows? I'm not sure)

That'd be great, but appears unfeasible on GNU/Linux in case of a crash: https://stackoverflow.com/questions/471344/guaranteed-file-deletion-upon-program-termination-c-c

> 2. If there isn't a better general way to avoid creating temp trash that's a problem, I'd have thought that KDevelop/other tools would have issues with other temp files too, and want a more general solution (I'd have thought changing the process's temp directory, and changing it back for user process launches, would be sufficient - so it can cleanup after itself for all files, not just these particular clang-created ones)

I do plan to put other temporary files KDevelop generates in the same session-specific temporary directory and clean it on start. But modifying KDevelop's temporary directory environment variable has been rejected during code review for good reason. As the bug this review aims to fix <https://github.com/llvm/llvm-project/issues/51847> puts it:

> setting the environment variables is problematic, because they are inherited by the IDE's code and all child processes it spawns (compiler, build system and user-provided executables). The IDE must then remove the temporary directory environment variable from each child process where it can cause undesirable behavior.

In D139774#4045460 <https://reviews.llvm.org/D139774#4045460>, @MaskRay wrote:

> libclang functions like `clang_reparseTranslationUnit_Impl` call `clang/lib/Frontend/ASTUnit.cpp:1397` and build the preamble with `/*StoreInMemory=*/false`.
> If `StoreInMemory` is configurable (true), then you can avoid the temporary file `preamble-*.pch`.
>
> `clang/lib/Frontend/ASTUnit.cpp` is used by `clang/lib/Tooling/Tooling.cpp` and libclang. Personally I think an unconditional `/*StoreInMemory=*/true` is fine. (The concern is slightly memory usage increase).

This is a simple and promising solution. But maybe it should be configurable. The `StoreInMemory` option was introduced for the benefit of //clangd//, and //clangd// still stores the preambles on disk by default. See D39843 <https://reviews.llvm.org/D39843>. I'm pinging another KDevelop developer who may know better how this should work in KDevelop: @milianw

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139774/new/

https://reviews.llvm.org/D139774