labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.
I think this needs a lot more discussion. First, there's some weird layering
going on here, where the class SDK is declared in lldb/Utility, but it's
implemented in PlatformDarwin. But even before we sort that out, we probably
need to figure out what exactly is the scope of the new class (e.g. should it
cover only Mac sdks, or more).
Then there's the question of the usage of the host platform in the Module
class, which is not very ideal. Elsewhere, one would ask the target for it's
current platform and use that, but since Modules are idependent of a target,
that can't happen here. Still, I don't think that jumping to the host platform
is the right solution to that.
It also needs tests. The SDK class looks to be perfectly unit-testable, and if
we include the "module sdk" (or maybe the resultant path mappings) in its
"dump" output, then we could use `lldb-test` to test it's interaction with the
module and dwarf classes too.
================
Comment at: lldb/include/lldb/Utility/SDK.h:18-19
+
+/// An abstraction for Apple SDKs that works like \p llvm::Triple.
+class SDK {
+ ConstString m_name;
----------------
If this is going to be specific to apple sdks, then it should have a less
generic name (other platforms have sdks too).
================
Comment at: lldb/include/lldb/Utility/SDK.h:20
+class SDK {
+ ConstString m_name;
+
----------------
Are all of these ConstStrings really needed? The code uses StringRefs for
string manipulation anyway, and it's very doubtful that any of this is going to
be a performance bottleneck. OTOH, each ConstString adds some junk to the
global string pool which never gets deleted.
================
Comment at: lldb/source/Core/Module.cpp:1603
+ m_sdk.Merge(sdk);
+ PlatformSP host_platform = Platform::GetHostPlatform();
+ ConstString host_sdk_path = host_platform->GetSDKPath(sdk.GetSDKType());
----------------
Routing this via host platform seems like a bad design choice. Theoretically,
if I am cross-debugging a linux binary, I should be able to ask some entity
which knows about linux sysroots even if I am on a mac.
And indeed, we don't have that many callers of `Platform::GetHostPlatform`, and
none of them are in the Module hierarchy.
================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:1285-1358
+SDK &SDK::operator=(SDK other) {
+ m_name = other.m_name;
+ return *this;
+}
+
+bool SDK::operator==(SDK other) {
+ return m_name == other.m_name;
----------------
Implementing this inside PlatformDarwin.cpp is super weird.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:670-681
+SDK DWARFUnit::GetSDK() {
+ if (const DWARFDebugInfoEntry *die = GetUnitDIEPtrOnly())
+ return SDK(die->GetAttributeValueAsString(this, DW_AT_APPLE_sdk, ""));
+ return {};
+}
+
+llvm::StringRef DWARFUnit::GetSysroot() {
----------------
Unless we're planning to add these methods to llvm::DWARFUnit, they should go
someplace else. There's no reason why this functionality needs to be
implemented from within DWARFUnits.
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
