[PATCH] D82362: Move default module cache from system temporary directory

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Reverted for now in 4d5c4489435dc1cb3d4989614e96b157c74afdea 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82362



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82362: Move default module cache from system temporary directory

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Looks like this breaks tests on Mac: http://45.33.8.238/mac/16222/step_11.txt

Please take a look and revert for now if this takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82362



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82362: Move default module cache from system temporary directory

[David Zarzycki via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbb26838ceffb: [clang driver] Move default module cache from 
system temporary directory (authored by davezarzycki).

Changed prior to commit:
  https://reviews.llvm.org/D82362?vs=272655=273674#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82362

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/modules-cache-path.m
  clang/unittests/Driver/ModuleCacheTest.cpp
  llvm/lib/Support/Unix/Path.inc

Index: llvm/lib/Support/Unix/Path.inc
===
--- llvm/lib/Support/Unix/Path.inc
+++ llvm/lib/Support/Unix/Path.inc
@@ -1133,19 +1133,6 @@
   return true;
 }
 
-bool cache_directory(SmallVectorImpl ) {
-  if (const char *RequestedDir = getenv("XDG_CACHE_HOME")) {
-result.clear();
-result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
-return true;
-  }
-  if (!home_directory(result)) {
-return false;
-  }
-  append(result, ".cache");
-  return true;
-}
-
 static bool getDarwinConfDir(bool TempDir, SmallVectorImpl ) {
   #if defined(_CS_DARWIN_USER_TEMP_DIR) && defined(_CS_DARWIN_USER_CACHE_DIR)
   // On Darwin, use DARWIN_USER_TEMP_DIR or DARWIN_USER_CACHE_DIR.
@@ -1171,6 +1158,27 @@
   return false;
 }
 
+bool cache_directory(SmallVectorImpl ) {
+#ifdef __APPLE__
+  if (getDarwinConfDir(false/*tempDir*/, result)) {
+return true;
+  }
+#else
+  // XDG_CACHE_HOME as defined in the XDG Base Directory Specification:
+  // http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
+  if (const char *RequestedDir = getenv("XDG_CACHE_HOME")) {
+result.clear();
+result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
+return true;
+  }
+#endif
+  if (!home_directory(result)) {
+return false;
+  }
+  append(result, ".cache");
+  return true;
+}
+
 static const char *getEnvTempDir() {
   // Check whether the temporary directory is specified by an environment
   // variable.
Index: clang/unittests/Driver/ModuleCacheTest.cpp
===
--- clang/unittests/Driver/ModuleCacheTest.cpp
+++ clang/unittests/Driver/ModuleCacheTest.cpp
@@ -21,7 +21,7 @@
   SmallString<128> Buf;
   Driver::getDefaultModuleCachePath(Buf);
   StringRef Path = Buf;
-  EXPECT_TRUE(Path.find("org.llvm.clang") != Path.npos);
+  EXPECT_TRUE(Path.find("clang") != Path.npos);
   EXPECT_TRUE(Path.endswith("ModuleCache"));  
 }
 } // end anonymous namespace.
Index: clang/test/Driver/modules-cache-path.m
===
--- clang/test/Driver/modules-cache-path.m
+++ clang/test/Driver/modules-cache-path.m
@@ -1,5 +1,2 @@
-// RUN: env USERNAME=asdf LOGNAME=asdf %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=CHECK-SET
-// CHECK-SET: -fmodules-cache-path={{.*}}org.llvm.clang.asdf{{[/\\]+}}ModuleCache
-
 // RUN: %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=CHECK-DEFAULT
-// CHECK-DEFAULT: -fmodules-cache-path={{.*}}org.llvm.clang.{{[A-Za-z0-9_]*[/\\]+}}ModuleCache
+// CHECK-DEFAULT: -fmodules-cache-path={{.*}}clang{{[/\\]+}}ModuleCache
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -721,38 +721,6 @@
   }
 }
 
-static void appendUserToPath(SmallVectorImpl ) {
-#ifdef LLVM_ON_UNIX
-  const char *Username = getenv("LOGNAME");
-#else
-  const char *Username = getenv("USERNAME");
-#endif
-  if (Username) {
-// Validate that LoginName can be used in a path, and get its length.
-size_t Len = 0;
-for (const char *P = Username; *P; ++P, ++Len) {
-  if (!clang::isAlphanumeric(*P) && *P != '_') {
-Username = nullptr;
-break;
-  }
-}
-
-if (Username && Len > 0) {
-  Result.append(Username, Username + Len);
-  return;
-}
-  }
-
-// Fallback to user id.
-#ifdef LLVM_ON_UNIX
-  std::string UID = llvm::utostr(getuid());
-#else
-  // FIXME: Windows seems to have an 'SID' that might work.
-  std::string UID = "";
-#endif
-  Result.append(UID.begin(), UID.end());
-}
-
 static void addPGOAndCoverageFlags(const ToolChain , Compilation ,
const Driver , const InputInfo ,
const ArgList ,
@@ -3201,11 +3169,13 @@
 CmdArgs.push_back("-fno-math-builtin");
 }
 
-void Driver::getDefaultModuleCachePath(SmallVectorImpl ) {
-  llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/false, Result);
-  llvm::sys::path::append(Result, "org.llvm.clang.");
-  appendUserToPath(Result);
-  llvm::sys::path::append(Result, "ModuleCache");
+bool Driver::getDefaultModuleCachePath(SmallVectorImpl ) {
+  if 

[PATCH] D82362: Move default module cache from system temporary directory

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3253
+  CmdArgs.push_back(Args.MakeArgString(Path));
+}
   }

davezarzycki wrote:
> sammccall wrote:
> > what happens in the else case?
> No module caching.
This seems reasonable (though a bit surprising that it happens silently). 
Please add a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82362



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82362: Move default module cache from system temporary directory

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82362



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82362: Move default module cache from system temporary directory

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki marked an inline comment as done.
davezarzycki added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3253
+  CmdArgs.push_back(Args.MakeArgString(Path));
+}
   }

sammccall wrote:
> what happens in the else case?
No module caching.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82362



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82362: Move default module cache from system temporary directory

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Can you update the commit message to also reflect the change in 
`cache_directory` behavior? (This is going to move clangd index files on macOS, 
for instance... I think that's OK, but let's not bury it)




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3253
+  CmdArgs.push_back(Args.MakeArgString(Path));
+}
   }

what happens in the else case?



Comment at: llvm/lib/Support/Unix/Path.inc:1163
+#ifdef __APPLE__
+  if (getDarwinConfDir(false/*tempDir*/, result)) {
+return true;

nit: `/*TempDir=*/false` is conventional and e.g. clang-tidy will verify it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82362



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82362: Move default module cache from system temporary directory

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki created this revision.
davezarzycki added reviewers: compnerd, aprantl, jakehehrlich, espindola, 
respindola, ilya-biryukov, pcc, sammccall.
davezarzycki added projects: clang, LLVM.
Herald added a subscriber: hiraditya.

1. Shared writable directories like /tmp are a security problem.
2. Systems provide dedicated cache directories these days anyway.

As a followup, I hope to deprecate LLVM's 
`llvm::sys::path::system_temp_directory()`. See: https://reviews.llvm.org/D82259


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82362

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Driver.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/modules-cache-path.m
  clang/unittests/Driver/ModuleCacheTest.cpp
  llvm/lib/Support/Unix/Path.inc

Index: llvm/lib/Support/Unix/Path.inc
===
--- llvm/lib/Support/Unix/Path.inc
+++ llvm/lib/Support/Unix/Path.inc
@@ -1133,19 +1133,6 @@
   return true;
 }
 
-bool cache_directory(SmallVectorImpl ) {
-  if (const char *RequestedDir = getenv("XDG_CACHE_HOME")) {
-result.clear();
-result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
-return true;
-  }
-  if (!home_directory(result)) {
-return false;
-  }
-  append(result, ".cache");
-  return true;
-}
-
 static bool getDarwinConfDir(bool TempDir, SmallVectorImpl ) {
   #if defined(_CS_DARWIN_USER_TEMP_DIR) && defined(_CS_DARWIN_USER_CACHE_DIR)
   // On Darwin, use DARWIN_USER_TEMP_DIR or DARWIN_USER_CACHE_DIR.
@@ -1171,6 +1158,27 @@
   return false;
 }
 
+bool cache_directory(SmallVectorImpl ) {
+#ifdef __APPLE__
+  if (getDarwinConfDir(false/*tempDir*/, result)) {
+return true;
+  }
+#else
+  // XDG_CACHE_HOME as defined in the XDG Base Directory Specification:
+  // http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
+  if (const char *RequestedDir = getenv("XDG_CACHE_HOME")) {
+result.clear();
+result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
+return true;
+  }
+#endif
+  if (!home_directory(result)) {
+return false;
+  }
+  append(result, ".cache");
+  return true;
+}
+
 static const char *getEnvTempDir() {
   // Check whether the temporary directory is specified by an environment
   // variable.
Index: clang/unittests/Driver/ModuleCacheTest.cpp
===
--- clang/unittests/Driver/ModuleCacheTest.cpp
+++ clang/unittests/Driver/ModuleCacheTest.cpp
@@ -21,7 +21,7 @@
   SmallString<128> Buf;
   Driver::getDefaultModuleCachePath(Buf);
   StringRef Path = Buf;
-  EXPECT_TRUE(Path.find("org.llvm.clang") != Path.npos);
+  EXPECT_TRUE(Path.find("clang") != Path.npos);
   EXPECT_TRUE(Path.endswith("ModuleCache"));  
 }
 } // end anonymous namespace.
Index: clang/test/Driver/modules-cache-path.m
===
--- clang/test/Driver/modules-cache-path.m
+++ clang/test/Driver/modules-cache-path.m
@@ -1,5 +1,2 @@
-// RUN: env USERNAME=asdf LOGNAME=asdf %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=CHECK-SET
-// CHECK-SET: -fmodules-cache-path={{.*}}org.llvm.clang.asdf{{[/\\]+}}ModuleCache
-
 // RUN: %clang -fmodules -### %s 2>&1 | FileCheck %s -check-prefix=CHECK-DEFAULT
-// CHECK-DEFAULT: -fmodules-cache-path={{.*}}org.llvm.clang.{{[A-Za-z0-9_]*[/\\]+}}ModuleCache
+// CHECK-DEFAULT: -fmodules-cache-path={{.*}}clang{{[/\\]+}}ModuleCache
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -733,38 +733,6 @@
   }
 }
 
-static void appendUserToPath(SmallVectorImpl ) {
-#ifdef LLVM_ON_UNIX
-  const char *Username = getenv("LOGNAME");
-#else
-  const char *Username = getenv("USERNAME");
-#endif
-  if (Username) {
-// Validate that LoginName can be used in a path, and get its length.
-size_t Len = 0;
-for (const char *P = Username; *P; ++P, ++Len) {
-  if (!clang::isAlphanumeric(*P) && *P != '_') {
-Username = nullptr;
-break;
-  }
-}
-
-if (Username && Len > 0) {
-  Result.append(Username, Username + Len);
-  return;
-}
-  }
-
-// Fallback to user id.
-#ifdef LLVM_ON_UNIX
-  std::string UID = llvm::utostr(getuid());
-#else
-  // FIXME: Windows seems to have an 'SID' that might work.
-  std::string UID = "";
-#endif
-  Result.append(UID.begin(), UID.end());
-}
-
 static void addPGOAndCoverageFlags(const ToolChain , Compilation ,
const Driver , const InputInfo ,
const ArgList ,
@@ -3203,11 +3171,13 @@
 CmdArgs.push_back("-fno-math-builtin");
 }
 
-void Driver::getDefaultModuleCachePath(SmallVectorImpl ) {
-  llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/false, Result);
-  llvm::sys::path::append(Result, "org.llvm.clang.");
-