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