[PATCH] D78179: [Darwin] Fix symbolization for recent simulator runtimes.

[Julian Lettner via Phabricator via llvm-commits]

yln added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:68
+  void LateInitialize() {
+    if (SANITIZER_IOSSIM) {
+      // `putenv()` may call malloc/realloc so it is only safe to do this
----------------
What's our stance on `if(<defined-value>)` vs `#ifdef`?  In compiler-rt we seem to usually use #ifdef.  Should we aim for consistency here? 


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:69
+    if (SANITIZER_IOSSIM) {
+      // `putenv()` may call malloc/realloc so it is only safe to do this
+      // during LateInitialize() or later (i.e. we can't do this in the
----------------
Thanks for the explanation of why this is needed! :)


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:102
+      // `atos_mach_port_env_var_entry_` variable with our current PID.
+      UpdateAtosEnvVar(pid_str_);
+    }
----------------
If you remove the unnecessary call in the constructor, then this is the only usage of the function and we can inline it.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:138
   char pid_str_[16];
+  static const char kAtosEnvVar_[sizeof(K_ATOS_ENV_VAR)];
+  // Space for `\0` in `kAtosEnvVar_` is reused for `=`.
----------------
I think you can get away without the defining `kAtosEnvVar_` at all:
Here:
`char atos_mach_port_env_var_entry_[sizeof(K_ATOS_ENV_VAR) + sizeof(pid_str_)];`
Above: make it part of the format string:
`internal_snprintf(.., .., K_ATOS_ENV_VAR "=%s", pid_str);`




================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.h:38
   bool SymbolizeData(uptr addr, DataInfo *info) override;
+  void LateInitialize() override;
 
----------------
delcypher wrote:
> kubamracek wrote:
> > I don't see a virtual LateInitialize in SymbolizerTool... is this supposed to not be an "override"?
> Please see the patch that this patch depends on: https://reviews.llvm.org/D78178
Just a quick question: will the principled resolution (where we modify the env only for posix_spawn, but not in our own process) require this as well.  If not, is the plan to remove it again?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78179