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

David Blaikie via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 11 15:03:42 PST 2023


dblaikie added a comment.

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

> In D139774#4043886 <https://reviews.llvm.org/D139774#4043886>, @vedgy wrote:
>
>> In D139774#4041308 <https://reviews.llvm.org/D139774#4041308>, @aaron.ballman wrote:
>>
>>> Is your concern essentially that someone will add a new use to put files in a temp directory and not have access to this information from ASTUnit? Or do you have other concerns in mind?
>>>
>>> We should certainly investigate whether there are other uses of temp files from libclang as part of these changes. It's possible we'll find a situation that makes my suggestion untenable because we don't have easy access to the information we need.
>>
>> The temporary directory could be used, now or in future, by some support code, which is used indirectly by libclang. I found the following uses potentially relevant to libclang:
>>
>> - `Driver::GetTemporaryDirectory(StringRef Prefix)` calls `llvm::sys::fs::createUniqueDirectory(Prefix, Path);`
>> - `StandardInstrumentations` => `DotCfgChangeReporter` calls `sys::fs::createUniquePath("cfgdot-%%%%%%.dot", SV, true);`
>> - There are plenty of `createTemporaryFile()` uses in llvm and clang. Some of them are likely used by libclang.
>>
>> I don't know how to efficiently check whether or not each of these indirect `system_temp_directory()` uses is in turn used by libclang. Even if they aren't know, they might be later, when libclang API grows.
>
> Oof, that is more exposure than I was thinking we had...
>
>> On a different note, do you think overriding temporary directory path is useful only to libclang and not likely to be useful to any other LLVM API users?
>
> I don't think there are any in-tree needs for that functionality, and the solution is fragile enough that I'd like to avoid it if possible (for example, it also makes the API much harder to use across threads because now it's accessing global state). That said, I think the libclang use case you have is compelling and worth solving in-tree if we can (so others don't have to find similar workarounds).
>
>>>> Another issue is with the `FileSystemOptions` part: this class is widely used in Clang. So adding a member to it could adversely affect performance of unrelated code.
>>>
>>> It's good to keep an eye on that, but I'm not too worried about the overhead from it (I don't see uses in AST nodes). We can keep an eye on https://llvm-compile-time-tracker.com/ to see if there is a surprising compile time increase though.
>>
>> [in case we pursue this design approach, which I currently don't feel is right] Why not add a temporary path data member into `class ASTUnit` under the `FileSystemOptions FileSystemOpts` member, not inside it? So that other code is not burdened with unused struct member, and to be able to use a more suitable type, like `SmallString` for the temporary path buffer.
>
> That's certainly an option, but the design of that would be a bit strange in that we have some file system options in one place and other file system options in another place. I think ultimately, we're going to want them all to live on `FileSystemOptions`. That said, I'm going to rope in some more folks for a wider selection of opinions on how/if to proceed. CC @dblaikie @MaskRay @d0k

Yeah, I think, at a quick glance, I'd lean towards FilesystemOptions too - it's mostly used as a member of FileManager, which already has a lot of members/fairly large footprint, so I wouldn't worry too much about this having a huge impact on the total memory usage/perf/etc.

I'm not sure how I feel about the general premise (though I don't have enough context/ownership to want to veto any direction/progress here, just thoughts in case they resonate):

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774



More information about the cfe-commits mailing list