[PATCH] D86170: PrintStackTrace: don't symbolize if LLVM_DISABLE_SYMBOLIZATION is set

Fri Aug 21 08:29:58 PDT 2020

MaskRay added inline comments.

================
Comment at: llvm/utils/not/not.cpp:37-43
+#ifdef _WIN32
+    SetEnvironmentVariableA("LLVM_DISABLE_CRASH_REPORT", "1");
+    SetEnvironmentVariableA("LLVM_DISABLE_SYMBOLIZATION", "1");
+#else
+    setenv("LLVM_DISABLE_CRASH_REPORT", "1", 0);
+    setenv("LLVM_DISABLE_SYMBOLIZATION", "1", 0);
+#endif
----------------
aganea wrote:
> dblaikie wrote:
> > MaskRay wrote:
> > > dblaikie wrote:
> > > > Rather than adding non-portable code, any chance of passing this environment variable into the child process creation in ExecuteAndWait?
> > > That requires to construct a vector<StringRef>. The complexity may not be lower.
> > Yeah, I'm not expecting it to be less code - but more portable/keeping the portability layers down in the System library in one place, etc.
> +1 with David said.
I checked about 16 uses of ExecuteAndWait - there is no instance passing an envp array. Moreover, I cannot find an API returning the current set environment variables. GetEnv can return the value of one environment variable but that is apparently insufficient.

In addition, dispatching on `_WIN32` already exists in this file. I'd argue that `setenv` is more straightforward as the intention is very clearly expressed in the 2 lines of setenv. The `#ifdef _WIN32` branch, even if the user is not familiar with it, the user can easily infer its intention as well. Getting the environment variable array and adding two more may be slightly less obvious, even if it does not use `#ifdef`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86170