[lldb] Don't create instance of SymbolFileDWARFDebugMap for non-Mach-O files

[royitaqi]

Change

SymbolFileDWARFDebugMap::CreateInstance() will return nullptr if the file is not a Mach-O.

Benefit

This may improve Linux debugger launch time by skipping the creation of SymbolFileDWARFDebugMap during the SymbolFile::FindPlugin() call, which loops through a list of SymbolFile plugins and tries to find the one that provides the best abilities. If the SymbolFileDWARFDebugMap is created during this loop, it will load the symbol table of the file in question and loop through all the compile units in the debug map (the OSO entries) to calculate the abilities.

Tests

Two unittests are added to verify the creation and the absence of SymbolFileDWARFDebugMap for Mach-O and non-Mach-O files, respectively.

Ran ninja check-lldb on both Darwin and Linux. No regression spotted. See results compared for mac and linux.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (royitaqi)

Changes

Change

SymbolFileDWARFDebugMap::CreateInstance() will return nullptr if the file is not a Mach-O.

Benefit

This may improve Linux debugger launch time by skipping the creation of SymbolFileDWARFDebugMap during the SymbolFile::FindPlugin() call, which loops through a list of SymbolFile plugins and tries to find the one that provides the best abilities. If the SymbolFileDWARFDebugMap is created during this loop, it will load the symbol table of the file in question and loop through all the compile units in the debug map (the OSO entries) to calculate the abilities.

Tests

Two tests are added to verify the creation and the absence for Mach-O and non-Mach-O files.

Ran ninja check-lldb on both Darwin and Linux. No regression spotted. See results compared for mac and linux.

---

Full diff: https://github.com/llvm/llvm-project/pull/139170.diff

3 Files Affected:

• (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp (+3)
• (modified) lldb/unittests/SymbolFile/DWARF/CMakeLists.txt (+1)
• (added) lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFDebugMapTests.cpp (+142)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
index 961c212e2e6dc..1793c23950d55 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
@@ -38,6 +38,7 @@
 #include "lldb/Target/StackFrame.h"
 
 #include "LogChannelDWARF.h"
+#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "SymbolFileDWARF.h"
 #include "lldb/lldb-private-enumerations.h"
 
@@ -246,6 +247,8 @@ llvm::StringRef SymbolFileDWARFDebugMap::GetPluginDescriptionStatic() {
 }
 
 SymbolFile *SymbolFileDWARFDebugMap::CreateInstance(ObjectFileSP objfile_sp) {
+  if (objfile_sp->GetPluginName() != ObjectFileMachO::GetPluginNameStatic())
+    return nullptr;
   return new SymbolFileDWARFDebugMap(std::move(objfile_sp));
 }
 
diff --git a/lldb/unittests/SymbolFile/DWARF/CMakeLists.txt b/lldb/unittests/SymbolFile/DWARF/CMakeLists.txt
index d5b0be7ea2a28..5aacb24fc5206 100644
--- a/lldb/unittests/SymbolFile/DWARF/CMakeLists.txt
+++ b/lldb/unittests/SymbolFile/DWARF/CMakeLists.txt
@@ -4,6 +4,7 @@ add_lldb_unittest(SymbolFileDWARFTests
   DWARFDIETest.cpp
   DWARFIndexCachingTest.cpp
   DWARFUnitTest.cpp
+  SymbolFileDWARFDebugMapTests.cpp
   SymbolFileDWARFTests.cpp
   XcodeSDKModuleTests.cpp
 
diff --git a/lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFDebugMapTests.cpp b/lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFDebugMapTests.cpp
new file mode 100644
index 0000000000000..8c65fca889a40
--- /dev/null
+++ b/lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFDebugMapTests.cpp
@@ -0,0 +1,142 @@
+//===-- SymbolFileDWARFDebugMapTests.cpp ----------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
+#include "Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "TestingSupport/TestUtilities.h"
+
+#include "lldb/Core/Module.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::plugin::dwarf;
+
+class SymbolFileDWARFDebugMapTests : public testing::Test {
+  SubsystemRAII<ObjectFileELF, ObjectFileMachO> subsystems;
+};
+
+TEST_F(SymbolFileDWARFDebugMapTests, CreateInstanceReturnNonNullForMachOFile) {
+  // Make sure we don't crash parsing a null unit DIE.
+  const char *yamldata = R"(
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x80000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      152
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         152
+    segname:         __TEXT
+    vmaddr:          0
+    vmsize:          4
+    fileoff:         184
+    filesize:        4
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        content:         'AABBCCDD'
+        size:            4
+        offset:          184
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+...
+)";
+
+  llvm::Expected<TestFile> file = TestFile::fromYaml(yamldata);
+  EXPECT_THAT_EXPECTED(file, llvm::Succeeded());
+  auto module_sp = std::make_shared<Module>(file->moduleSpec());
+  ASSERT_NE(module_sp, nullptr);
+  auto object_file = module_sp->GetObjectFile();
+  ASSERT_NE(object_file, nullptr);
+  auto debug_map =
+      SymbolFileDWARFDebugMap::CreateInstance(object_file->shared_from_this());
+  ASSERT_NE(debug_map, nullptr);
+}
+
+TEST_F(SymbolFileDWARFDebugMapTests, CreateInstanceReturnNullForNonMachOFile) {
+  // Make sure we don't crash parsing a null unit DIE.
+  const char *yamldata = R"(
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_386
+DWARF:
+  debug_abbrev:
+    - Table:
+        - Code:            0x00000001
+          Tag:             DW_TAG_compile_unit
+          Children:        DW_CHILDREN_no
+          Attributes:
+            - Attribute:       DW_AT_addr_base
+              Form:            DW_FORM_sec_offset
+  debug_info:
+    - Version:         5
+      AddrSize:        4
+      UnitType:        DW_UT_compile
+      Entries:
+        - AbbrCode:        0x00000001
+          Values:
+            - Value:           0x8 # Offset of the first Address past the header
+        - AbbrCode:        0x0
+  debug_addr:
+    - Version: 5
+      AddressSize: 4
+      Entries:
+        - Address: 0x1234
+        - Address: 0x5678
+  debug_line:
+    - Length:          42
+      Version:         2
+      PrologueLength:  36
+      MinInstLength:   1
+      DefaultIsStmt:   1
+      LineBase:        251
+      LineRange:       14
+      OpcodeBase:      13
+      StandardOpcodeLengths: [ 0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 ]
+      IncludeDirs:
+        - '/tmp'
+      Files:
+        - Name:            main.cpp
+          DirIdx:          1
+          ModTime:         0
+          Length:          0
+)";
+
+  llvm::Expected<TestFile> file = TestFile::fromYaml(yamldata);
+  EXPECT_THAT_EXPECTED(file, llvm::Succeeded());
+  auto module_sp = std::make_shared<Module>(file->moduleSpec());
+  ASSERT_NE(module_sp, nullptr);
+  auto object_file = module_sp->GetObjectFile();
+  ASSERT_NE(object_file, nullptr);
+  auto debug_map =
+      SymbolFileDWARFDebugMap::CreateInstance(object_file->shared_from_this());
+  ASSERT_EQ(debug_map, nullptr);
+}

[adrian-prantl]

Who is calling this? I'm wondering if the check might make more sense at the call site?

[JDevlieghere]

Plugins shouldn't depend on each other. The correct way to do this would be to have a method in ObjectFile, something like ObjectFile::SupportsDebugMap() which returns false for all implementations except the ObjectFileMachO one.

[bulbazord]

+1 to Jonas's comment. Plugins ideally don't depend on each other and they should strive to fail gracefully if they depend on a specific plugin instance that's not available. I recognize that many plugins depend on each other today, but I don't think we should be adding more direct dependencies if we can help it.