[libcxx-commits] [PATCH] D133082: [clang] Implement setting crash_diagnostics_dir through env variable

[Aaron Ballman via Phabricator via libcxx-commits]

aaron.ballman added subscribers: cor3ntin, tahonermann.
aaron.ballman added a comment.

In D133082#3763829 <https://reviews.llvm.org/D133082#3763829>, @erichkeane wrote:

> I don't mind the idea, but wonder if some level of RFC/project-wide decision should be made here?  WDYT @aaron.ballman ?

We use environment variables in several other places within the driver, but I am a bit skittish about adding new ones to correspond to feature flags as that seems to be somewhat novel. I think it'd be a good idea to get more buy-in with an RFC on Discourse for more broad awareness.

If we go this route, we definitely need user-facing documentation that explains what's going on. I don't think we have anything corresponding to https://gcc.gnu.org/onlinedocs/gcc/Environment-Variables.html, but having such a page for Clang would be a good idea (even if the page is incomplete).



================
Comment at: clang/docs/ReleaseNotes.rst:135-144
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 New Compiler Flags
 ------------------
+- It's now possible to set the crash diagnostics directory through
+  the environment variable 'CLANG_CRASH_DIAGNOSTICS_DIR'.
----------------



================
Comment at: clang/lib/Driver/Driver.cpp:5427
+                                   ? A->getValue()
+                                   : std::getenv("CLANG_CRASH_DIAGNOSTICS_DIR");
+  if (CrashDirectory) {
----------------
This should use `llvm::sys::Process::GetEnv()` so it properly handles text encodings. CC @tahonermann @cor3ntin for text encoding awareness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133082