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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
