Naive usage of shared temporary directories in Unix is a classic security problem. Let's deprecate two APIs that enable unsafe code and encourage people to use createUniqueFile or createUniqueDirectory instead.
This is doing two things - changing the behavior of clang (where it stores its module cache) and deprecating an API. I'd recommend we start by focusing on whether the first part is the right thing to do, and just remove the API if this is the only client of it.
|3177 ↗||(On Diff #272260)|
This default path is not ideal for Linux. On Linux, if we want to put this into the home directory, we should follow XDG and put this into the ~/.cache/org.llvm.clang/ModuleCache, respecting the environment variable XDG_CACHE_DIR.
Sure, we can honor XDG_CACHE_DIR. Maybe as a followup, somebody can wire up Darwin's cache directory (which is retrievable via a BSD specific confstr() API with _CS_DARWIN_USER_CACHE_DIR). I'm not sure about other platforms.
I'll wait for more feedback before updating the patch.
llvm::sys::path::cache_directory exists, it doesn't do anything special on Darwin (though would be the right place), but it does handle windows.
I'm not sure what the original motivation for org.llvm.clang.$USER format was, but it doesn't seem to be worth carrying over vs $CONFIG/clang/ to me - clang is well-known enough to "own" the name and most tools don't seem to add reverse-domains to these paths.
Thanks @davezarzycki! Minor nit: git-clang-format the patch please, there was at least one linter warning in the changed code.
I do wonder if we can get away with the removal of the API though doing that in a follow up is pretty reasonable to me.
I think that one minor change is needed for this - an entry in the release notes would be nice to indicate to users that the cache paths have changed. Its difficult to say if anyone is depending on it being in TMP, but it is a user visible change to the driver (though they still have control via -fmodules-cache-path).
|3176 ↗||(On Diff #272434)|
I would prefer to separate this out into its own patch. This is going to silently(!) break all sorts of scripts — for example many buildbots have a "clean module cache" action that searches for a directory with this name.