[lldb][Process/FreeBSDKernelCore] Add ppc64le support#180669
[lldb][Process/FreeBSDKernelCore] Add ppc64le support#180669
Conversation
|
@llvm/pr-subscribers-lldb Author: Minsoo Choo (mchoo7) ChangesThis is LLDB version of https://cgit.freebsd.org/ports/tree/devel/gdb/files/kgdb/ppcfbsd-kern.c. Trapframe unwinding will be implemented in future. Full diff: https://github.com/llvm/llvm-project/pull/180669.diff 4 Files Affected:
diff --git a/lldb/source/Plugins/Process/FreeBSDKernel/CMakeLists.txt b/lldb/source/Plugins/Process/FreeBSDKernel/CMakeLists.txt
index c35b4def24e25..251e8cea784af 100644
--- a/lldb/source/Plugins/Process/FreeBSDKernel/CMakeLists.txt
+++ b/lldb/source/Plugins/Process/FreeBSDKernel/CMakeLists.txt
@@ -15,6 +15,7 @@ add_lldb_library(lldbPluginProcessFreeBSDKernel PLUGIN
ProcessFreeBSDKernel.cpp
RegisterContextFreeBSDKernel_arm64.cpp
RegisterContextFreeBSDKernel_i386.cpp
+ RegisterContextFreeBSDKernel_ppc64le.cpp
RegisterContextFreeBSDKernel_x86_64.cpp
ThreadFreeBSDKernel.cpp
diff --git a/lldb/source/Plugins/Process/FreeBSDKernel/RegisterContextFreeBSDKernel_ppc64le.cpp b/lldb/source/Plugins/Process/FreeBSDKernel/RegisterContextFreeBSDKernel_ppc64le.cpp
new file mode 100644
index 0000000000000..5fb2908bfdb2c
--- /dev/null
+++ b/lldb/source/Plugins/Process/FreeBSDKernel/RegisterContextFreeBSDKernel_ppc64le.cpp
@@ -0,0 +1,95 @@
+//===-- RegisterContextFreeBSDKernel_ppc64le.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 "RegisterContextFreeBSDKernel_ppc64le.h"
+
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Thread.h"
+#include "lldb/Utility/RegisterValue.h"
+#include "llvm/Support/Endian.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+RegisterContextFreeBSDKernel_ppc64le::RegisterContextFreeBSDKernel_ppc64le(
+ Thread &thread, lldb_private::RegisterInfoInterface *register_info,
+ lldb::addr_t pcb_addr)
+ : RegisterContextPOSIX_ppc64le(thread, 0, register_info),
+ m_pcb_addr(pcb_addr) {}
+
+bool RegisterContextFreeBSDKernel_ppc64le::ReadRegister(
+ const RegisterInfo *reg_info, RegisterValue &value) {
+ if (m_pcb_addr == LLDB_INVALID_ADDRESS)
+ return false;
+
+ // https://cgit.freebsd.org/src/tree/sys/powerpc/include/pcb.h
+ struct {
+ llvm::support::ulittle64_t context[20];
+ llvm::support::ulittle64_t cr;
+ llvm::support::ulittle64_t sp;
+ llvm::support::ulittle64_t toc;
+ llvm::support::ulittle64_t lr;
+ } pcb;
+
+ Status error;
+ size_t rd =
+ m_thread.GetProcess()->ReadMemory(m_pcb_addr, &pcb, sizeof(pcb), error);
+ if (rd != sizeof(pcb))
+ return false;
+
+ uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
+ switch (reg) {
+ case gpr_r1_ppc64le:
+ // r1 is saved in the sp field
+ value = pcb.sp;
+ break;
+ case gpr_r2_ppc64le:
+ // r2 is saved in the toc field
+ value = pcb.toc;
+ break;
+ case gpr_r12_ppc64le:
+ case gpr_r13_ppc64le:
+ case gpr_r14_ppc64le:
+ case gpr_r15_ppc64le:
+ case gpr_r16_ppc64le:
+ case gpr_r17_ppc64le:
+ case gpr_r18_ppc64le:
+ case gpr_r19_ppc64le:
+ case gpr_r20_ppc64le:
+ case gpr_r21_ppc64le:
+ case gpr_r22_ppc64le:
+ case gpr_r23_ppc64le:
+ case gpr_r24_ppc64le:
+ case gpr_r25_ppc64le:
+ case gpr_r26_ppc64le:
+ case gpr_r27_ppc64le:
+ case gpr_r28_ppc64le:
+ case gpr_r29_ppc64le:
+ case gpr_r30_ppc64le:
+ case gpr_r31_ppc64le:
+ value = pcb.context[reg - gpr_r12_ppc64le];
+ break;
+ case gpr_pc_ppc64le:
+ case gpr_lr_ppc64le:
+ // The pc of crashing thread is stored in lr.
+ value = pcb.lr;
+ break;
+ case gpr_cr_ppc64le:
+ value = pcb.cr;
+ break;
+ default:
+ return false;
+ }
+ return true;
+}
+
+bool RegisterContextFreeBSDKernel_ppc64le::WriteRegister(
+ const RegisterInfo *reg_info, const RegisterValue &value) {
+ return false;
+}
diff --git a/lldb/source/Plugins/Process/FreeBSDKernel/RegisterContextFreeBSDKernel_ppc64le.h b/lldb/source/Plugins/Process/FreeBSDKernel/RegisterContextFreeBSDKernel_ppc64le.h
new file mode 100644
index 0000000000000..2f90a0e4bce0a
--- /dev/null
+++ b/lldb/source/Plugins/Process/FreeBSDKernel/RegisterContextFreeBSDKernel_ppc64le.h
@@ -0,0 +1,33 @@
+//===-- RegisterContextFreeBSDKernel_ppc64le.h ------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SOURCE_PLUGINS_PROCESS_FREEBSDKERNEL_REGISTERCONTEXTFREEBSDKERNEL_PPC64LE_H
+#define LLDB_SOURCE_PLUGINS_PROCESS_FREEBSDKERNEL_REGISTERCONTEXTFREEBSDKERNEL_PPC64LE_H
+
+#include "Plugins/Process/Utility/RegisterContextPOSIX_ppc64le.h"
+#include "Plugins/Process/elf-core/RegisterUtilities.h"
+
+class RegisterContextFreeBSDKernel_ppc64le
+ : public RegisterContextPOSIX_ppc64le {
+public:
+ RegisterContextFreeBSDKernel_ppc64le(
+ lldb_private::Thread &thread,
+ lldb_private::RegisterInfoInterface *register_info,
+ lldb::addr_t pcb_addr);
+
+ bool ReadRegister(const lldb_private::RegisterInfo *reg_info,
+ lldb_private::RegisterValue &value) override;
+
+ bool WriteRegister(const lldb_private::RegisterInfo *reg_info,
+ const lldb_private::RegisterValue &value) override;
+
+private:
+ lldb::addr_t m_pcb_addr;
+};
+
+#endif // LLDB_SOURCE_PLUGINS_PROCESS_FREEBSDKERNEL_REGISTERCONTEXTFREEBSDKERNEL_PPC64LE_H
diff --git a/lldb/source/Plugins/Process/FreeBSDKernel/ThreadFreeBSDKernel.cpp b/lldb/source/Plugins/Process/FreeBSDKernel/ThreadFreeBSDKernel.cpp
index dd1ed52719749..7f55ea509fb3e 100644
--- a/lldb/source/Plugins/Process/FreeBSDKernel/ThreadFreeBSDKernel.cpp
+++ b/lldb/source/Plugins/Process/FreeBSDKernel/ThreadFreeBSDKernel.cpp
@@ -15,9 +15,11 @@
#include "Plugins/Process/Utility/RegisterContextFreeBSD_i386.h"
#include "Plugins/Process/Utility/RegisterContextFreeBSD_x86_64.h"
#include "Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h"
+#include "Plugins/Process/Utility/RegisterInfoPOSIX_ppc64le.h"
#include "ProcessFreeBSDKernel.h"
#include "RegisterContextFreeBSDKernel_arm64.h"
#include "RegisterContextFreeBSDKernel_i386.h"
+#include "RegisterContextFreeBSDKernel_ppc64le.h"
#include "RegisterContextFreeBSDKernel_x86_64.h"
using namespace lldb;
@@ -62,6 +64,11 @@ ThreadFreeBSDKernel::CreateRegisterContextForFrame(StackFrame *frame) {
*this, std::make_unique<RegisterInfoPOSIX_arm64>(arch, 0),
m_pcb_addr);
break;
+ case llvm::Triple::ppc64le:
+ m_thread_reg_ctx_sp =
+ std::make_shared<RegisterContextFreeBSDKernel_ppc64le>(
+ *this, new RegisterInfoPOSIX_ppc64le(arch), m_pcb_addr);
+ break;
case llvm::Triple::x86:
m_thread_reg_ctx_sp = std::make_shared<RegisterContextFreeBSDKernel_i386>(
*this, new RegisterContextFreeBSD_i386(arch), m_pcb_addr);
|
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
|
I would be very careful citing what, at a glance, appears to be the source code of a GPL licensed project. In this case I think what you are trying to say is that like GDB, LLDB needs to have some form of kernel awareness. And looking at the code, it's 50% lldb APIs and 50% FreeBSD specifics so it's fine but you can understand why I want people to be careful about this. Perhaps that file was added by a FreeBSD contributor but still, I see GDB in the path and immediately I'm wondering. And there are some in the community who for professional reasons will not go near GPL code, so this is going to cut down your potential reviewers, if it is the case that the file is important context. Besides all that, I'm not sure how that file is relevant to us as reviewers in LLDB. So please expand on that. Perhaps you want to highlight that this is the first thing that needed to be done, as was done for your GDB port. Perhaps you want to say that it uses the same interfaces as the GDB port, and therefore there is prior art for that approach. In which case tell us about that in your own words. What's important for us here is what features this code enables. Or does not enable, as you have already mentioned with the trap handler. |
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
A FreeBSD related person needs to be the judge of this but I don't see anything overall wrong with it.
lldb/source/Plugins/Process/FreeBSD-Kernel-Core/RegisterContextFreeBSDKernelCore_ppc64le.cpp
Show resolved
Hide resolved
lldb/source/Plugins/Process/FreeBSDKernel/RegisterContextFreeBSDKernel_ppc64le.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/FreeBSDKernel/RegisterContextFreeBSDKernel_ppc64le.h
Outdated
Show resolved
Hide resolved
https://cgit.freebsd.org/ports/tree/devel/gdb/files/kgdb/ppcfbsd-kern.c This is portion of code is under BSD-2 license. |
Updated PR description. |
Ok cool, wasn't sure if that was upstream or not. But anyway, the description now includes everything a reviewer needs to know, thanks.
I have been assuming so far that kernel debugging can be done with a core dump or a live kernel. Is that right? I think "from PCB structure" is the live part. |
From core dump as well. I'll update description clarify it. |
mchoo7
changed the title
|
ppc doesn't have enough tool to support kernel debugging. Let's focus on ppc64le for now |
mchoo7
force-pushed
the
ppckern
branch
2 times, most recently
from
76c8129 to
37f5490
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
mchoo7
changed the title
|
For a bit of history here, we used to have a vendored copy of gdb in the FreeBSD base system, but we did not update past 6.6 (we stopped at the last GPLv2 version). We also shipped a binary kgdb which had support for kernel debugging (live and core files). It was not upstreamed to gdb at the time due to challenges with copyright assignment, FSF paperwork, etc. Some of our work did make it upstream afterwards, but I'm not sure if kgdb did. It lives on in our gdb port/package though. In any case, the code Minsoo references here was written by FreeBSD developers and was BSD licensed from inception. |
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
Looks fine in that it looks like the other architecture's changes.
A FreeBSD person should be the approver, just in case there are fine details I have missed.
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
mchoo7
changed the title
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
|
Assuming FreeBSD follows https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#REG, the register lookup into the pcb makes sense to me. I got on a side track wondering why not all registers are saved in the pcb. From what I've read, some registers are saved in the memory of the process itself, and the rest are in the pcb. Are those assumptions correct? There's no support here for the FPU and I assume that's intentional. If so please say that in the description. "I am not implementing FPU support at this time." is fine. |
It does saves fpu and vector registers in pcb (https://github.com/freebsd/freebsd-src/blob/main/sys/powerpc/include/pcb.h) but I don't know why kgdb didn't implement this. I asked other FreeBSD devs and hopefully they can answer my question. If FPU support was simply forgotten, I'll update the PR. |
|
Yeah if there's some blocking reason then make a note of it, but if not, parity with kgdb is fine for this PR. |
No replies from IRC channel, but based on the fact that this was introduced to KGDB back in 2015 and we didn't have proper support for ppc64le back then, I can only assume that no one was interested in this. The real problem is that in pcb.h there are other structs like pmap between the registers I added and fpu registers. Someone is working (or at least announced to work) on separating pcb fields for debugging and making it stable, so I'll defer fpu register support for now. |
|
Updated PR description |
Like setjmp/longjmp, context switching is a call into the cpu_switch function, and so only saves the call-preserved registers (with a bit of an asterisk to include registers that would normally be constant within a thread but change between threads). Call-clobbered registers, if needed by anyone up the call stack, will already be saved/restored by the compiler just like setjmp/longjmp, so it would be a bug for any code to rely on them being saved in struct pcb by cpu_switch. Contrast with struct trapframe, which saves the entire state (specifically, the subset that the kernel might clobber; on architectures like AArch64 with banked registers the EL0 registers can be left alone and instead handled as part of cpu_switch, optimising for the case that a trap does not result in context switching to a different thread) as traps can occur at any point during execution, not just function call boundaries. |
|
Thanks for explaining, I understand the difference now.
Yeah I didn't get the difference when I read that in some of the PR descriptions. I was thinking that pcb was what trapframe is, and trapframe was some other code that needed to be created in lldb, rather than another structure. |
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
Looks good to me.
@jrtc27 anything else you want to say here?
There was a problem hiding this comment.
This looks plausible, though PowerPC is the FreeBSD architecture I'm least familiar with. It would be nice if this supported all PowerPC flavours, given they have the same PCB layout, so the only thing that needs parameterising is the type to map register_t to. However, it seems LLDB's current approach of duplicating the entire register enumeration between ppc64 and ppc64le (no 32-bit it seems?), and curiously with some reordering and different extensions despite the architecture being the same between them, makes that difficult. So as future work I would suggest first unifying those two into a single "ppc" register set and then it should be trivial to support all of powerpc, powerpc64 and powerpc64le from FreeBSD with one implementation.
ppc64be doesn't have RegisterContextPosix support yet and ppc had issue with cpp casting IIRC. I will come back to those once I have time and motivation to do so. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/39885 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/17404 Here is the relevant piece of the build log for the reference |
This is LLDB version of https://cgit.freebsd.org/ports/tree/devel/gdb/files/kgdb/ppcfbsd-kern.c. This enables selecting ppc64le and reading registers from PCB structure on core dump and live kernel debugging. FPU registers aren't supported yet due to pcb structure issue, but this change still achieves feature parity with KGDB. Trapframe unwinding support will be implemented in future. Test files using core dump from ppc64le will be implemented once other kernel debugging improvements are done. --------- Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
This is LLDB version of https://cgit.freebsd.org/ports/tree/devel/gdb/files/kgdb/ppcfbsd-kern.c. This enables selecting ppc64le and reading registers from PCB structure on core dump and live kernel debugging. FPU registers aren't supported yet due to pcb structure issue, but this change still achieves feature parity with KGDB. Trapframe unwinding support will be implemented in future. Test files using core dump from ppc64le will be implemented once other kernel debugging improvements are done. --------- Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
This commit brings improves `ProcessFreeBSDKernelCore::DoUpdateThreadList()` by porting features from KGDB (`fbsd_kthr.c` specifically) and adding fixes. It includes: 1. Validate `stopped_cpus` before accessing `stoppcbs` 2. Check bounds for `mp_maxid` 3. Check errors when reading proc from memory 4. Detect new architectures from previous PRs (ppc64le, riscv64, arm) which weren't handled correctly in `DoUpdateThreadList()`. Fallbacks for finding offsets aren't ported since kernel exposes hardcoded variables for offsets starting from FreeBSD 11. Fallbacks aren't needed as LLDB 23 only supports FreeBSD 14 and later. This commit also Fixes: 2430410(#180669), 4a602c0(#180670), 3d25128(#180674) --------- Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
6 participants
This is LLDB version of https://cgit.freebsd.org/ports/tree/devel/gdb/files/kgdb/ppcfbsd-kern.c. This enables selecting ppc64le and reading registers from PCB structure on core dump and live kernel debugging. FPU registers aren't supported yet due to pcb structure issue, but this change still achieves feature parity with KGDB. Trapframe unwinding support will be implemented in future. Test files using core dump from ppc64le will be implemented once other kernel debugging improvements are done.