[lldb] Don't create instance of SymbolFileDWARFDebugMap for non-Mach-O files
#139170
Conversation
royitaqi
changed the title
SymbolFileDWARFDebugMap for non-Mach-O filesSymbolFileDWARFDebugMap for non-Mach-O files
|
@llvm/pr-subscribers-lldb Author: None (royitaqi) ChangesChange
BenefitThis may improve Linux debugger launch time by skipping the creation of TestsTwo tests are added to verify the creation and the absence for Mach-O and non-Mach-O files. Ran Full diff: https://github.com/llvm/llvm-project/pull/139170.diff 3 Files Affected:
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);
+}
|
|
+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. |
|
@JDevlieghere I see your point (some learning questions below about the principle). However, I feel Instead, maybe it's better if we frame "DebugMap" as an WDYT? (Note: I am trying to think about how does this relate to the -- (my learning question about the principle of "Plugins shouldn't depend on each other") -- I guess I'm missing a lot of context here. From my limited and naive view, there are natural connections between different kinds of plugins because how they usually work together in existing systems. E.g. macOS usually use Mach-O files and can use either |
|
FWIW, I completely agree @royitaqi. I'm not saying inter-plugin dependencies should be a free-for-all (*), but I think that some kinds of dependencies make sense and I definitely think this is one of them. The debug map format is intimately tied to the MachO and darwin ecosystems and I think there's approximately zero chance that it will be implemented elsewhere. Using the debug map requires MachO files, so why not just admit that? This isn't the only example of that. For example, ProcessElfCore requires elf core files. ProcessMachCore requires MachO core files. And I think that's a perfectly natural state of the world and doing anything else would make things a lot more convoluted. I think something like (*) I think that plugin dependencies should form a DAG. The fact that SymbolFileDWARFDebugMap depends on ObjectFileMachO means that ObjectFileMachO should not depend on SymbolFileDWARFDebugMap -- which I think is also perfectly natural restriction. And even if there is a reason why you might want to depend the other way, you can try to break it using the usual dependency reversal techniques, but you'd only need to touch these two plugins -- not every plugin of the two kinds. I might also say that, for the sake of clarity, the dependency direction should be uniform across all plugins of the same type. So that SymbolFileDWARFDebugMap depends on ObjectFileMachO means that no other object file plugin can depend on any other symbol file plugin, which is, again, I think a reasonable requirement, and I would say that if we ever find a case where we have two plugin pairs which want to depend in the opposite directions, that this means that those plugin types are not designed properly (maybe they ought to be a single plugin?). |
To answer your last question: because the plugins (which is really a misnomer), as I understand it, are there to provide abstraction: the caller shouldn't have to know what concrete instance an I'll be the first to admit that the current approach has shortcomings (like it does here, I totally agree with that). However we should also remember that it exists for a reason and that a lot of time and effort has been spent on detangling the plugins to fix layering issues and reduce binary sizes. The current rule is easy to understand and easy to enforce which makes it appealing and ensures we don't make things worse as we're still working on this. IMHO, the downsides being listed here don't outweigh the benefits, especially since we have a bunch of examples of those "moral dependencies". That said, if there's a way to enforce the DAG described in the asterisk, I'm totally fine with changing the rule to allow inter-plugin dependencies "where it makes sense" (which is still subjective). I made a similar argument in another PR a while ago, but I don't think we can expect reviewers to mentally verify this DAG every time someone tries to introduce a new inter-plugin dependencies. TL;DR: My stance is that if there's a way to enforce the DAG, I'm totally fine with allowing inter-plugin dependencies where they make sense. Barring that I think the current rule, albeit overly strict, is the easiest way to enforce we don't regress inter-plugin dependencies in general. PS: I'm actually glad we're having this discussion. Whatever the outcome, I'd like to document it on the website so contributors can know what to expect going forward. This comes up periodically and there's no reason this should be a surprise for existing or new contributors. |
This is my problem with that. You're saying the "caller shouldn't have to know" -- which is a statement about his state of mind. We can't enforce the state of mind through syntactic restrictions. We can prevent the caller from saying he expects a specific subclass, but there's not way to prevent code from expecting that implicitly (perhaps even without it knowing). Functions like Nonetheless, I agree that plugins should create abstractions, but I think it's important to think about who are those abstractions directed at. The core code definitely shouldn't need to "look behind the curtain" (ideally not even through calls like ProcessElfCore needs to extract a lot of detailed information from ObjectFileELF. Same goes for ProcessMachCore. At some level, the information these two pairs are exchanging is the same (threads, registers, ...), so one could imagine a generic interface to pass this -- but would that help anything? I say "no", because you still want to have the two plugins to work in pair -- if you didn't, then you wouldn't need separate process core plugins. This setup would make sense if we had a generic ProcessCore class (not a plugin) which works off of information provided by the object plugins -- but that's not the situation we're in now. You can make the same case for dynamic loaders -- should MacOSX-DYLD work with ObjectFilePECOFF?
Enforcement is tricky, because "where it makes sense" is obviously a judgement call. It's also complicated by the fact that current graph is not a DAG. Linux linkers actually kind of enforce the graph implicitly, but since it's not a DAG now, we have a workaround and that prevents us catching any new bad deps. I guess it would be possible to do something similar to I think the best we can do is some cmake check plus some documentation about which dependencies we consider "okay". A rule like "there shall be no inter-plugin dependencies" is easy to enforce and that makes it appealing, but I think it will be very hard for us to come into compliance with that (and I don't think we'd like the result even if we did). If we say some deps are okay, then a lot of things become automatically compliant, and we can focus on the high-value high-complexity deps like SymbolFile<->TypeSystem. |
To be clear, I pretty much agree with everything you've said. I'm not arguing that the current approach is better, at least not on merit. My point is that barring a better alternative, those shortcoming are the price to pay to avoid the free-for-all nobody wants. I'm glad that you're offering an alternative below.
I'm fine with that. What I'm looking for is something that's (relatively) easy to understand and (relatively) easy to spot in code reviews. I'd love to have something foolproof (or work towards that) but I acknowledge that given the current state of things that's not achievable. What you describe here checks both boxes.
Ack, agreed. To make this actionable, @labath or @royitaqi, is either of you willing to sign up to add the CMakery and document the new policy? |
|
I'm glad we're in (abstract) agreement. As much as I would like to, I can't promise to do this by any particular date. I'd can try, but I don't know when will that happen. In a way, I feel that this patch isn't a good reason to motivate this (although I think that legalising the existing core file dependencies is a good reason) for this as I think there are acceptable workarounds (or even not workarounds) for the problem. In particular, I think that the solution from #139554 which looks at the Triple is okay. The Triple object even has a |
royitaqi
force-pushed
the
dont-create-debug-map-on-linux
branch
from
96b17f8 to
09bd622
Compare
royitaqi
force-pushed
the
dont-create-debug-map-on-linux
branch
from
09bd622 to
57145ec
Compare
|
Thanks for the great discussion, @JDevlieghere and @labath. I appreciate it and am learning a lot by reading through it. As far as I can understand, I think what you have aligned make sense. Re. CMakery and document, sorry that I cannot promise to do this by a timeline, because as a CMake noob I will probably need to learn about it first before I can understand what exactly needs to be done. With that said, will keep this in my mind and learn towards it when I have a chance. -- Also thanks to @labath for the suggestion about using (already fixed) |
|
Updated the code to using @labath / @JDevlieghere: Please kindly take a look when you have the time. |
lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFDebugMapTests.cpp
Outdated
Show resolved
Hide resolved
|
NB I reverted this on main because it depends on |
Hi @jasonmolenda, I appreciate you actively trying to make the code base better, and I can see why the other revert may make sense, BUT:
I respectfully disagree. This patch doesn't depend on #142704.
Please kindly LMK if I missed something in the above. Thanks, |
|
I'll admit I didn't debug it very far, but when I reverted the ObjectFileMachO.cpp change, |
|
e.g. https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/27150/ but it reproduced on my arm64 macOS desktop no problem when I did |
|
Ultimately I expect was not succeeding for this binary. The compile command in the shell test is doing Pretty unusual for a userland binary, but you could definitely see something like this with a standalone firmware/kernel Mach-O binary. I don't think depending on the existence of the LC_BUILD load command to set the ObjectFile's architecture is a good choice, it will be fragile. |
|
FTR I suspect the correct fix for #142704 is
|
|
@jasonmolenda I appreciate the details. IIRC I ran |
|
You are right, Thank you for catching this and the revert. My bad (mistakenly thought the patch is independent). |
How about:
I think this is related to what you said in #142704. I guess my question is "why not look at vendor" (esp when the OS is not set).
|
Change
SymbolFileDWARFDebugMap::CreateInstance()will returnnullptrif the file is not a Mach-O.Benefit
This may improve Linux debugger launch time by skipping the creation of
SymbolFileDWARFDebugMapduring theSymbolFile::FindPlugin()call, which loops through a list ofSymbolFileplugins and tries to find the one that provides the best abilities. If theSymbolFileDWARFDebugMapis 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
SymbolFileDWARFDebugMapfor Mach-O and non-Mach-O files, respectively.Ran
ninja check-lldbon both Darwin and Linux. No regression spotted. See results compared for mac and linux.