[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D143418#4117246 <https://reviews.llvm.org/D143418#4117246>, @vedgy wrote:

> In D143418#4116290 <https://reviews.llvm.org/D143418#4116290>, @aaron.ballman wrote:
>
>>> So how about a more sincere general solution: setPreferredTempDirPath()? The documentation would say that libclang tries its best to place temporary files into the specified directory, but some might end up in the system temporary directory instead.
>>
>> I don't think those semantics are reasonable. "Tries its best" is fine for things that are heuristic based, like deciding when to do an analysis cut point, but is too non-deterministic for something like "where do files get placed". I think it's reasonable that we either:
>>
>> 1. expose an option to say "I prefer preambles to be put in this directory" for libclang, or
>> 2. expose an option to say "I prefer using this directory over the temp directory for all files" for libclang,
>>
>> I thought we agreed that #2 is too high of risk because it requires auditing libclang for all the places it decides to put something into the temp directory, and so we were going with #1 and doing it as part of the cindex constructor so that users cannot run into the surprise issues (multithreading, files changing location mid-run) with an independent setter.
>
> This revision implements #2, but warns about possible imperfections of the implementation.

I don't think "tries hard" is a good enough guarantee for where files are placed. I'm not comfortable with the security posture of that approach as it could potentially leak sensitive information (try to override the temp directory to something that's chroot jailed, get sensitive files showing up in the system temp directory anyway).

> In D139774#4066519 <https://reviews.llvm.org/D139774#4066519>, @dblaikie wrote:
>
>> In D139774#4065753 <https://reviews.llvm.org/D139774#4065753>, @aaron.ballman wrote:
>>
>>> From a design perspective, my preference is to not add new APIs at the file system support layer in LLVM to override the temporary directory but instead allow individual components to override the decision where to put files. Overriding a system directory at the LLVM support level feels unclean to me because 1) threading concerns (mostly related to lock performance), 2) consistency concerns (put files in temp directory, override temp directory, put additional files in the new directory), 3) principle of least surprise. To me, the LLVM layer is responsible for interfacing with the system and asking for the system temp directory should get you the same answer as querying for that from the OS directly.
>>
>> I've mixed feelings about this, but yeah, I guess the issues with global state are pretty undesirable. I don't worry too much about (2) - doesn't feel too problematic to me (for any individual file, presumably the implementation recorded the name of the file if they were going to access it again - so still be using the old path if necessary).
>>
>> But, yeah, lack of any "system" abstraction that could be passed into all the various LLVM/MC/AST contexts to swap out the implementation limits the options a bit to more ad-hoc/case-by-case solutions. I feel like that's not a great solution to KDevelop's general problem, though - they're dealing with one particularly big file, but it doesn't feel very principled to me to fix that one compared to all the other (albeit smaller) temp files & maybe at some point in the future some bigger temp files are created elsewhere...
>
> Based on this comment and on current preamble storage path code, I thought files-changing-location-mid-run should not be a problem.
>
> Does the multithreading issue mean that `clang_parseTranslationUnit_Impl()` can be called in a non-main thread and thus concurrently with `clang_CXIndex_setPreferredTempDirPath()`?

Yes. However, I don't think that situation is officially supported (it's more that we don't want to knowingly make it harder to support in the future).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418