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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Target/Platform.h:103
+  static lldb::PlatformSP
+  GetPlatformForArchitectures(std::vector<ArchSpec> archs,
+                              const ArchSpec &process_host_arch,
----------------



================
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;
----------------
JDevlieghere wrote:
> 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. 
I agree, so maybe lets switch to say if the selected platform matches at least one, then it is all good. Will the selected platform ever be the host platform? 


================
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();
+    }
----------------
JDevlieghere wrote:
> 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.
Sounds good, just update the comment then and all good.


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

https://reviews.llvm.org/D122684