[Lldb-commits] [PATCH] D79364: Move the Xcode SDK path caching to HostInfo

Tue May 5 10:14:23 PDT 2020

aprantl marked an inline comment as done.
aprantl added a comment.

In D79364#2019818 <https://reviews.llvm.org/D79364#2019818>, @labath wrote:

> Since the option of going to the "host platform" was discussed already and rejected, I think a new discussion is in order before going back to it. What are the exact circumstances where you did not get a PlatformDarwin object?

You're right. The gist is that the old scheme did not work for debugging iOS processes from a macOS host. The fundamental issue was that GetPlatformForArchitecture chooses the first out of a list of possible platforms, and I the first hit would be a PlatformRemoteGDBServer, which does not inherit from PlatformDarwin. Basically, inside of Module, we don't have enough information (we'd need a Target) to select a meaningful platform.

> Note that I am not opposed (and I never was) having the list of xcode sdk be provided by the "host". However, I don't think it's a good idea to route all of this through the host *platform*, for a couple of reasons:
> 
> - it's completely redundant (if I "inline" `GetHostPlatform()->GetSDKPath(sdk)` I get exactly `HostInfo::GetXcodeSDK(sdk)`)
> - it makes it appear like it does more things than it actually does (asking PlatformLinux for an sdk does nothing, and it's not even clear if it should have that api)
> - it removes the only usage of "platform" in module code (using platforms in modules is very questionable because it can break caching. In fact, the only reason this usage is not questionable is because the function always returns the exact same value it gets from the host).

Good point! The reason why I went with `Platform::GetHostPlatform()` over `HostInfo::GetXcodeSDK(sdk))` was because I hadn't realized that I already put a default implementation of GetXcodeSDK into HostInfoBase, and I wanted to avoid guarding this in an ugly `#if APPLE`. But I think that that is completely unnecessary.

I'll change it to calling HostInfo directly!

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

https://reviews.llvm.org/D79364