[PATCH] D77696: [Darwin] Teach `AtosSymbolizerProcess` to work on a copy of the environment.

[Julian Lettner via Phabricator via llvm-commits]

yln accepted this revision.
yln added a comment.
This revision is now accepted and ready to land.

LGTM with nits



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:53
 
+static const uptr kMaxNumEnvPEntries = 512;
+
----------------
Can be moved inside private section of class: it's a private implementation detail of the class.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:60
     pid_str_[0] = '\0';
+    internal_memset(custom_env_, 0, sizeof(custom_env_));
   }
----------------
I wouldn't do this (initialize members).  My argument is *not* performance or that it is unnecessary, but rather that it makes it seem as if `AtosSymbolizerProcess` is fully usable after being constructed.  Good C++ objects are fully initialized/usable after construction.  `AtosSymbolizerProcess` is not like that. It's very special: you have to call its methods in a specific order for it to work.  Stray of that path and it won't work correctly.  And we can't fix that, not even by initializing its members with good (empty) defaults.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:107
+    char **current_env = SymbolizerProcess::GetEnvP();
+    CopyEnv(current_env);
+    return custom_env_;
----------------
Should this also be setup in `StartSymbolizerSubprocess` for consistency?  Or the other way around: should `pid_str_` be populated in `GetArgV`?  But then we would probably introduce an unwanted order dependency between `GetEnvP` and `GetArgV`.  (These are just questions, not suggestions.)


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/Darwin/symbolizer-large-env.cpp:1
+// RUN: rm -rf %t %t.dSYM
+// RUN: %clangxx %s -g -O0 -o %t
----------------
I think this is unnecessary


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/Darwin/symbolizer-large-env.cpp:22
+
+int main() {
+  size_t current_entries = get_num_env_entries();
----------------
I think the most important aspect to test would be to check that the started symbolizer process has all the env vars that we would expect it to have.  I thought about how to test this but don't have a good idea, so I am happy to move forward without it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77696