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