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

[Dan Liew via Phabricator via llvm-commits]

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


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:53
 }
+static const char kAtosEnvVar[] = "__check_mach_ports_lookup";
 
----------------
delcypher wrote:
> kubamracek wrote:
> > Can you sink this into the class please?
> If I try to sink this into the class then this doesn't compile.
> 
> ```
> /Volumes/data/dev/llvm/monorepo_upstream/master/llvm/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:152:21: error: in-class initializer for static data member of type 'const char [26]' requires 'constexpr' specifier
>   static const char kAtosEnvVar[] = "__check_mach_ports_lookup";
>                     ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   constexpr 
> ```
> 
> If I make it constexpr then we'll get linking error because we need the memory to exist at runtime.
> 
> ```
> Undefined symbols for architecture i386:
>   "__sanitizer::AtosSymbolizerProcess::kAtosEnvVar", referenced from:
>       __sanitizer::AtosSymbolizer::AtosSymbolizer(char const*, __sanitizer::LowLevelAllocator*) in sanitizer_symbolizer_mac.cpp.o
>       __sanitizer::AtosSymbolizer::AtosSymbolizer(char const*, __sanitizer::LowLevelAllocator*) in sanitizer_symbolizer_mac.cpp.o
>       __sanitizer::AtosSymbolizerProcess::StartSymbolizerSubprocess() in sanitizer_symbolizer_mac.cpp.o
> ```
> 
> To fix this I'd have initialize the static data member outside the class which means I'd have to do...
> 
> ```
> // In class
> static const char kAtosEnvVar[26];
> 
> // Outside of class
> const char AtosSymbolizerProcess::kAtosEnvVar[] = "__check_mach_ports_lookup";
> ```
> 
> This seems clumsy to me because I have to hardcode the `26` size.
Do you have a preference between the global var and the clumsy static data member?


================
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 !=
----------------
delcypher wrote:
> 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.
Are you have for me to drop the check of the assumption?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78179