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

[Aaron Ballman via Phabricator via libcxx-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:5427
+                                   ? A->getValue()
+                                   : std::getenv("CLANG_CRASH_DIAGNOSTICS_DIR");
+  if (CrashDirectory) {
----------------
mizvekov wrote:
> 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).
Yeah, I noticed the same thing when I was looking around. Every one of those is a bug! It likely works fine for Linux where UTF-8 is common, but they will all fail on Windows where UTF-16 is used for the OS but `::getenv()` uses the C locale. Any sort of non-ASCII characters in the file path are likely not going to do the right thing in that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133082