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

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

kubamracek added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:28
+#include "sanitizer_symbolizer_mac.h"
+
 namespace __sanitizer {
----------------
Can you drop these non-essential changes, please?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:53
 }
+static const char kAtosEnvVar[] = "__check_mach_ports_lookup";
 
----------------
Can you sink this into the class please?


================
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);
+    }
----------------
Just for readability, please move the putenv call outside of the CHECK.


================
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 !=
----------------
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?


================
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_,
----------------
All methods here in this class start with uppercase letter. Let's keep it that way, please.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:258
 
+void AtosSymbolizer::LateInitialize() { process_->LateInitialize(); }
+
----------------
Who's calling this?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.h:38
   bool SymbolizeData(uptr addr, DataInfo *info) override;
+  void LateInitialize() override;
 
----------------
I don't see a virtual LateInitialize in SymbolizerTool... is this supposed to not be an "override"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78179