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

Thu Sep 1 09:11:24 PDT 2022

mizvekov added a comment.

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

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

Okay, fair!

We will have an RFC about this.

================
Comment at: clang/lib/Driver/Driver.cpp:5427
+                                   ? A->getValue()
+                                   : std::getenv("CLANG_CRASH_DIAGNOSTICS_DIR");
+  if (CrashDirectory) {
----------------
aaron.ballman wrote:
> This should use `llvm::sys::Process::GetEnv()` so it properly handles text encodings. CC @tahonermann @cor3ntin for text encoding awareness.
Okay, makes sense. But be aware that we have multiple places in the llvm project where we are using std::getenv to retrieve a filesystem path variable. There is another one within clang itself as well (`CLANG_MODULE_CACHE_PATH`, which is where I took inspiration from).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133082