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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
