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

[Dan Liew via Phabricator via llvm-commits]

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


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:28
+#include "sanitizer_symbolizer_mac.h"
+
 namespace __sanitizer {
----------------
kubamracek wrote:
> Can you drop these non-essential changes, please?
Sorry clang-format changed this and I didn't notice.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:73
+      // variable is set to
+      CHECK_EQ(putenv(atos_mach_port_env_var_entry_), 0);
+    }
----------------
kubamracek wrote:
> Just for readability, please move the putenv call outside of the CHECK.
Sure.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:98
+      // `atos_mach_port_env_var_entry_` variable with our current PID.
+      const char *current_atos_env_var = getenv(kAtosEnvVar);
+      if (current_atos_env_var !=
----------------
kubamracek wrote:
> I'd rather not call getenv here too. We can just access `atos_mach_port_env_var_entry_` directly, no? Also, why not just unconditionally call updateAtosEnvVar?
The reason is that I wanted to check our assumption is that the `__check_mach_ports_lookup` environment variable is still in the environment **and** that we still control it.

This assumption could be broken by the user's code changing the environment.

If you don't care about detecting this case so that we can fail more gracefully then I can rip this out which also means I can rip out `getValueFromEnvEntry()` as well.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:115
 
+  void updateAtosEnvVar(const char *pid_str) {
+    uptr count = internal_snprintf(atos_mach_port_env_var_entry_,
----------------
kubamracek wrote:
> All methods here in this class start with uppercase letter. Let's keep it that way, please.
Oops I missed this. I'll fix that.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:258
 
+void AtosSymbolizer::LateInitialize() { process_->LateInitialize(); }
+
----------------
kubamracek wrote:
> Who's calling this?
This will be called via `Symbolizer::LateInitialize()`

Please see the patch (https://reviews.llvm.org/D78178) that this patch depends on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78179