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

[Dan Liew via Phabricator via llvm-commits]

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


================
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_);
+    }
----------------
yln wrote:
> delcypher wrote:
> > delcypher wrote:
> > > yln wrote:
> > > > If you remove the unnecessary call in the constructor, then this is the only usage of the function and we can inline it.
> > > @yln 
> > > 
> > > > If you remove the unnecessary call in the constructor, then this is the only usage of the function and we can inline it.
> > > 
> > > The call in the constructor is **essential**. When you call `putenv()` Darwin's LibC checks the data is well formed so we **have to initialize `atos_mach_port_env_var_entry_` before giving it to `putenv()`**.
> > > 
> > > Later on we need to modify `atos_mach_port_env_var_entry_`  again to have it contain the right PID so it made sense to refactor this repeated action into `UpdateAtosEnvVar()`.
> > @yln Sorry a little bit more on this. The call to `UpdateAtosEnvVar()` is essential but it doesn't actually need to be in the constructor. We could move it into `LateInitialize()` to set `atos_mach_port_env_var_entry_` just before we give it to `putenv()`. Is that what you'd prefer?
> How about the following?
> 
> Initialize to default that is good enough to use for putenv():
> ```
> // Comment about '\0' and '='
> char AtosSymbolizerProcess::atos_mach_port_env_var_entry_[sizeof(K_ATOS_ENV_VAR) + sizeof(pid_str_)] = K_ATOS_ENV_VAR "=0";
> ```
> 
> Do nothing in the ctor (UpdateAtosEnvVar can be inlined).
> 
> UpdateAtosEnvVar() becomes:
> ```
> dst = &atos_mach_port_env_var_entry_[sizeof(K_ATOS_ENV_VAR)];
> strlcpy(dst, pid_str, ...);
> ```
> 
> Nit: use less verbose name for `atos_mach_port_env_var_entry_`
> 
> Double check the buffer sizes/string offsets/space for `=`...
I tried doing this with `constexpr` but I gave up as I would need to be able to concatenate strings and I'd have to implement a bunch of infrastructure to do that. So I went with your macro approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78179