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

[Pavel Labath via Phabricator via lldb-commits]

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