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

Thu Jan 19 10:43:45 PST 2023

dblaikie added a comment.

In D139774#4065753 <https://reviews.llvm.org/D139774#4065753>, @aaron.ballman wrote:

> In D139774#4063131 <https://reviews.llvm.org/D139774#4063131>, @vedgy wrote:
>
>> After a discussion under the corresponding KDevelop merge request, I can see 4-6 alternative ways to address the temporary directory issue:
>>
>> 1. Add an option to store the //preamble-*.pch// files in RAM instead of /tmp and add a corresponding option in KDevelop configuration UI. This would work perfectly for me, provided I don't change my mind and decide to turn this option off, in which case I'll be back to square one.
>> 2. Add an option to store the //preamble-*.pch// files in a custom directory instead of a general temporary directory. The option could be named generally (2a: overrideTemporaryDirectory) or specially (2b: setPreambleStoragePath). If the option is named generally, other temporary files created by libclang could be identified in the future and placed in the same directory without changing the API.
>> 3. 1 and 2 - the options can be propagated between identical end points together, so this combination is natural.
>> 4. The current patch. This is the easiest (already implemented) and most reliable (the temporary directory is definitely and completely overridden) approach. But there are thread safety concerns due to the introduction of global state.
>>
>> If the 4th option is unacceptable, I lean towards the option 3a (1 and 2a), because the amount of implementation work should not be much greater than 1 alone or 2a alone. If the 4th option is unacceptable, I suppose a separate review request based on the current LLVM main branch should be created and this one closed, right?
>
> 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...

> So my preference is for components to have an option to pick a different location as part of their API layer, rather than at the lower level. Based on that, I'm definitely in support of #1, and I'm in support of one of the options for #2. I lean towards #2b over #2a due to wanting individual overrides rather than a blanket override (e.g., we should be able to put preamble files in a different location than we put, say, crash logs).
>
> @dblaikie does that sound reasonable to you?

(1) seems OK-ish, I guess there's some question as to what the tradeoff is for that option - does that blow out memory usage of the client/kdevelop? But I guess it's probably fine.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774