[Lldb-commits] [PATCH] D84537: [lldb/AppleSimulator] Always provide a -simulator environment

[Frederic Riss via Phabricator via lldb-commits]

friss marked 4 inline comments as done.
friss added inline comments.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp:270
+    static const ArchSpec platform_arch(
+        HostInfo::GetArchitecture(HostInfo::eArchKind64));
+    arch = platform_arch;
----------------
aprantl wrote:
> I guess I should really know this... does this construct create a static initializer? If yes, isn't that something we generally want to avoid in LLVM?
> 
> I checked: HostInfo::GetArchitecture() caches the result anyway, so there is no need to cache it again here.
static inside of a function will not create a global initializer, it will ensure that the variable is initialized once when the function is first executed. I can remove the caching though, this was cargo-culted.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h:59
+  llvm::Triple::OSType m_os_type = llvm::Triple::UnknownOS;
+  llvm::ArrayRef<llvm::StringRef> m_supported_triples = {};
+
----------------
aprantl wrote:
> If the StringRef is supposed to be a triple, we might want to store an array of llvm::Triple instead? They are basically std::strings.
I was going for constant expressions given the `StringRef` constructor is `constexpr`. The `Triple` constructor is not.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformAppleTVSimulator.cpp:159
+  static const llvm::StringRef supported_triples[] = {
+      "x86_64-apple-tvos-simulator",
+  };
----------------
aprantl wrote:
> I'm probably missing something: Instead of declaring the static list of supported triples here and then manually adding the host architecture, why not create the list on-the-fly in GetSupportedArchitectureAtIndex()? Is `m_supported_triples` used directly somewhere?
> 
> Something like:
> 
> ```
> switch (i) {
> #ifdef __APPLE__
> #if __arm64__
> case 0:  return  "arm64-apple-tvos-simulator";
> case 1:  return  "x86_64-apple-tvos-simulator";
> case 2: return "x86_64h-apple-tvos-simulator",
>   };
> #else
> if (is_haswell)
> switch (i) {
> case 0: return "x86_64h-apple-tvos-simulator";
> case 1: return "x86_64-apple-tvos-simulator";
> }
> else  return "x86_64-apple-tvos-simulator"
> ```
> 
> 
Again, I was going for constant expressions that can be indexed directly into, while this requires executing code. Spelling that argument out makes it sound really silly :-) Still I prefer a hardcoded list that any logic if we can get by with it. Maybe we just put all the variants there? If the binary cannot be executed, then the kernel will complain. Yeah, I think I'll do that.


================
Comment at: lldb/unittests/Platform/PlatformAppleSimulatorTest.cpp:72
+  }
+}
+
----------------
aprantl wrote:
> Does this explicitly test that platform[0] == HostInfo::GetArchitecture()?
No. It tests the second `::Create` method. This test would fail on Apple Silicon without the `case llvm::Triple::aarch64` I added in there. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84537