[Lldb-commits] [PATCH] D122684: [lldb] Use the selected and host platform to disambiguate between fat binary architectures

[Jonas Devlieghere via Phabricator via lldb-commits]

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


================
Comment at: lldb/source/Target/Platform.cpp:1277-1279
+  // Prefer the selected platform if it matches all architectures.
+  if (selected_platform_matches_all_archs)
+    return selected_platform_sp;
----------------
clayborg wrote:
> Shouldn't we just check if the selected platform matches one architecture here and return the selected platform? Why do all architectures need to match. If I select a platform, and then do "file a.out" and "a.out" contained any number of architectures, it should just use the selected platform, no?
That's a good question. I did this to mimic what we're doing today, i.e. preferring the platform that matches all architectures in the fat binary. 

I was trying to imagine what that could look like. One possible scenario is a fat binary with triples `arm64-apple-macosx` and `arm64-apple-ios`. In this case, both the host platform supports `arm64-apple-macosx` and `arm64-apple-ios` (https://support.apple.com/guide/app-store/iphone-ipad-apps-mac-apple-silicon-fird2c7092da/mac). And `remote-ios` supports `arm64-apple-ios`. If the binary is fat like that, it's more than likely meant to run on macosx, so with this algorithm, we'd do the right thing. 

The second reason is that the order of the architectures is pretty arbitrary. Let's consider the `arm64-apple-macosx` and `arm64-apple-ios` example again. If we say that we'll pick whichever matches first, then we'll pick the host platform. But if the order was `arm64-apple-ios` and  `arm64-apple-macosx` then we'll pick `remote-ios`. 

Anyway I'm not married to this approach. I personally think that picking the selected platform is one host matches makes sense. I'm less sure about the host platform. 


================
Comment at: lldb/source/Target/Platform.cpp:1281-1283
+  // Prefer the host platform if it matches all architectures.
+  if (host_platform_matches_all_archs)
+    return host_platform_sp;
----------------
clayborg wrote:
> Again, why would we not just return the host platform if one arch matches? same kind of thing as above?
See my reply above.


================
Comment at: lldb/source/Target/Platform.cpp:1285-1286
+
+  // If there's only one platform left then that means that means that it
+  // supports all architectures.
+  if (candidates.size() == archs.size()) {
----------------
clayborg wrote:
> Is this comment out of date? We are iterating through more than one platform here.
Yup, I was uniquing the platforms originally.


================
Comment at: lldb/source/Target/Platform.cpp:1287-1294
+  if (candidates.size() == archs.size()) {
+    if (std::all_of(candidates.begin(), candidates.end(),
+                    [&](const PlatformSP &p) -> bool {
+                      return p->GetName() == candidates.front()->GetName();
+                    })) {
+      return candidates.front();
+    }
----------------
clayborg wrote:
> What is this doing? Comparing the first entry in candidates to each of the entries and if the name matches, returns the first entry? When would this ever not return true?
This checks that all the platforms are identical. If they're all identical then that means that we have one platform that works for all architectures. This covers case (3) from the summary.


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

https://reviews.llvm.org/D122684