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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

I don't see any measureable improvement in terms of execution time on a 36-core Windows with `ninja check-llvm` Release with assertions (164 sec both with & without the patch). However there's a measurable saving on a 6-core: 820 sec without patch -> 784 sec with patch, median times after a few runs.



================
Comment at: llvm/test/tools/not/disable-symbolization.test:1
+# RUN: not --crash env > %t || true
+# RUN: FileCheck %s < %t
----------------
dblaikie wrote:
> MaskRay wrote:
> > aganea wrote:
> > > This test also fails on Windows - I think lit doesn't like `env` alone.
> > > 
> > > ```
> > > ******************** TEST 'LLVM :: tools/not/disable-symbolization.test' FAILED ********************
> > > Script:
> > > --
> > > : 'RUN: at line 1';   not --crash env > D:\llvm-project\buildninjaRel\test\tools\not\Output\disable-symbolization.test.tmp || true
> > > : 'RUN: at line 2';   d:\llvm-project\buildninjarel\bin\filecheck.exe D:\llvm-project\llvm\test\tools\not\disable-symbolization.test < D:\llvm-project\buildninjaRel\test\tools\not\Output\disable-symbolization.test.tmp
> > > --
> > > Exit Code: 127
> > > 
> > > Command Output (stdout):
> > > --
> > > $ ":" "RUN: at line 1"
> > > $ "not" "--crash" "env"
> > > # command stderr:
> > > Error: 'env' requires a subcommand
> > > error: command failed with exit status: 127
> > > 
> > > --
> > > ```
> > > 
> > > `printenv` instead?
> > Thanks! `printenv` is not specified by POSIX, so I did not use it. But I just checked: `printenv` appeared in 3.0BSD & Solaris has it as well, so the portability is good.
> Any ideas if this'll work on Windows?
@dblaikie It does work, since a Unix-like shell is required anyway to run the lit tests on Windows, either MinGW, CygWin or GnuWin32 on the top of the regular cmd shell.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86170