[Lldb-commits] [PATCH] D76471: Remap the target SDK directory to the host SDK directory

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D76471#1947250 <https://reviews.llvm.org/D76471#1947250>, @aprantl wrote:

> I've reworked this a little based on your feedback.
>
> First, I've renamed `SDK` to `XcodeSDK`. An Xcode SDK is a fairly specific concept and I'm not going to pretend that it makes sense to generalize it for something else, so I thought the name should reflect this. I've kept it in Utility though, since the functionality mirrors `ArchSpec` and one platform typically hosts many SDKs at the same time. I entertained the idea of creating a base class and each platform would implement its own SDK, which sounds neat, but with all the merging functionality needed, this doesn't really work out.


Thanks.

That sounds fine. I don't have a problem with a class like this residing in Utility. I also don't think it's time to try to generalize this just yet, as its very likely that the generalized concept would not fit what some other platform wants to do. I just think some of the apis should reflect the specificity of this more. Some inline comments reflect that.

> I did incorporate the suggestion of requesting a platform (because I do think it should be Platform or Host that is calling `xcrun`) via the Module's ArchSpec.

That sounds fine to me, though I'd like to hear @jingham's take on Platform in this way. The part that I'm not super-clear on is the `type` argument to the `GetSDKPath` function. I mean, if I look at the `XcodeSDK::Type` enum, then the list pretty much mirrors the list if Platform classes we have. So asking a `PlatformRemoteAppleWatch` for an "AppleTV" sdk sounds nonsensical, and one gets the impression that this information should be already encoded in the selection of the platform object. It probably already is, via the ArchSpec argument, no?

In case of Apple platforms, this won't make a difference in practice, since the support for that is implemented in PlatformDarwin (which all of these inherit from), but it sounds like this will be a problem for the "linux" sdk (assuming this is what I think it is), as there the selected platform will be PlatformLinux, which has no clue about these sdk thingies.

So, it sounds to me like the sdk type should somehow play a role in the selection of the platform instance, or this functionality should be moved down into some Host function.

> Finally I added unit tests for all of the SDK functionality.
> 
> I have no great idea for how to test the remapping functionality other than completely end-to-end, because its primary effect is showing when debugging the OS itself (you need to resolve files *inside* the SDK).

I guess this is down to our current inability to "mock" `xcrun` responses (?) It might be possible to do something about that in a unit test with a custom platform plugin, but it seems that the amount of untested functionality here is not very big (Module::RegisterSDK, basically), so we could let that slide.



================
Comment at: lldb/include/lldb/Core/Module.h:517
+  /// \param sysroot will be added to the path remapping dictionary.
+  void RegisterSDK(llvm::StringRef sdk, llvm::StringRef sysroot);
+
----------------
RegisterXCodeSDK?


================
Comment at: lldb/include/lldb/Core/Module.h:983
+  /// The SDK this module was compiled with.
+  XcodeSDK m_sdk;
+  
----------------
m_xcode_sdk


================
Comment at: lldb/include/lldb/Target/Platform.h:437
 
+  virtual ConstString GetSDKPath(int type) { return {}; }
+
----------------
GetXCodeSDKPath? (and maybe the argument can be the proper enum type now.


================
Comment at: lldb/include/lldb/Utility/XcodeSDK.h:24
+  XcodeSDK() = default;
+  XcodeSDK(std::string &&name) : m_name(name) {}
+
----------------
the fancy `&&` is useless here without `std::move` (and it's not even very useful with it).


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

https://reviews.llvm.org/D76471