[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:102
+      // `atos_mach_port_env_var_entry_` variable with our current PID.
+      UpdateAtosEnvVar(pid_str_);
+    }
----------------
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 `=`...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78179