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

[Dan Liew via Phabricator via llvm-commits]

delcypher marked 4 inline comments as done.
delcypher added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:53
 
+static const uptr kMaxNumEnvPEntries = 512;
+
----------------
yln wrote:
> Can be moved inside private section of class: it's a private implementation detail of the class.
Seems reasonable. I thought there was some issue with C++ wanting this to be `constexpr` when I tried this last time but it doesn't seem to be happening now so I'll make this change.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:60
     pid_str_[0] = '\0';
+    internal_memset(custom_env_, 0, sizeof(custom_env_));
   }
----------------
yln wrote:
> 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.
The point of this initialization is so that if methods are some how called in the wrong order we get deterministic behaviour. If we just leave garbage in this array and pass this to posix_spawn by accident then we'd likely get unpredictable crashes. Technically this initialization is unnecessary so if you feel very strongly about this then I'll remove it. Do you still want this removed?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:107
+    char **current_env = SymbolizerProcess::GetEnvP();
+    CopyEnv(current_env);
+    return custom_env_;
----------------
yln wrote:
> 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.)
Should **what** also be setup in `StartSymbolizerSubprocess`?

Also please bear in mind the subsequent patch (https://reviews.llvm.org/D77706) where the semantics of `CopyEnv` change to allocate space for a new environment variable.
Although looking at this..  all of this could actually be moved in `StartSymbolizerSubprocess` that would actually be way cleaner because we can keep the branching logic for `SANITIZER_IOSSIM` contained to a single method.

I'll refactor this.


================
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
----------------
yln wrote:
> I think this is unnecessary
This was originally here because I was hitting some weird issue where atos was reporting stale line locations. I'll remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77696