[lldb][Process/FreeBSDKernel] Remove libfbsdvmcore support#181283
[lldb][Process/FreeBSDKernel] Remove libfbsdvmcore support#181283
Conversation
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
mchoo7
temporarily deployed
to
main-branch-only
GitHub Actions
Inactive
llvmbot
temporarily deployed
to
main-branch-only
GitHub Actions
Inactive
|
@llvm/pr-subscribers-lldb Author: Minsoo Choo (mchoo7) ChangesKernel debuggers, by its nature, aren't frequently used to debug kernel dumps from other operating systems. For example, Due to libfbsdvmcore, adding new features that requires both modifying FreeBSDKernel's testing relies on libfbsdvmcore while most use cases are kvm (native) debugging, so the testing doesn't reflect the reality either. Some might say it's tolerable since FreeBSD's kgdb doesn't have testing, but still having one is always beneficial to both developers and users. Thus remove libfbsdvmcore support from the FreeBSDKernel plugin. Full diff: https://github.com/llvm/llvm-project/pull/181283.diff 5 Files Affected:
diff --git a/lldb/cmake/modules/LLDBConfig.cmake b/lldb/cmake/modules/LLDBConfig.cmake
index d4471b8a5418d..fe2a62cd04314 100644
--- a/lldb/cmake/modules/LLDBConfig.cmake
+++ b/lldb/cmake/modules/LLDBConfig.cmake
@@ -63,7 +63,6 @@ add_optional_dependency(LLDB_ENABLE_LZMA "Enable LZMA compression support in LLD
add_optional_dependency(LLDB_ENABLE_LUA "Enable Lua scripting support in LLDB" LuaAndSwig LUAANDSWIG_FOUND)
add_optional_dependency(LLDB_ENABLE_PYTHON "Enable Python scripting support in LLDB" PythonAndSwig PYTHONANDSWIG_FOUND)
add_optional_dependency(LLDB_ENABLE_LIBXML2 "Enable Libxml 2 support in LLDB" LibXml2 LIBXML2_FOUND VERSION ${LLDB_LIBXML2_VERSION})
-add_optional_dependency(LLDB_ENABLE_FBSDVMCORE "Enable libfbsdvmcore support in LLDB" FBSDVMCore FBSDVMCore_FOUND QUIET)
option(LLDB_USE_ENTITLEMENTS "When codesigning, use entitlements if available" ON)
option(LLDB_BUILD_FRAMEWORK "Build LLDB.framework (Darwin only)" OFF)
diff --git a/lldb/source/Plugins/Process/CMakeLists.txt b/lldb/source/Plugins/Process/CMakeLists.txt
index 3413360e975fb..9a5f1cd433717 100644
--- a/lldb/source/Plugins/Process/CMakeLists.txt
+++ b/lldb/source/Plugins/Process/CMakeLists.txt
@@ -8,6 +8,7 @@ if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
add_subdirectory(POSIX)
elseif (CMAKE_SYSTEM_NAME MATCHES "FreeBSD")
add_subdirectory(FreeBSD)
+ add_subdirectory(FreeBSDKernel)
add_subdirectory(POSIX)
elseif (CMAKE_SYSTEM_NAME MATCHES "NetBSD")
add_subdirectory(NetBSD)
@@ -28,5 +29,4 @@ add_subdirectory(Utility)
add_subdirectory(elf-core)
add_subdirectory(mach-core)
add_subdirectory(minidump)
-add_subdirectory(FreeBSDKernel)
add_subdirectory(wasm)
diff --git a/lldb/source/Plugins/Process/FreeBSDKernel/CMakeLists.txt b/lldb/source/Plugins/Process/FreeBSDKernel/CMakeLists.txt
index c35b4def24e25..2d90d0c333026 100644
--- a/lldb/source/Plugins/Process/FreeBSDKernel/CMakeLists.txt
+++ b/lldb/source/Plugins/Process/FreeBSDKernel/CMakeLists.txt
@@ -1,16 +1,3 @@
-set(FBSDKERNEL_LIBS)
-if(FBSDVMCore_FOUND)
- list(APPEND FBSDKERNEL_LIBS fbsdvmcore)
-endif()
-if("${CMAKE_SYSTEM_NAME}" MATCHES "FreeBSD")
- list(APPEND FBSDKERNEL_LIBS kvm)
-endif()
-
-if (NOT FBSDKERNEL_LIBS)
- message(STATUS "Skipping FreeBSDKernel plugin due to missing libfbsdvmcore")
- return()
-endif()
-
add_lldb_library(lldbPluginProcessFreeBSDKernel PLUGIN
ProcessFreeBSDKernel.cpp
RegisterContextFreeBSDKernel_arm64.cpp
@@ -23,5 +10,5 @@ add_lldb_library(lldbPluginProcessFreeBSDKernel PLUGIN
LINK_LIBS
lldbCore
lldbTarget
- ${FBSDKERNEL_LIBS}
+ kvm
)
diff --git a/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp b/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
index 196cb5a620f32..beb7e52adf49e 100644
--- a/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
+++ b/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
@@ -14,62 +14,20 @@
#include "ProcessFreeBSDKernel.h"
#include "ThreadFreeBSDKernel.h"
-#if LLDB_ENABLE_FBSDVMCORE
-#include <fvc.h>
-#endif
-#if defined(__FreeBSD__)
-#include <kvm.h>
-#endif
-
using namespace lldb;
using namespace lldb_private;
LLDB_PLUGIN_DEFINE(ProcessFreeBSDKernel)
-namespace {
-
-#if LLDB_ENABLE_FBSDVMCORE
-class ProcessFreeBSDKernelFVC : public ProcessFreeBSDKernel {
-public:
- ProcessFreeBSDKernelFVC(lldb::TargetSP target_sp, lldb::ListenerSP listener,
- fvc_t *fvc, const FileSpec &core_file);
-
- ~ProcessFreeBSDKernelFVC();
-
- size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
- lldb_private::Status &error) override;
-
-private:
- fvc_t *m_fvc;
-
- const char *GetError();
-};
-#endif // LLDB_ENABLE_FBSDVMCORE
-
-#if defined(__FreeBSD__)
-class ProcessFreeBSDKernelKVM : public ProcessFreeBSDKernel {
-public:
- ProcessFreeBSDKernelKVM(lldb::TargetSP target_sp, lldb::ListenerSP listener,
- kvm_t *fvc, const FileSpec &core_file);
-
- ~ProcessFreeBSDKernelKVM();
-
- size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
- lldb_private::Status &error) override;
-
-private:
- kvm_t *m_kvm;
-
- const char *GetError();
-};
-#endif // defined(__FreeBSD__)
-
-} // namespace
-
ProcessFreeBSDKernel::ProcessFreeBSDKernel(lldb::TargetSP target_sp,
- ListenerSP listener_sp,
+ ListenerSP listener_sp, kvm_t *kvm,
const FileSpec &core_file)
- : PostMortemProcess(target_sp, listener_sp, core_file) {}
+ : PostMortemProcess(target_sp, listener_sp, core_file), m_kvm(kvm) {}
+
+ProcessFreeBSDKernel::~ProcessFreeBSDKernel() {
+ if (m_kvm)
+ kvm_close(m_kvm);
+}
lldb::ProcessSP ProcessFreeBSDKernel::CreateInstance(lldb::TargetSP target_sp,
ListenerSP listener_sp,
@@ -77,23 +35,12 @@ lldb::ProcessSP ProcessFreeBSDKernel::CreateInstance(lldb::TargetSP target_sp,
bool can_connect) {
ModuleSP executable = target_sp->GetExecutableModule();
if (crash_file && !can_connect && executable) {
-#if LLDB_ENABLE_FBSDVMCORE
- fvc_t *fvc =
- fvc_open(executable->GetFileSpec().GetPath().c_str(),
- crash_file->GetPath().c_str(), nullptr, nullptr, nullptr);
- if (fvc)
- return std::make_shared<ProcessFreeBSDKernelFVC>(target_sp, listener_sp,
- fvc, *crash_file);
-#endif
-
-#if defined(__FreeBSD__)
kvm_t *kvm =
kvm_open2(executable->GetFileSpec().GetPath().c_str(),
crash_file->GetPath().c_str(), O_RDONLY, nullptr, nullptr);
if (kvm)
- return std::make_shared<ProcessFreeBSDKernelKVM>(target_sp, listener_sp,
- kvm, *crash_file);
-#endif
+ return std::make_shared<ProcessFreeBSDKernel>(target_sp, listener_sp, kvm,
+ *crash_file);
}
return nullptr;
}
@@ -287,50 +234,8 @@ lldb::addr_t ProcessFreeBSDKernel::FindSymbol(const char *name) {
return sym ? sym->GetLoadAddress(&GetTarget()) : LLDB_INVALID_ADDRESS;
}
-#if LLDB_ENABLE_FBSDVMCORE
-
-ProcessFreeBSDKernelFVC::ProcessFreeBSDKernelFVC(lldb::TargetSP target_sp,
- ListenerSP listener_sp,
- fvc_t *fvc,
- const FileSpec &core_file)
- : ProcessFreeBSDKernel(target_sp, listener_sp, crash_file), m_fvc(fvc) {}
-
-ProcessFreeBSDKernelFVC::~ProcessFreeBSDKernelFVC() {
- if (m_fvc)
- fvc_close(m_fvc);
-}
-
-size_t ProcessFreeBSDKernelFVC::DoReadMemory(lldb::addr_t addr, void *buf,
- size_t size, Status &error) {
- ssize_t rd = 0;
- rd = fvc_read(m_fvc, addr, buf, size);
- if (rd < 0 || static_cast<size_t>(rd) != size) {
- error = Status::FromErrorStringWithFormat("Reading memory failed: %s",
- GetError());
- return rd > 0 ? rd : 0;
- }
- return rd;
-}
-
-const char *ProcessFreeBSDKernelFVC::GetError() { return fvc_geterr(m_fvc); }
-
-#endif // LLDB_ENABLE_FBSDVMCORE
-
-#if defined(__FreeBSD__)
-
-ProcessFreeBSDKernelKVM::ProcessFreeBSDKernelKVM(lldb::TargetSP target_sp,
- ListenerSP listener_sp,
- kvm_t *fvc,
- const FileSpec &core_file)
- : ProcessFreeBSDKernel(target_sp, listener_sp, core_file), m_kvm(fvc) {}
-
-ProcessFreeBSDKernelKVM::~ProcessFreeBSDKernelKVM() {
- if (m_kvm)
- kvm_close(m_kvm);
-}
-
-size_t ProcessFreeBSDKernelKVM::DoReadMemory(lldb::addr_t addr, void *buf,
- size_t size, Status &error) {
+size_t ProcessFreeBSDKernel::DoReadMemory(lldb::addr_t addr, void *buf,
+ size_t size, Status &error) {
ssize_t rd = 0;
rd = kvm_read2(m_kvm, addr, buf, size);
if (rd < 0 || static_cast<size_t>(rd) != size) {
@@ -341,6 +246,4 @@ size_t ProcessFreeBSDKernelKVM::DoReadMemory(lldb::addr_t addr, void *buf,
return rd;
}
-const char *ProcessFreeBSDKernelKVM::GetError() { return kvm_geterr(m_kvm); }
-
-#endif // defined(__FreeBSD__)
+const char *ProcessFreeBSDKernel::GetError() { return kvm_geterr(m_kvm); }
diff --git a/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.h b/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.h
index 81c567581dd56..d933f7bc219f2 100644
--- a/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.h
+++ b/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.h
@@ -11,10 +11,14 @@
#include "lldb/Target/PostMortemProcess.h"
+#include <kvm.h>
+
class ProcessFreeBSDKernel : public lldb_private::PostMortemProcess {
public:
ProcessFreeBSDKernel(lldb::TargetSP target_sp, lldb::ListenerSP listener,
- const lldb_private::FileSpec &core_file);
+ kvm_t *kvm, const lldb_private::FileSpec &core_file);
+
+ ~ProcessFreeBSDKernel();
static lldb::ProcessSP
CreateInstance(lldb::TargetSP target_sp, lldb::ListenerSP listener,
@@ -44,11 +48,19 @@ class ProcessFreeBSDKernel : public lldb_private::PostMortemProcess {
lldb_private::DynamicLoader *GetDynamicLoader() override;
+ size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
+ lldb_private::Status &error) override;
+
protected:
bool DoUpdateThreadList(lldb_private::ThreadList &old_thread_list,
lldb_private::ThreadList &new_thread_list) override;
lldb::addr_t FindSymbol(const char *name);
+
+private:
+ kvm_t *m_kvm;
+
+ const char *GetError();
};
#endif // LLDB_SOURCE_PLUGINS_PROCESS_FREEBSDKERNEL_PROCESSFREEBSDKERNEL_H
|
I disagree with that statement, unless you drop the kernel part. We certainly support debugging XNU core dumps on non Darwin platforms. MacOSX-Kernel is for live debugging over KDP, not core file debugging which is handled by mach-core. That said, that doesn't need to changes the direction of this PR. |
My apologies, I was wrong there. The current libfbsdvmcore (fvc) implementation is copying FreeBSD's kvm source code and making it cross-platform. The biggest problem is here is that any modifications (e.g. bug fixes) should be backported to fvc, but fvc has no update for the last five years (https://github.com/Moritz-Systems/libfbsdvmcore) while modification still happen on the FreeBSD kvm side (https://github.com/freebsd/freebsd-src/commits/main/lib/libkvm). Since fvc isn't catching up with kvm, this makes implementing new features based on newer kvm harder. (and again, developers need to test on both platforms while fvc is undertested and we don't hear any feedbacks from the fvc side) |
Thanks for the context. That sounds like a totally reasonable motivation for this change. And apologies if I came across as pushing back on the idea, that wasn't my intention. |
|
I'm a bit in a conflict of interest here, since I was asked to develop this as part of a contract back in the day. IIRC the goal behind libfbsdvmcore was not to burden LLDB with maintaining a local implementation, but I guess this didn't really account for long-term maintenance. I suppose from FreeBSD perspective, the best way forward would be either to fork the project and maintain it independently, or inline the code into LLDB. But I don't really have the time to do it either, so I'm not going to stop you from removing it. |
|
This is a decision for FreeBSD developers to make, but if it is removed then there should be a release note stating that. |
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
Updated relnotes. |
|
I think this change is a little unfortunate, as cross-debugging a FreeBSD kernel core from a Linux, Mac, or Windows host is a valuable use case. I also arranged the funding to implement this originally. That said, I had the following comments (in an email thread) at the time this was being developed:
I think libfbsdvmcore was a useful exercise, but the practical experience is that the approach hasn't really worked out -- the library is not being maintained, and is not available on other operating systems. I still really want LLDB to be able to cross-debug FreeBSD kernels, but I'm fine with any one of the following approaches:
|
|
Note also that there should be 3 ways we can debug a FreeBSD kernel:
I think this change will also remove (3) on non-FreeBSD OSes, but should not need to? CC @bsdjhb |
LLVM allows licences other then Apache 2.0 with LLVM-exception:
Importing kvm functionality into the LLVM codebase can be justified and BSD-2 licence wouldn't be an issue. My only worry here is "take at least 4-6 weeks" part. Depending on what lies under kvm, my other works can be delayed or cause merge hell. I prefer having the LLVM foundation's consent in advance so I don't need to waste 4-6 weeks minimum.
I thought the first option for a while and came to a conclusion that it's near impossible. Since it is not in freebsd-src tree, it's easy to forget and miss it when changes happen to kvm in future. Since fvc is only used by LLDB, we are creating extra bridge between kvm and LLDB that is unnecessary. 3 is more efficient than 2. LLDB (and LLVM) depends on C++ features and I don't see the benefit of C/C++ interop. Also having C++ implementation makes code cleaner and simpler, especially class initializers part. 4 might be good, but due to time constraints I have, I don't think I can finish it before university starts in May. My preference is 3&5. LLVM 23 branching will happen on July 14th so it sounds realistic to finish them before LLVM 23. (Backporting this to FreeBSD's source tree is another problem though). Again, I find the 4-6 weeks part bit concerning. |
|
We could also change (1) into "incorporate libkvm portability directly into freebsd-src". In any case libfbsdvmcore is not really serving a purpose today if it's not packaged on other operating systems, and I'm fine with landing this PR and restoring cross-debugging later on. |
|
Okay, we can talk about the future plan later. @DavidSpickett , could you review the changes? I confirmed it builds and works on my core dump. |
I thought 3) is done through gdb-remote plugin? |
|
Please update the PR description to take into account the discussion here. Mainly to emphasise the maintenance part rather than its usefulness or lack of. Which seems to be in some dispute. Or even if it isn't, it's academic because there are enough issues with the current approach to justify removal.
Someone needs to confirm what methods work after this patch, so an accurate release note can be written. |
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
kvm/fvc has nothing to do with GDB remote protocol. The server-side implementation is done on FreeBSD. FreeBSD doc on this |
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
Code wise, LGTM. Can be merged when it has a FreeBSD approval.
@emaste does it look ok to you?
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
|
TestFreeBSDKernelVMCore.py is now failing on non-FreeBSD hosts. Could you update the test or revert until you have a chance to look? Thank you. https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/38910/console |
Of course, sorry for inconvenience. |
#181283 removed fvc but it still remains in tests. This causes testing on non-FreeBSD hosts. Thus add `skipUnlessPlatform` decorator. Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
Support was removed in llvm#181283 (a8cd1ac)
7 participants
Due to libfbsdvmcore, adding new features requires modifying both
ProcessFreeBSDKernelFVCandProcessFreeBSDKernelKVMwhich also requires testing on both. This is highly inefficient while the user base of fvc is currently invisible since most package manager don't ship libfbsdvmcore with LLDB.There is still demand for cross-platform kernel dump debugging. This will be implemented in future either by cloning and embedding kvm interface into LLDB or unifying dump formats to ELF core with minidump-to-elf conversion tool on FreeBSD side.