[Lldb-commits] [lldb] [lldb] Change Module to have a concrete UnwindTable, update (PR #101130)
jasonmolenda wrote: Thanks for the suggestion @jimingham , I pushed an update to implement that idea. https://github.com/llvm/llvm-project/pull/101130 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change Module to have a concrete UnwindTable, update (PR #101130)
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/101130
>From 55b1ac1fbad89ebc50038738c1e09045e9884be8 Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Mon, 29 Jul 2024 22:37:43 -0700
Subject: [PATCH 1/3] [lldb] Change Module to have a concrete UnwindTable,
update
Currently a Module has a std::optional which is
created when the UnwindTable is requested from outside the
Module. The idea is to delay its creation until the Module
has an ObjectFile initialized, which will have been done by
the time we're doing an unwind.
However, Module::GetUnwindTable wasn't doing any locking, so it was
possible for two threads to ask for the UnwindTable for the first
time, one would be created and returned while another thread would
create one, destroy the first in the process of emplacing it.
It was an uncommon crash, but it was possible.
Grabbing the Module's mutex would be one way to address it, but
when loading ELF binaries, we start creating the SymbolTable on one
thread (ObjectFileELF) grabbing the Module's mutex, and then spin
up worker threads to parse the individual DWARF compilation units,
which then try to also get the UnwindTable and deadlock if they try
to get the Module's mutex.
This changes Module to have a concrete UnwindTable as an ivar, and
when it adds an ObjectFile or SymbolFileVendor, it will call the
Update method on it, which will re-evaluate which sections exist
in the ObjectFile/SymbolFile. UnwindTable used to have an Initialize
method which set all the sections, and an Update method which would
set some of them if they weren't set. I unified these with the
Initialize method taking a `force` option to re-initialize the
section pointers even if they had been done already before.
This is addressing a rare crash report we've received, and also a
failure Adrian spotted on the -fsanitize=address CI bot last week,
it's still uncommon with ASAN but it can happen with the standard
testsuite.
rdar://132514736
---
lldb/include/lldb/Core/Module.h| 6 +--
lldb/include/lldb/Symbol/UnwindTable.h | 2 +-
lldb/source/Core/Module.cpp| 26 +++--
lldb/source/Symbol/UnwindTable.cpp | 51 ++
4 files changed, 23 insertions(+), 62 deletions(-)
diff --git a/lldb/include/lldb/Core/Module.h b/lldb/include/lldb/Core/Module.h
index 0188057247a68..5589c1c9a350d 100644
--- a/lldb/include/lldb/Core/Module.h
+++ b/lldb/include/lldb/Core/Module.h
@@ -1021,9 +1021,9 @@ class Module : public
std::enable_shared_from_this,
lldb::ObjectFileSP m_objfile_sp; ///< A shared pointer to the object file
/// parser for this module as it may or may
/// not be shared with the SymbolFile
- std::optional m_unwind_table; ///< Table of FuncUnwinders
- /// objects created for this
- /// Module's functions
+ UnwindTable m_unwind_table; ///< Table of FuncUnwinders
+ /// objects created for this
+ /// Module's functions
lldb::SymbolVendorUP
m_symfile_up; ///< A pointer to the symbol vendor for this module.
std::vector
diff --git a/lldb/include/lldb/Symbol/UnwindTable.h
b/lldb/include/lldb/Symbol/UnwindTable.h
index 26826e5d1b497..0e7d76b1b896c 100644
--- a/lldb/include/lldb/Symbol/UnwindTable.h
+++ b/lldb/include/lldb/Symbol/UnwindTable.h
@@ -64,7 +64,7 @@ class UnwindTable {
private:
void Dump(Stream );
- void Initialize();
+ void Initialize(bool force = false);
std::optional GetAddressRange(const Address ,
const SymbolContext );
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 9c105b3f0e57a..acc17ecad015b 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -131,7 +131,8 @@ Module *Module::GetAllocatedModuleAtIndex(size_t idx) {
}
Module::Module(const ModuleSpec _spec)
-: m_file_has_changed(false), m_first_file_changed_log(false) {
+: m_unwind_table(*this), m_file_has_changed(false),
+ m_first_file_changed_log(false) {
// Scope for locker below...
{
std::lock_guard guard(
@@ -238,7 +239,8 @@ Module::Module(const FileSpec _spec, const ArchSpec
,
: m_mod_time(FileSystem::Instance().GetModificationTime(file_spec)),
m_arch(arch), m_file(file_spec), m_object_name(object_name),
m_object_offset(object_offset), m_object_mod_time(object_mod_time),
- m_file_has_changed(false), m_first_file_changed_log(false) {
+ m_unwind_table(*this), m_file_has_changed(false),
+ m_first_file_changed_log(false) {
// Scope for locker below...
{
std::lock_guard guard(
@@ -254,7 +256,9 @@ Module::Module(const FileSpec _spec, const ArchSpec
,
m_object_name.AsCString(""), m_object_name.IsEmpty() ? "" : ")");
}
-Module::Module()
[Lldb-commits] [lldb] [lldb][test] Fix TestMultipleDebuggers test on non-x86, other small issues (PR #101169)
https://github.com/jasonmolenda approved this pull request. Thanks for the good cleanup. https://github.com/llvm/llvm-project/pull/101169 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change Module to have a concrete UnwindTable, update (PR #101130)
jasonmolenda wrote: > It is a little odd that the laziness of the UnwindTable itself gets defeated > by Update, which forces the Initialize even if the unwind table isn't > currently being requested. I could add an ivar `m_module_has_updated` which is set when a new ObjectFile/SymbolFileVendor is added to the Module. And all the getter methods in UnwindTable call `Initialize()` which currently checks that it has been initialized or not, and add that in. > Not sure if that matters? If it does, Update could just tell the UnwindTable > that next time it gets asked a question it has to reread the unwind > information. Maybe it could even just set m_initialized back to `false` and > then let the lazy initialization redo the work when requested? Hah, I didn't read your full comment. Yes, same idea, and just as good. I'll do that. https://github.com/llvm/llvm-project/pull/101130 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change Module to have a concrete UnwindTable, update (PR #101130)
@@ -64,7 +64,7 @@ class UnwindTable {
private:
void Dump(Stream );
- void Initialize();
+ void Initialize(bool force = false);
jasonmolenda wrote:
Yeah I'm not thrilled with the terminology I used. But I had `Initialize`
which assumed no unwind sources had been discovered, and `Update` called after
a SymbolFileVendor is added to the Module which looks for any un-set unwind
sources and checks to see if they can now be found. That duplication didn't
thrill me.
https://github.com/llvm/llvm-project/pull/101130
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Optimized lldb-server memory usage (PR #100666)
jasonmolenda wrote: Hm, I was worried that we might get the filepath of a unix socket in some use case, but we llvm::to_integer(port_cstr) shortly later, so that can't be the case. https://github.com/llvm/llvm-project/pull/100666 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add constant value mode for RegisterLocation in UnwindPlans (PR #100624)
https://github.com/jasonmolenda approved this pull request. This looks good to me. One small nit on the Dump method. As a bit of background, this would be very unusual for a normal UnwindPlan that is reused for a function in many different contexts (different places on the stack during the program runtime). Felipe is dealing with the special case when the Swift LanguageRuntime creates an UnwindPlan that is used *once* in a given stack, and not saved in the UnwindTable for this Module. Felipe could have constructed a DWARF expression, where the register value was stored in memory, or a DWARF expression with a constant value, but then we'd need to add a `DataBufferSP` to the `RegisterLocation` to have ownership of this heap-allocated dwarf expression, in addition to hand-creating the DWARF expression bytecodes. It was a lot more straightforward to add this type of "this register is value x" for this one-shot UnwindPlan's that a LanguageRuntime can provide. https://github.com/llvm/llvm-project/pull/100624 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add constant value mode for RegisterLocation in UnwindPlans (PR #100624)
@@ -153,6 +155,9 @@ void UnwindPlan::Row::RegisterLocation::Dump(Stream ,
if (m_type == atDWARFExpression)
s.PutChar(']');
} break;
+ case isConstant:
+s.Printf("=%x", m_location.offset);
jasonmolenda wrote:
Should the Printf be `("=0x%" PRIx64, m_location.constant_value)`? If you do
an unwind with one of these register locations in use, we should see this
output when the unwind engine shows the unwind rules being used for this
function.
https://github.com/llvm/llvm-project/pull/100624
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add constant value mode for RegisterLocation in UnwindPlans (PR #100624)
https://github.com/jasonmolenda edited https://github.com/llvm/llvm-project/pull/100624 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][SBSaveCore] Implement a selectable threadlist for Core Options. (PR #100443)
jasonmolenda wrote: Stepping back a bit, do we gain anything in `SaveCoreOptions` by having `m_process_sp` be optional? Is there a distinction between an empty ProcessSP and no ProcessSP object? https://github.com/llvm/llvm-project/pull/100443 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][SBSaveCore] Implement a selectable threadlist for Core Options. (PR #100443)
@@ -46,8 +48,79 @@ SaveCoreOptions::GetOutputFile() const {
return m_file;
}
+Status SaveCoreOptions::SetProcess(lldb::ProcessSP process_sp) {
+ Status error;
+ if (!process_sp) {
+ClearProcessSpecificData();
+m_process_sp = std::nullopt;
+return error;
+ }
+
+ if (!process_sp->IsValid()) {
+error.SetErrorString("Cannot assign an invalid process.");
+return error;
+ }
+
+ if (m_process_sp.has_value())
jasonmolenda wrote:
Just take this as one possible opinion: I find the naming of this
`std::optional m_process_sp` a little confusing. In this method we
have a `ProcessSP process_sp` and `m_process_sp` which is an `optional`. Above
we see code doing `if (!process_sp)` - cool. Then I come to `if
(m_process_sp.has_value())` and I'm trying to figure out what that method does
in a `std::shared_ptr` and why it's different than the above bool.
I'm not sure `m_process_sp` is the best name, but we have no convention for
this kind of thing today.
https://github.com/llvm/llvm-project/pull/100443
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][SBSaveCore] Implement a selectable threadlist for Core Options. (PR #100443)
@@ -46,8 +48,79 @@ SaveCoreOptions::GetOutputFile() const {
return m_file;
}
+Status SaveCoreOptions::SetProcess(lldb::ProcessSP process_sp) {
+ Status error;
+ if (!process_sp) {
+ClearProcessSpecificData();
+m_process_sp = std::nullopt;
+return error;
+ }
+
+ if (!process_sp->IsValid()) {
+error.SetErrorString("Cannot assign an invalid process.");
+return error;
+ }
+
+ if (m_process_sp.has_value())
+ClearProcessSpecificData();
+
+ m_process_sp = process_sp;
+ return error;
+}
+
+Status SaveCoreOptions::AddThread(lldb_private::Thread *thread) {
+ Status error;
+ if (!thread) {
+error.SetErrorString("Thread is null");
+ }
+
+ if (!m_process_sp.has_value())
+m_process_sp = thread->GetProcess();
+
+ if (m_process_sp.value() != thread->GetProcess()) {
+error.SetErrorString("Cannot add thread from a different process.");
+return error;
+ }
+
+ std::pair tid_pair(thread->GetID(),
+ thread->GetBackingThread());
+ m_threads_to_save.insert(tid_pair);
+ return error;
+}
+
+bool SaveCoreOptions::RemoveThread(lldb_private::Thread *thread) {
+ return thread && m_threads_to_save.erase(thread->GetID()) > 0;
+}
+
+bool SaveCoreOptions::ShouldSaveThread(lldb::tid_t tid) const {
jasonmolenda wrote:
`ShouldSaveThread` makes this method sound like a caller can use this to
request a thread is included. This is more like `ThreadWillBeSaved` maybe?
https://github.com/llvm/llvm-project/pull/100443
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][SBSaveCore] Implement a selectable threadlist for Core Options. (PR #100443)
@@ -46,8 +48,79 @@ SaveCoreOptions::GetOutputFile() const {
return m_file;
}
+Status SaveCoreOptions::SetProcess(lldb::ProcessSP process_sp) {
+ Status error;
+ if (!process_sp) {
+ClearProcessSpecificData();
+m_process_sp = std::nullopt;
+return error;
+ }
+
+ if (!process_sp->IsValid()) {
+error.SetErrorString("Cannot assign an invalid process.");
+return error;
+ }
+
+ if (m_process_sp.has_value())
+ClearProcessSpecificData();
+
+ m_process_sp = process_sp;
+ return error;
+}
+
+Status SaveCoreOptions::AddThread(lldb_private::Thread *thread) {
+ Status error;
+ if (!thread) {
+error.SetErrorString("Thread is null");
jasonmolenda wrote:
Do you want to early-return here? We might dereference the `thread` a few
lines below.
https://github.com/llvm/llvm-project/pull/100443
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][SBSaveCore] Implement a selectable threadlist for Core Options. (PR #100443)
@@ -46,8 +48,79 @@ SaveCoreOptions::GetOutputFile() const {
return m_file;
}
+Status SaveCoreOptions::SetProcess(lldb::ProcessSP process_sp) {
+ Status error;
+ if (!process_sp) {
+ClearProcessSpecificData();
+m_process_sp = std::nullopt;
+return error;
+ }
+
+ if (!process_sp->IsValid()) {
+error.SetErrorString("Cannot assign an invalid process.");
+return error;
+ }
+
+ if (m_process_sp.has_value())
+ClearProcessSpecificData();
+
+ m_process_sp = process_sp;
+ return error;
+}
+
+Status SaveCoreOptions::AddThread(lldb_private::Thread *thread) {
+ Status error;
+ if (!thread) {
+error.SetErrorString("Thread is null");
+ }
+
+ if (!m_process_sp.has_value())
+m_process_sp = thread->GetProcess();
+
+ if (m_process_sp.value() != thread->GetProcess()) {
+error.SetErrorString("Cannot add thread from a different process.");
+return error;
+ }
+
+ std::pair tid_pair(thread->GetID(),
+ thread->GetBackingThread());
+ m_threads_to_save.insert(tid_pair);
jasonmolenda wrote:
personal preference, but you could ` m_threads_to_save.insert({thread->GetID(),
thread->GetBackingThread()})` here. Fine this way too.
https://github.com/llvm/llvm-project/pull/100443
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][SBSaveCore] Implement a selectable threadlist for Core Options. (PR #100443)
https://github.com/jasonmolenda edited https://github.com/llvm/llvm-project/pull/100443 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][SBSaveCore] Implement a selectable threadlist for Core Options. (PR #100443)
https://github.com/jasonmolenda commented: The MachO changes look fine to me. I had a few other small pieces of feedback, I think they're mostly matters of opinion so just take them as such, not something that must be changed. https://github.com/llvm/llvm-project/pull/100443 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] IRMemoryMap zero address mapping fix (PR #99045)
jasonmolenda wrote: I've merged https://github.com/llvm/llvm-project/pull/100288 I think we should close this PR. https://github.com/llvm/llvm-project/pull/99045 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Don't use a vm addr range starting at 0 for local memory (PR #100288)
https://github.com/jasonmolenda closed https://github.com/llvm/llvm-project/pull/100288 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Don't use a vm addr range starting at 0 for local memory (PR #100288)
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/100288
>From 28439443b9e44a242e59bf1cdd114486380d54fc Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Tue, 23 Jul 2024 18:54:31 -0700
Subject: [PATCH 1/2] [lldb] Don't use a vm addr range starting at 0 for local
memory
When an inferior stub cannot allocate memory for lldb, and lldb
needs to store the result of expressions, it will do it in lldb's
own memory range ("host memory"). But it needs to find a virtual
address range that is not used in the inferior process. It tries
to use the qMemoryRegionInfo gdb remote serial protocol packet to
find a range that is inaccessible, starting at address 0 and moving
up the size of each region.
If the first region found at address 0 is inaccessible, lldb will
use the address range starting at 0 to mean "read lldb's host memory,
not the process memory", and programs that crash with a null
dereference will have poor behavior.
This patch skips consideration of a memory region that starts at
address 0.
I also clarified the documentation of qMemoryRegionInfo to make it
clear that the stub is required to provide permissions for a memory
range that is accessable, it is not an optional key in this response.
This issue was originally found by a stub that did not list
permissions, and lldb treated the first region returned as the one
it would use.
---
lldb/docs/resources/lldbgdbremote.md | 8 +++-
lldb/source/Expression/IRMemoryMap.cpp | 17 -
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/lldb/docs/resources/lldbgdbremote.md
b/lldb/docs/resources/lldbgdbremote.md
index 7076a75032dae..5cac3736337a8 100644
--- a/lldb/docs/resources/lldbgdbremote.md
+++ b/lldb/docs/resources/lldbgdbremote.md
@@ -1403,6 +1403,12 @@ For instance, with a macOS process which has nothing
mapped in the first
The lack of `permissions:` indicates that none of read/write/execute are valid
for this region.
+The stub must include `permissions:` key-value on all memory ranges
+that are valid to access in the inferior process -- the lack of
+`permissions:` means that this is an inaccessible (no page table
+entries exist, in a system using VM) memory range. If a stub cannot
+determine actual permissions, return `rwx`.
+
**Priority To Implement:** Medium
This is nice to have, but it isn't necessary. It helps LLDB
@@ -2434,4 +2440,4 @@ The `0x` prefixes are optional - like most of the
gdb-remote packets,
omitting them will work fine; these numbers are always base 16.
The length of the payload is not provided. A reliable, 8-bit clean,
-transport layer is assumed.
\ No newline at end of file
+transport layer is assumed.
diff --git a/lldb/source/Expression/IRMemoryMap.cpp
b/lldb/source/Expression/IRMemoryMap.cpp
index de631370bb048..bb3e4e0a9a9fc 100644
--- a/lldb/source/Expression/IRMemoryMap.cpp
+++ b/lldb/source/Expression/IRMemoryMap.cpp
@@ -84,7 +84,7 @@ lldb::addr_t IRMemoryMap::FindSpace(size_t size) {
// any allocations. Otherwise start at the beginning of memory.
if (m_allocations.empty()) {
-ret = 0x0;
+ret = 0;
} else {
auto back = m_allocations.rbegin();
lldb::addr_t addr = back->first;
@@ -116,10 +116,17 @@ lldb::addr_t IRMemoryMap::FindSpace(size_t size) {
Status err = process_sp->GetMemoryRegionInfo(ret, region_info);
if (err.Success()) {
while (true) {
-if (region_info.GetReadable() != MemoryRegionInfo::OptionalBool::eNo ||
-region_info.GetWritable() != MemoryRegionInfo::OptionalBool::eNo ||
-region_info.GetExecutable() !=
-MemoryRegionInfo::OptionalBool::eNo) {
+if (region_info.GetRange().GetRangeBase() == 0 &&
+region_info.GetRange().GetRangeEnd() - 1 < end_of_memory) {
+ // Don't use a region that starts at address 0,
+ // that can mask null dereferences in the inferior.
+ ret = region_info.GetRange().GetRangeEnd();
+} else if (region_info.GetReadable() !=
+ MemoryRegionInfo::OptionalBool::eNo ||
+ region_info.GetWritable() !=
+ MemoryRegionInfo::OptionalBool::eNo ||
+ region_info.GetExecutable() !=
+ MemoryRegionInfo::OptionalBool::eNo) {
if (region_info.GetRange().GetRangeEnd() - 1 >= end_of_memory) {
ret = LLDB_INVALID_ADDRESS;
break;
>From 6fbe7592b2baccdb7656b20198088a350ab887f1 Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Wed, 24 Jul 2024 16:01:21 -0700
Subject: [PATCH 2/2] fix minor nit.
---
lldb/source/Expression/IRMemoryMap.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lldb/source/Expression/IRMemoryMap.cpp
b/lldb/source/Expression/IRMemoryMap.cpp
index bb3e4e0a9a9fc..0c1d9016616cb 100644
--- a/lldb/source/Expression/IRMemoryMap.cpp
+++ b/lldb/source/Expression/IRMemoryMap.cpp
@@ -117,9 +117,10 @@ lldb::addr_t
[Lldb-commits] [lldb] [lldb] Prevent passing a nullptr to std::string in ObjectFileMachO (PR #100421)
https://github.com/jasonmolenda approved this pull request. LGTM. https://github.com/llvm/llvm-project/pull/100421 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] IRMemoryMap zero address mapping fix (PR #99045)
jasonmolenda wrote: I put up a PR that avoids a memory region starting at address 0, and also clarifies the documentation of the qMemoryRegionInfo packet to state that permissions: are required for all memory regions that are accessible by the inferior process. A memory region with no `permissions:` key means that it is not an accessible region of memory. I put this PR up at https://github.com/llvm/llvm-project/pull/100288 @dlav-sc what do you think? I know you've moved past this issue by implementing memory-allocation and fixing your qMemoryRegionInfo packet response, but I wanted to get a final fix before we all walk away from this PR. https://github.com/llvm/llvm-project/pull/99045 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Don't use a vm addr range starting at 0 for local memory (PR #100288)
jasonmolenda wrote: I created this PR as an alternative approach to the issue reported in https://github.com/llvm/llvm-project/pull/99045 https://github.com/llvm/llvm-project/pull/100288 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Don't use a vm addr range starting at 0 for local memory (PR #100288)
https://github.com/jasonmolenda created
https://github.com/llvm/llvm-project/pull/100288
When an inferior stub cannot allocate memory for lldb, and lldb needs to store
the result of expressions, it will do it in lldb's own memory range ("host
memory"). But it needs to find a virtual address range that is not used in the
inferior process. It tries to use the qMemoryRegionInfo gdb remote serial
protocol packet to find a range that is inaccessible, starting at address 0 and
moving up the size of each region.
If the first region found at address 0 is inaccessible, lldb will use the
address range starting at 0 to mean "read lldb's host memory, not the process
memory", and programs that crash with a null dereference will have poor
behavior.
This patch skips consideration of a memory region that starts at address 0.
I also clarified the documentation of qMemoryRegionInfo to make it clear that
the stub is required to provide permissions for a memory range that is
accessable, it is not an optional key in this response. This issue was
originally found by a stub that did not list permissions in its response, and
lldb treated the first region returned as the one it would use. (the stub also
didn't support the memory-allocate packet)
>From 28439443b9e44a242e59bf1cdd114486380d54fc Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Tue, 23 Jul 2024 18:54:31 -0700
Subject: [PATCH] [lldb] Don't use a vm addr range starting at 0 for local
memory
When an inferior stub cannot allocate memory for lldb, and lldb
needs to store the result of expressions, it will do it in lldb's
own memory range ("host memory"). But it needs to find a virtual
address range that is not used in the inferior process. It tries
to use the qMemoryRegionInfo gdb remote serial protocol packet to
find a range that is inaccessible, starting at address 0 and moving
up the size of each region.
If the first region found at address 0 is inaccessible, lldb will
use the address range starting at 0 to mean "read lldb's host memory,
not the process memory", and programs that crash with a null
dereference will have poor behavior.
This patch skips consideration of a memory region that starts at
address 0.
I also clarified the documentation of qMemoryRegionInfo to make it
clear that the stub is required to provide permissions for a memory
range that is accessable, it is not an optional key in this response.
This issue was originally found by a stub that did not list
permissions, and lldb treated the first region returned as the one
it would use.
---
lldb/docs/resources/lldbgdbremote.md | 8 +++-
lldb/source/Expression/IRMemoryMap.cpp | 17 -
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/lldb/docs/resources/lldbgdbremote.md
b/lldb/docs/resources/lldbgdbremote.md
index 7076a75032dae..5cac3736337a8 100644
--- a/lldb/docs/resources/lldbgdbremote.md
+++ b/lldb/docs/resources/lldbgdbremote.md
@@ -1403,6 +1403,12 @@ For instance, with a macOS process which has nothing
mapped in the first
The lack of `permissions:` indicates that none of read/write/execute are valid
for this region.
+The stub must include `permissions:` key-value on all memory ranges
+that are valid to access in the inferior process -- the lack of
+`permissions:` means that this is an inaccessible (no page table
+entries exist, in a system using VM) memory range. If a stub cannot
+determine actual permissions, return `rwx`.
+
**Priority To Implement:** Medium
This is nice to have, but it isn't necessary. It helps LLDB
@@ -2434,4 +2440,4 @@ The `0x` prefixes are optional - like most of the
gdb-remote packets,
omitting them will work fine; these numbers are always base 16.
The length of the payload is not provided. A reliable, 8-bit clean,
-transport layer is assumed.
\ No newline at end of file
+transport layer is assumed.
diff --git a/lldb/source/Expression/IRMemoryMap.cpp
b/lldb/source/Expression/IRMemoryMap.cpp
index de631370bb048..bb3e4e0a9a9fc 100644
--- a/lldb/source/Expression/IRMemoryMap.cpp
+++ b/lldb/source/Expression/IRMemoryMap.cpp
@@ -84,7 +84,7 @@ lldb::addr_t IRMemoryMap::FindSpace(size_t size) {
// any allocations. Otherwise start at the beginning of memory.
if (m_allocations.empty()) {
-ret = 0x0;
+ret = 0;
} else {
auto back = m_allocations.rbegin();
lldb::addr_t addr = back->first;
@@ -116,10 +116,17 @@ lldb::addr_t IRMemoryMap::FindSpace(size_t size) {
Status err = process_sp->GetMemoryRegionInfo(ret, region_info);
if (err.Success()) {
while (true) {
-if (region_info.GetReadable() != MemoryRegionInfo::OptionalBool::eNo ||
-region_info.GetWritable() != MemoryRegionInfo::OptionalBool::eNo ||
-region_info.GetExecutable() !=
-MemoryRegionInfo::OptionalBool::eNo) {
+if (region_info.GetRange().GetRangeBase() == 0 &&
+region_info.GetRange().GetRangeEnd() - 1 < end_of_memory) {
+ //
[Lldb-commits] [lldb] [lldb] Don't crash when attaching to pid and no binaries found (PR #100287)
https://github.com/jasonmolenda closed https://github.com/llvm/llvm-project/pull/100287 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Don't crash when attaching to pid and no binaries found (PR #100287)
https://github.com/jasonmolenda created
https://github.com/llvm/llvm-project/pull/100287
There is a narrow window during process launch on macOS where lldb can attach
and no binaries will be seen as loaded in the process (none reported by libdyld
SPI). A year ago I made changes to set the new-binary-loaded breakpoint
correctly despite this. But we've seen a crash when this combination is seen,
where CommandObjectProcessAttach::DoExecute assumed there was at least one
binary registered in the Target. Fix that.
Also fix two FileSpec API uses from when we didn't have a GetPath() method that
returned a std::string, and was copying the filepaths into fixed length
buffers. All of this code was from ~14 years ago when we didn't have that API.
rdar://131631627
>From 4cec9c8e71229cb47ab0d0536f7e654caf05d2f1 Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Tue, 23 Jul 2024 17:33:24 -0700
Subject: [PATCH] [lldb] Don't crash when attaching to pid and no binaries
found
There is a narrow window during process launch on macOS where
lldb can attach and no binaries will be seen as loaded in the
process (none reported by libdyld SPI). A year ago I made
changes to set the new-binary-loaded breakpoint correctly despite
this. But we've seen a crash when this combination is seen, where
CommandObjectProcessAttach::DoExecute assumed there was at least
one binary registered in the Target. Fix that.
Also fix two FileSpec API uses from when we didn't have a GetPath()
method that returned a std::string, and was copying the filepaths
into fixed length buffers. All of this code was from ~14 years ago
when we didn't have that API.
rdar://131631627
---
lldb/source/Commands/CommandObjectProcess.cpp | 18 --
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp
b/lldb/source/Commands/CommandObjectProcess.cpp
index 50695af556939..e605abdb3c771 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -369,25 +369,23 @@ class CommandObjectProcessAttach : public
CommandObjectProcessLaunchOrAttach {
// Okay, we're done. Last step is to warn if the executable module has
// changed:
-char new_path[PATH_MAX];
ModuleSP new_exec_module_sp(target->GetExecutableModule());
if (!old_exec_module_sp) {
// We might not have a module if we attached to a raw pid...
if (new_exec_module_sp) {
-new_exec_module_sp->GetFileSpec().GetPath(new_path, PATH_MAX);
-result.AppendMessageWithFormat("Executable module set to \"%s\".\n",
- new_path);
+result.AppendMessageWithFormat(
+"Executable binary set to \"%s\".\n",
+new_exec_module_sp->GetFileSpec().GetPath().c_str());
}
+} else if (!new_exec_module_sp) {
+ result.AppendWarningWithFormat("No executable binary.");
} else if (old_exec_module_sp->GetFileSpec() !=
new_exec_module_sp->GetFileSpec()) {
- char old_path[PATH_MAX];
-
- old_exec_module_sp->GetFileSpec().GetPath(old_path, PATH_MAX);
- new_exec_module_sp->GetFileSpec().GetPath(new_path, PATH_MAX);
result.AppendWarningWithFormat(
- "Executable module changed from \"%s\" to \"%s\".\n", old_path,
- new_path);
+ "Executable binary changed from \"%s\" to \"%s\".\n",
+ old_exec_module_sp->GetFileSpec().GetPath().c_str(),
+ new_exec_module_sp->GetFileSpec().GetPath().c_str());
}
if (!old_arch_spec.IsValid()) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][debugserver] Retry sleep(0.25s) when interrupted (PR #99962)
https://github.com/jasonmolenda closed https://github.com/llvm/llvm-project/pull/99962 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][debugserver] Retry sleep(0.25s) when interrupted (PR #99962)
jasonmolenda wrote: Ah, Jonas points out that I've fixed the wrong sleep. He has a patch he'll put up for the real problem. This does not fix a known problem. https://github.com/llvm/llvm-project/pull/99962 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][debugserver] Retry sleep(0.25s) when interrupted (PR #99962)
https://github.com/jasonmolenda edited https://github.com/llvm/llvm-project/pull/99962 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][debugserver] Retry sleep(0.25s) when interrupted (PR #99962)
https://github.com/jasonmolenda created
https://github.com/llvm/llvm-project/pull/99962
After lldb has ptrace()'ed the process, it sleeps for 0.25 seconds before
trying to pause the process. We see logging that in some rare cases, this
sleep is interrupted and the pause fails, the attach fails. usleep(3) says
that the only errno it will return is EINTR if interrupted, so retry on that
case. I capped it at four retries so the process can't hang infinitely if it's
being spammed with asynchronous signals.
I don't know what environment setup results in this behavior, it's only rarely
seen by end users and we see evidence, after the fact of it, in logging.
rdar://131602093
>From 01255f7c906c9b5995b0f33c420c7c7eb707bddf Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Mon, 22 Jul 2024 14:55:04 -0700
Subject: [PATCH] [lldb][debugserver] Retry sleep(0.25s) when interrupted
After lldb has ptrace()'ed the process, it sleeps for 0.25 seconds
before trying to pause the process. We see logging that in some
rare cases, this sleep is interrupted and the pause fails. usleep(3)
says that the only errno it will return is EINTR if interrupted,
so retry on that case. I capped it at four retries so the process
can't hang infinitely if it's being spammed with asynchronous
signals.
I don't know what environment setup results in this behavior,
it's only rarely seen by end users and we see evidence, after
the factof it, in logging.
rdar://131602093
---
lldb/tools/debugserver/source/MacOSX/MachProcess.mm | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
index cbe3c5459e91f..89c031fdc75d6 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
+++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
@@ -2846,9 +2846,12 @@ static uint64_t bits(uint64_t value, uint32_t msbit,
uint32_t lsbit) {
if (err.Success()) {
m_flags |= eMachProcessFlagsAttached;
// Sleep a bit to let the exception get received and set our process
- // status
- // to stopped.
- ::usleep(25);
+ // status to stopped.
+ int max_retry = 4;
+ do {
+errno = 0;
+usleep(25); // If our 0.25 second sleep is interrupted, redo
+ } while (errno == EINTR && --max_retry > 0);
DNBLog("[LaunchAttach] (%d) Done napping after ptrace(PT_ATTACHEXC)'ing",
getpid());
DNBLogThreadedIf(LOG_PROCESS, "successfully attached to pid %d", pid);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] IRMemoryMap zero address mapping fix (PR #99045)
jasonmolenda wrote: Ah, but I see my misunderstanding as I look at what debugserver returns. It uses "no permissions" to indicate a memory range that is unmapped -- ``` < 23> send packet: $qMemoryRegionInfo:0#44 < 27> read packet: $start:0;size:1;#00 < 31> send packet: $qMemoryRegionInfo:1#c5 < 67> read packet: $start:1;size:4000;permissions:rx;dirty-pages:1;#00 < 31> send packet: $qMemoryRegionInfo:14000#c9 < 57> read packet: $start:14000;size:4000;permissions:r;dirty-pages:;#00 < 31> send packet: $qMemoryRegionInfo:18000#cd < 32> read packet: $start:18000;size:404000;#00 < 31> send packet: $qMemoryRegionInfo:10040c000#fc < 198> read packet: $start:10040c000;size:4;permissions:rw;dirty-pages:10040c000,10041,100414000,100418000,10041c000,10042,100424000,100428000,10042c000,10043,100434000,100438000,10043c000,10044;#00 ``` 0x18000-0x00010040c000 is a free address range in this example, and what this loop should choose if it had to find an unused memory range. That conflicts with the behavior that this PR started with originally, where no permissions were returned for any memory regions --- which looks like the way debugserver reports a free memory region. (The low 4 GB segment at the start of my output above is the PAGEZERO empty segment of virtual address space on 64-bit darwin processes) https://github.com/llvm/llvm-project/pull/99045 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] IRMemoryMap zero address mapping fix (PR #99045)
jasonmolenda wrote:
I haven't tested it (or even tried to compile it, lol) but I think this loop
might be expressable as simply
```
MemoryRegionInfo region_info;
while (process_sp->GetMemoryRegionInfo(ret, region_info) == err.Success() &&
region_info.GetRange().GetRangeEnd() - 1 < end_of_memory) {
// Don't ever choose a memory region starting at address 0,
// it will conflict with programs that deference null pointers.
if (ret == 0) {
ret = region_info.GetRange().GetRangeEnd();
continue;
}
// A memory region that is inaccessible, and large enough, is a good
// choice.
if (region_info.GetReadable() != MemoryRegionInfo::OptionalBool::eNo &&
region_info.GetWritable() != MemoryRegionInfo::OptionalBool::eNo &&
region_info.GetExecutable() != MemoryRegionInfo::OptionalBool::eNo) {
if (ret + size < region_info.GetRange().GetRangeEnd()) {
return ret;
}
}
// Get the next region.
ret = region_info.GetRange().GetRangeEnd();
}
```
I dropped two behaviors here - one is that it would emit a unique assert if
qMemoryRegionInfo worked once, but failed for a different address. The second
is that I think the old code would try to combine consecutive memory regions to
make one block large enough to satisfy the size requirement (not sure it was
doing this correctly).
https://github.com/llvm/llvm-project/pull/99045
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
jasonmolenda wrote: > FYI, I also ran into failures due to this change, when debugging on Windows > (with DWARF). See the run in > https://github.com/mstorsjo/actions-test/actions/runs/10020326811 vs > https://github.com/mstorsjo/actions-test/actions/runs/10020393197. Thanks for > reverting; hopefully it’s the same root cause as on other platforms. oooh that 10020393197 is surprising. It looks like we hit a breakpoint, did `finish` up until a recursive function and it resumed execution without stopping. I haven't seen that one before, and not sure how I could have introduced it. I'll def need to look at my ProcessWindows changes and again see if I can't figure out what might be going on. Aleksandr ran a slightly earlier version of the windows changes against the lldb testsuite and didn't uncover any problems, but it didn't include this test file. @mstorsjo is this test source file on-line somewhere, or can you paste it into a comment? I'll try to repo on linux or macos too, maybe it is a unique corner case unrelated to Windows that is a problem. (unlikely, but you never know if we might get lucky). Thanks for the heads-up. https://github.com/llvm/llvm-project/pull/96260 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
jasonmolenda wrote: I've reverted this change until I can debug the failures found on the CI bots. Two debuginfo dexter tests failed, likely because the way stepping and breakpoints works is now different. If you're sitting at a BreakpointSite which has not yet executed, previously lldb would overwrite the stop reason with "breakpoint hit" even though it had not been hit. Now, you'll have your actual stop reason -- and when you resume, you'll hit the breakpoint and stop again. If you 'continue', it stops again. If you 'next/step', it stops again and your next/step is still on the thread plan stack, so if you 'continue', you're going to first do the next/step that was on the thread stack, and then you can do your other command. I've spent a lot of time thinking about any other approach to this, and they all have bad outcomes that we don't want to add. In short, I expect the debuginfo tests are continuing from a breakpoint and not expecting the breakpoint to be hit. The ubuntu-arm bot failed TestConsecutiveBreakpoints.py and TestConsecutiveBreakpoints.py, but it seems like the only failure text was like "process exited -9" or something. I may need to re-land the patch with a lot more logging enabled on these two, to narrow down what this bot is seeing. I let it run an extra cycle and it failed in the same spot again, so it wasn't a oneoff error. https://github.com/llvm/llvm-project/pull/96260 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] 52c08d7 - Revert "[lldb] Change lldb's breakpoint handling behavior (#96260)"
Author: Jason Molenda
Date: 2024-07-19T18:43:53-07:00
New Revision: 52c08d7ffd380f4abd819c20bec76252272f6337
URL:
https://github.com/llvm/llvm-project/commit/52c08d7ffd380f4abd819c20bec76252272f6337
DIFF:
https://github.com/llvm/llvm-project/commit/52c08d7ffd380f4abd819c20bec76252272f6337.diff
LOG: Revert "[lldb] Change lldb's breakpoint handling behavior (#96260)"
This reverts commit 05f0e86cc895181b3d2210458c78938f83353002.
The debuginfo dexter tests are failing, probably because the way
stepping over breakpoints has changed with my patches. And there
are two API tests fails on the ubuntu-arm (32-bit) bot. I'll need
to investigate both of these, neither has an obvious failure reason.
Added:
Modified:
lldb/include/lldb/Target/Thread.h
lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
lldb/source/Target/StopInfo.cpp
lldb/source/Target/Thread.cpp
lldb/test/API/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py
lldb/test/API/functionalities/breakpoint/step_over_breakpoint/TestStepOverBreakpoint.py
Removed:
diff --git a/lldb/include/lldb/Target/Thread.h
b/lldb/include/lldb/Target/Thread.h
index d3e429e9256df..2ff1f50d497e2 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -129,7 +129,6 @@ class Thread : public std::enable_shared_from_this,
register_backup_sp; // You need to restore the registers, of course...
uint32_t current_inlined_depth;
lldb::addr_t current_inlined_pc;
-lldb::addr_t stopped_at_unexecuted_bp;
};
/// Constructor
@@ -379,26 +378,6 @@ class Thread : public std::enable_shared_from_this,
virtual void SetQueueLibdispatchQueueAddress(lldb::addr_t dispatch_queue_t)
{}
- /// When a thread stops at an enabled BreakpointSite that has not executed,
- /// the Process plugin should call SetThreadStoppedAtUnexecutedBP(pc).
- /// If that BreakpointSite was actually triggered (the instruction was
- /// executed, for a software breakpoint), regardless of whether the
- /// breakpoint is valid for this thread, SetThreadHitBreakpointSite()
- /// should be called to record that fact.
- ///
- /// Depending on the structure of the Process plugin, it may be easiest
- /// to call SetThreadStoppedAtUnexecutedBP(pc) unconditionally when at
- /// a BreakpointSite, and later when it is known that it was triggered,
- /// SetThreadHitBreakpointSite() can be called. These two methods
- /// overwrite the same piece of state in the Thread, the last one
- /// called on a Thread wins.
- void SetThreadStoppedAtUnexecutedBP(lldb::addr_t pc) {
-m_stopped_at_unexecuted_bp = pc;
- }
- void SetThreadHitBreakpointSite() {
-m_stopped_at_unexecuted_bp = LLDB_INVALID_ADDRESS;
- }
-
/// Whether this Thread already has all the Queue information cached or not
///
/// A Thread may be associated with a libdispatch work Queue at a given
@@ -1333,9 +1312,6 @@ class Thread : public
std::enable_shared_from_this,
bool m_should_run_before_public_stop; // If this thread has "stop others"
// private work to do, then it will
// set this.
- lldb::addr_t m_stopped_at_unexecuted_bp; // Set to the address of a
breakpoint
- // instruction that we have not yet
- // hit, but will hit when we resume.
const uint32_t m_index_id; ///< A unique 1 based index assigned to each
thread
/// for easy UI/command line access.
lldb::RegisterContextSP m_reg_context_sp; ///< The register context for this
diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index f1853be12638e..25cee369d7ee3 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -488,6 +488,38 @@ const char *StopInfoMachException::GetDescription() {
return m_description.c_str();
}
+static StopInfoSP GetStopInfoForHardwareBP(Thread , Target *target,
+ uint32_t exc_data_count,
+ uint64_t exc_sub_code,
+ uint64_t exc_sub_sub_code) {
+ // Try hardware watchpoint.
+ if (target) {
+// The exc_sub_code indicates the data break address.
+WatchpointResourceSP wp_rsrc_sp =
+target->GetProcessSP()->GetWatchpointResourceList().FindByAddress(
+(addr_t)exc_sub_code);
+if (wp_rsrc_sp &&
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
https://github.com/jasonmolenda closed https://github.com/llvm/llvm-project/pull/96260 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
jasonmolenda wrote: The pre-merge linux-x86 test builds both failed in TestConcurrentVFork.py after I updated the branch to current main. I don't know if there's a bot issue or a main issue. I tried repoing on aarch64 ubuntu; that test is skipped on aarch64 but I removed the skips and it runs without fails in my VM. I'm going to try landing this and see what happens; will revert if bots blow up. I've tried to test all the combinations I have access to here, and Aleksandr's testing on windows has been invaluable, but it's still possible it'll need a little reworking. https://github.com/llvm/llvm-project/pull/96260 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/96260 >From 9b541e6a035635e26c6a24eca022de8552fa4c17 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Thu, 20 Jun 2024 17:53:17 -0700 Subject: [PATCH 1/8] [lldb] Change lldb's breakpoint handling behavior lldb today has two rules: When a thread stops at a BreakpointSite, we set the thread's StopReason to be "breakpoint hit" (regardless if we've actually hit the breakpoint, or if we've merely stopped *at* the breakpoint instruction/point and haven't tripped it yet). And second, when resuming a process, any thread sitting at a BreakpointSite is silently stepped over the BreakpointSite -- because we've already flagged the breakpoint hit when we stopped there originally. In this patch, I change lldb to only set a thread's stop reason to breakpoint-hit when we've actually executed the instruction/triggered the breakpoint. When we resume, we only silently step past a BreakpointSite that we've registered as hit. We preserve this state across inferior function calls that the user may do while stopped, etc. Also, when a user adds a new breakpoint at $pc while stopped, or changes $pc to be the address of a BreakpointSite, we will silently step past that breakpoint when the process resumes. This is purely a UX call, I don't think there's any person who wants to set a breakpoint at $pc and then hit it immediately on resuming. One non-intuitive UX from this change, but I'm convinced it is necessary: If you're stopped at a BreakpointSite that has not yet executed, you `stepi`, you will hit the breakpoint and the pc will not yet advance. This thread has not completed its stepi, and the thread plan is still on the stack. If you then `continue` the thread, lldb will now stop and say, "instruction step completed", one instruction past the BreakpointSite. You can continue a second time to resume execution. I discussed this with Jim, and trying to paper over this behavior will lead to more complicated scenarios behaving non-intuitively. And mostly it's the testsuite that was trying to instruction step past a breakpoint and getting thrown off -- and I changed those tests to expect the new behavior. The bugs driving this change are all from lldb dropping the real stop reason for a thread and setting it to breakpoint-hit when that was not the case. Jim hit one where we have an aarch64 watchpoint that triggers one instruction before a BreakpointSite. On this arch we are notified of the watchpoint hit after the instruction has been unrolled -- we disable the watchpoint, instruction step, re-enable the watchpoint and collect the new value. But now we're on a BreakpointSite so the watchpoint-hit stop reason is lost. Another was reported by ZequanWu in https://discourse.llvm.org/t/lldb-unable-to-break-at-start/78282 we attach to/launch a process with the pc at a BreakpointSite and misbehave. Caroline Tice mentioned it is also a problem they've had with putting a breakpoint on _dl_debug_state. The change to each Process plugin that does execution control is that 1. If we've stopped at a BreakpointSite (whether we hit it or not), we call Thread::SetThreadStoppedAtBreakpointSite(pc) to record the state at the point when the thread stopped. (so we can detect newly-added breakpoints, or when the pc is changed to an instruction that is a BreakpointSite) 2. When we have actually hit a breakpoint, and it is enabled for this thread, we call Thread::SetThreadHitBreakpointAtAddr(pc) so we know that it should be silently stepped past when we resume execution. When resuming, we silently step over a breakpoint if we've hit it, or if it is newly added (or the pc was changed to an existing BreakpointSite). The biggest set of changes is to StopInfoMachException where we translate a Mach Exception into a stop reason. The Mach exception codes differ in a few places depending on the target (unambiguously), and I didn't want to duplicate the new code for each target so I've tested what mach exceptions we get for each action on each target, and reorganized StopInfoMachException::CreateStopReasonWithMachException to document these possible values, and handle them without specializing based on the target arch. rdar://123942164 --- lldb/include/lldb/Target/Thread.h | 29 ++ .../Process/Utility/StopInfoMachException.cpp | 296 +++--- .../Process/Windows/Common/ProcessWindows.cpp | 16 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 118 +++ .../Process/scripted/ScriptedThread.cpp | 9 + lldb/source/Target/Thread.cpp | 17 +- .../TestConsecutiveBreakpoints.py | 26 +- .../TestStepOverBreakpoint.py | 6 +- 8 files changed, 235 insertions(+), 282 deletions(-) diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h index c17bddf4d98b8..1e1aead896018 100644 --- a/lldb/include/lldb/Target/Thread.h +++
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/96260 >From 9b541e6a035635e26c6a24eca022de8552fa4c17 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Thu, 20 Jun 2024 17:53:17 -0700 Subject: [PATCH 1/8] [lldb] Change lldb's breakpoint handling behavior lldb today has two rules: When a thread stops at a BreakpointSite, we set the thread's StopReason to be "breakpoint hit" (regardless if we've actually hit the breakpoint, or if we've merely stopped *at* the breakpoint instruction/point and haven't tripped it yet). And second, when resuming a process, any thread sitting at a BreakpointSite is silently stepped over the BreakpointSite -- because we've already flagged the breakpoint hit when we stopped there originally. In this patch, I change lldb to only set a thread's stop reason to breakpoint-hit when we've actually executed the instruction/triggered the breakpoint. When we resume, we only silently step past a BreakpointSite that we've registered as hit. We preserve this state across inferior function calls that the user may do while stopped, etc. Also, when a user adds a new breakpoint at $pc while stopped, or changes $pc to be the address of a BreakpointSite, we will silently step past that breakpoint when the process resumes. This is purely a UX call, I don't think there's any person who wants to set a breakpoint at $pc and then hit it immediately on resuming. One non-intuitive UX from this change, but I'm convinced it is necessary: If you're stopped at a BreakpointSite that has not yet executed, you `stepi`, you will hit the breakpoint and the pc will not yet advance. This thread has not completed its stepi, and the thread plan is still on the stack. If you then `continue` the thread, lldb will now stop and say, "instruction step completed", one instruction past the BreakpointSite. You can continue a second time to resume execution. I discussed this with Jim, and trying to paper over this behavior will lead to more complicated scenarios behaving non-intuitively. And mostly it's the testsuite that was trying to instruction step past a breakpoint and getting thrown off -- and I changed those tests to expect the new behavior. The bugs driving this change are all from lldb dropping the real stop reason for a thread and setting it to breakpoint-hit when that was not the case. Jim hit one where we have an aarch64 watchpoint that triggers one instruction before a BreakpointSite. On this arch we are notified of the watchpoint hit after the instruction has been unrolled -- we disable the watchpoint, instruction step, re-enable the watchpoint and collect the new value. But now we're on a BreakpointSite so the watchpoint-hit stop reason is lost. Another was reported by ZequanWu in https://discourse.llvm.org/t/lldb-unable-to-break-at-start/78282 we attach to/launch a process with the pc at a BreakpointSite and misbehave. Caroline Tice mentioned it is also a problem they've had with putting a breakpoint on _dl_debug_state. The change to each Process plugin that does execution control is that 1. If we've stopped at a BreakpointSite (whether we hit it or not), we call Thread::SetThreadStoppedAtBreakpointSite(pc) to record the state at the point when the thread stopped. (so we can detect newly-added breakpoints, or when the pc is changed to an instruction that is a BreakpointSite) 2. When we have actually hit a breakpoint, and it is enabled for this thread, we call Thread::SetThreadHitBreakpointAtAddr(pc) so we know that it should be silently stepped past when we resume execution. When resuming, we silently step over a breakpoint if we've hit it, or if it is newly added (or the pc was changed to an existing BreakpointSite). The biggest set of changes is to StopInfoMachException where we translate a Mach Exception into a stop reason. The Mach exception codes differ in a few places depending on the target (unambiguously), and I didn't want to duplicate the new code for each target so I've tested what mach exceptions we get for each action on each target, and reorganized StopInfoMachException::CreateStopReasonWithMachException to document these possible values, and handle them without specializing based on the target arch. rdar://123942164 --- lldb/include/lldb/Target/Thread.h | 29 ++ .../Process/Utility/StopInfoMachException.cpp | 296 +++--- .../Process/Windows/Common/ProcessWindows.cpp | 16 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 118 +++ .../Process/scripted/ScriptedThread.cpp | 9 + lldb/source/Target/Thread.cpp | 17 +- .../TestConsecutiveBreakpoints.py | 26 +- .../TestStepOverBreakpoint.py | 6 +- 8 files changed, 235 insertions(+), 282 deletions(-) diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h index c17bddf4d98b8..1e1aead896018 100644 --- a/lldb/include/lldb/Target/Thread.h +++
[Lldb-commits] [lldb] [lldb/Target] Add GetStartSymbol method to DynamicLoader plugins (PR #99673)
https://github.com/jasonmolenda approved this pull request. LGTM. https://github.com/llvm/llvm-project/pull/99673 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/Target] Add GetStartSymbol method to DynamicLoader plugins (PR #99673)
@@ -10,9 +10,11 @@ #define LLDB_TARGET_DYNAMICLOADER_H #include "lldb/Core/PluginInterface.h" +#include "lldb/Symbol/Symbol.h" #include "lldb/Utility/FileSpec.h" #include "lldb/Utility/Status.h" #include "lldb/Utility/UUID.h" +#include "lldb/Utility/UnimplementedError.h" jasonmolenda wrote: We can remove this header file include now. https://github.com/llvm/llvm-project/pull/99673 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/Target] Add GetStartSymbol method to DynamicLoader plugins (PR #99673)
https://github.com/jasonmolenda edited https://github.com/llvm/llvm-project/pull/99673 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/Target] Add GetStartSymbol method to DynamicLoader plugins (PR #99673)
@@ -329,6 +330,11 @@ class DynamicLoader : public PluginInterface {
/// safe to call certain APIs or SPIs.
virtual bool IsFullyInitialized() { return true; }
+ /// Return the `start` function \b symbol in the dynamic loader module.
+ virtual llvm::Expected GetStartSymbol() {
jasonmolenda wrote:
I would DEFINITELY not bank on this assumption. The world of environments a
debugger needs to support is vast.
https://github.com/llvm/llvm-project/pull/99673
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/Target] Add GetStartSymbol method to DynamicLoader plugins (PR #99673)
@@ -329,6 +330,11 @@ class DynamicLoader : public PluginInterface {
/// safe to call certain APIs or SPIs.
virtual bool IsFullyInitialized() { return true; }
+ /// Return the `start` function \b symbol in the dynamic loader module.
+ virtual llvm::Expected GetStartSymbol() {
jasonmolenda wrote:
Should this be an llvm::Expected? It's only going to be defined for
DynamicLoaderDarwin today. If some generic code wanted to use this, where it
may not be applicable or defined for another DynamicLoader plugin, it'll need
to consume the UnimplementedError. Given the optionality of it, a
std::optional seems more natural to me maybe?
https://github.com/llvm/llvm-project/pull/99673
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/Target] Add GetStartSymbol method to DynamicLoader plugins (PR #99673)
@@ -329,6 +330,11 @@ class DynamicLoader : public PluginInterface {
/// safe to call certain APIs or SPIs.
virtual bool IsFullyInitialized() { return true; }
+ /// Return the `start` function \b symbol in the dynamic loader module.
jasonmolenda wrote:
The meaning of this is clear in a Darwin environment, but I wonder if we should
explain it a little more, like "This is the symbol the process will begin
executing with `process launch --stop-at-entry`". I don't feel strongly about
this.
https://github.com/llvm/llvm-project/pull/99673
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [lldb][RISCV] function calls support in lldb expressions (PR #99336)
@@ -0,0 +1,58 @@
+//===--- DirectToIndirectFCR.h - RISC-V specific pass
-===//
+//
+// 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
+//
+//===--===//
+
+#pragma once
+
+#include "lldb/lldb-types.h"
+
+#include "llvm/IR/Instructions.h"
+#include "llvm/Pass.h"
+
+namespace lldb_private {
+
+class ExecutionContext;
+
+// During the lldb expression execution lldb wraps a user expression, jittes
+// fabricated code and then puts it into the stack memory. Thus, if user tried
+// to make a function call there will be a jump from a stack address to a code
+// sections's address. RISC-V Architecture doesn't have a large code model yet
+// and can make only a +-2GiB jumps, but in 64-bit architecture a distance
+// between stack addresses and code sections's addresses is longer. Therefore,
+// relocations resolver obtains an invalid address. To avoid such problem, this
+// pass should be used. It replaces function calls with appropriate function's
+// addresses explicitly. By doing so it removes relocations related to function
+// calls. This pass should be cosidered as temprorary solution until a large
+// code model will be approved.
+class DirectToIndirectFCR : public llvm::FunctionPass {
jasonmolenda wrote:
It's a minor detail, and I never work on the llvm side of the codebase so this
is probably just my own ignorance, but: I have no idea what DirectToIndirectFCR
means, beyond the explanation above. I'm sure to people more familiar with
llvm/clang this is clear.
https://github.com/llvm/llvm-project/pull/99336
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [lldb][RISCV] function calls support in lldb expressions (PR #99336)
@@ -0,0 +1,58 @@
+//===--- DirectToIndirectFCR.h - RISC-V specific pass
-===//
+//
+// 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
+//
+//===--===//
+
+#pragma once
+
+#include "lldb/lldb-types.h"
+
+#include "llvm/IR/Instructions.h"
+#include "llvm/Pass.h"
+
+namespace lldb_private {
+
+class ExecutionContext;
+
+// During the lldb expression execution lldb wraps a user expression, jittes
+// fabricated code and then puts it into the stack memory. Thus, if user tried
jasonmolenda wrote:
I don't think this is quite true. lldb asks the remote stub to allocate memory
with read+write permissions for storing the arguments, and read+execute
permissions for the jitted code, e.g.
```
(lldb) p (int)isdigit('1')
< 13> send packet: $_M1000,rx#83
< 13> read packet: $18000#00
< 13> send packet: $_M1000,rw#82
< 13> read packet: $1c000#00
```
It would be surprising (to me) if the stub returned memory regions on the
stack, because lldb is allowed to reuse these regions throughout the debug
session, and we can't have the inferior process overwriting them.
You are correct that the allocated regions may be too far from the function(s)
they'll need to call, and the same problem you describe below could exist. I
don't mean to focus on the "stack memory" part of this, but I think it will
confuse people when they read it.
https://github.com/llvm/llvm-project/pull/99336
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] IRMemoryMap zero address mapping fix (PR #99045)
jasonmolenda wrote:
I think we should change these checks to look for an explicitly inaccessible
memory region, like
```
if (region_info.GetReadable() != MemoryRegionInfo::OptionalBool::eNo &&
region_info.GetWritable() != MemoryRegionInfo::OptionalBool::eNo &&
region_info.GetExecutable() !=
MemoryRegionInfo::OptionalBool::eNo) {
// This region is inaccessible, if it is large enough, use this
address.
if (ret + size < region_info.GetRange().GetRangeEnd())
return ret;
else
ret = region_info.GetRange().GetRangeEnd();
// keep searching
```
and I also do think there is value in adding a special case for address 0.
Even if we have an inaddressable memory block at address 0 which should be
eligible for shadowing with lldb's host memory, using that is a poor choice
because people crash at address 0 all the time and we don't want references to
that address finding the IRMemoryMap host side memory values.
```
} else if (ret == 0) {
// Don't put our host-side memory block at virtual address 0 even if
it
// seems eligible, we don't want to confuse things when a program
// crashes accessing that address, a common way of crashing.
ret = region_info.GetRange().GetRangeEnd();
```
https://github.com/llvm/llvm-project/pull/99045
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [lldb][RISCV] function calls support in lldb expressions (PR #99336)
jasonmolenda wrote: As for getting to the jitted code, of course lldb sets the pc directly so that initial switch from user code to jitted code region is fine. But getting from the jitted wrapper function to the user functions we need to call would require JALR instructions? https://github.com/llvm/llvm-project/pull/99336 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [lldb][RISCV] function calls support in lldb expressions (PR #99336)
jasonmolenda wrote: A quick look at the RISCV ISA and it says that the JAL instruction is pc relative and can jump +/- 1MB. JALR is also pc-relative but it gets the upper 20 bits from a general purpose register and it includes 12 low bits in its instruction encoding. I know almost nothing about rv32, but is this what you mean by a Large Memory Model, the use of the JALR instruction? I didn't find the part that talks about how JALR works in rv64, but it seems likely it can specify an arbitrary 64-bit address? https://github.com/llvm/llvm-project/pull/99336 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] IRMemoryMap zero address mapping fix (PR #99045)
jasonmolenda wrote:
I'd describe this patch as handling the case where a remote stub cannot jit
memory and has a qMemoryRegion info packet which does not specify permissions.
The patch you reference makes jitted code expressions work, but I think
IRMemoryMap::FindSpec only needs to find space in the inferior virtual address
space -- allocate memory -- and that was already working, wasn't it? Also,
your before & after packets show that you've got permissions returned for your
qMemoryRegionInfo responses now, which you don't before -- you won't hit this
failure because of that change.
One thing I'd say is that this patch is looking for "is this a region with
read, write, or execute permissions", so it can skip that. It does this with,
```
if (region_info.GetReadable() != MemoryRegionInfo::OptionalBool::eNo ||
region_info.GetWritable() != MemoryRegionInfo::OptionalBool::eNo ||
region_info.GetExecutable() !=
MemoryRegionInfo::OptionalBool::eNo) {
ret = region_info.GetRange().GetRangeEnd();
/// keep iterating
```
but `MemoryRegionInfo::OptionalBool` can be `eDontKnow` which it is here. I
think this conditional would be more clearly expressed as
```
bool all_permissions_unknown = region_info.GetReadable() ==
MemoryRegionInfo::OptionalBool::eDontKnow &&
region_info.GetWritable() == MemoryRegionInfo::OptionalBool::eDontKnow &&
region_info.GetExecutable() == MemoryRegionInfo::OptionalBool::eDontKnow;
bool any_permissions_yes = region_info.GetReadable() ==
MemoryRegionInfo::OptionalBool::eYes
region_info.GetWritable() == MemoryRegionInfo::OptionalBool::eYes
region_info.GetExecutable() == MemoryRegionInfo::OptionalBool::eYes;
if (all_permissions_unknown || any_permissions_yes)
ret = region_info.GetRange().GetRangeEnd();
/// keep iterating
```
or more simply, change the while (true) loop to first check if all permissions
are No in a region,
```
if (region_info.GetReadable() != MemoryRegionInfo::OptionalBool::eNo &&
region_info.GetWritable() != MemoryRegionInfo::OptionalBool::eNo &&
region_info.GetExecutable() !=
MemoryRegionInfo::OptionalBool::eNo) {
if (ret + size < region_info.GetRange().GetRangeEnd())
return ret;
else
ret = region_info.GetRange().GetRangeEnd();
// keep searching
```
https://github.com/llvm/llvm-project/pull/99045
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [lldb][RISCV] function calls support in lldb expressions (PR #99336)
@@ -0,0 +1,58 @@
+//===--- DirectToIndirectFCR.h - RISC-V specific pass
-===//
+//
+// 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
+//
+//===--===//
+
+#pragma once
+
+#include "lldb/lldb-types.h"
+
+#include "llvm/IR/Instructions.h"
+#include "llvm/Pass.h"
+
+namespace lldb_private {
+
+class ExecutionContext;
+
+// During the lldb expression execution lldb wraps a user expression, jittes
+// fabricated code and then puts it into the stack memory. Thus, if user tried
+// to make a function call there will be a jump from a stack address to a code
+// sections's address. RISC-V Architecture doesn't have a large code model yet
+// and can make only a +-2GiB jumps, but in 64-bit architecture a distance
+// between stack addresses and code sections's addresses is longer. Therefore,
+// relocations resolver obtains an invalid address. To avoid such problem, this
+// pass should be used. It replaces function calls with appropriate function's
+// addresses explicitly. By doing so it removes relocations related to function
+// calls. This pass should be cosidered as temprorary solution until a large
jasonmolenda wrote:
tiny nit: "cosidered", "temprorary". I do this all the time myself, just
happened to see them.
https://github.com/llvm/llvm-project/pull/99336
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
https://github.com/jasonmolenda edited https://github.com/llvm/llvm-project/pull/96260 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/96260 >From 9b541e6a035635e26c6a24eca022de8552fa4c17 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Thu, 20 Jun 2024 17:53:17 -0700 Subject: [PATCH 1/7] [lldb] Change lldb's breakpoint handling behavior lldb today has two rules: When a thread stops at a BreakpointSite, we set the thread's StopReason to be "breakpoint hit" (regardless if we've actually hit the breakpoint, or if we've merely stopped *at* the breakpoint instruction/point and haven't tripped it yet). And second, when resuming a process, any thread sitting at a BreakpointSite is silently stepped over the BreakpointSite -- because we've already flagged the breakpoint hit when we stopped there originally. In this patch, I change lldb to only set a thread's stop reason to breakpoint-hit when we've actually executed the instruction/triggered the breakpoint. When we resume, we only silently step past a BreakpointSite that we've registered as hit. We preserve this state across inferior function calls that the user may do while stopped, etc. Also, when a user adds a new breakpoint at $pc while stopped, or changes $pc to be the address of a BreakpointSite, we will silently step past that breakpoint when the process resumes. This is purely a UX call, I don't think there's any person who wants to set a breakpoint at $pc and then hit it immediately on resuming. One non-intuitive UX from this change, but I'm convinced it is necessary: If you're stopped at a BreakpointSite that has not yet executed, you `stepi`, you will hit the breakpoint and the pc will not yet advance. This thread has not completed its stepi, and the thread plan is still on the stack. If you then `continue` the thread, lldb will now stop and say, "instruction step completed", one instruction past the BreakpointSite. You can continue a second time to resume execution. I discussed this with Jim, and trying to paper over this behavior will lead to more complicated scenarios behaving non-intuitively. And mostly it's the testsuite that was trying to instruction step past a breakpoint and getting thrown off -- and I changed those tests to expect the new behavior. The bugs driving this change are all from lldb dropping the real stop reason for a thread and setting it to breakpoint-hit when that was not the case. Jim hit one where we have an aarch64 watchpoint that triggers one instruction before a BreakpointSite. On this arch we are notified of the watchpoint hit after the instruction has been unrolled -- we disable the watchpoint, instruction step, re-enable the watchpoint and collect the new value. But now we're on a BreakpointSite so the watchpoint-hit stop reason is lost. Another was reported by ZequanWu in https://discourse.llvm.org/t/lldb-unable-to-break-at-start/78282 we attach to/launch a process with the pc at a BreakpointSite and misbehave. Caroline Tice mentioned it is also a problem they've had with putting a breakpoint on _dl_debug_state. The change to each Process plugin that does execution control is that 1. If we've stopped at a BreakpointSite (whether we hit it or not), we call Thread::SetThreadStoppedAtBreakpointSite(pc) to record the state at the point when the thread stopped. (so we can detect newly-added breakpoints, or when the pc is changed to an instruction that is a BreakpointSite) 2. When we have actually hit a breakpoint, and it is enabled for this thread, we call Thread::SetThreadHitBreakpointAtAddr(pc) so we know that it should be silently stepped past when we resume execution. When resuming, we silently step over a breakpoint if we've hit it, or if it is newly added (or the pc was changed to an existing BreakpointSite). The biggest set of changes is to StopInfoMachException where we translate a Mach Exception into a stop reason. The Mach exception codes differ in a few places depending on the target (unambiguously), and I didn't want to duplicate the new code for each target so I've tested what mach exceptions we get for each action on each target, and reorganized StopInfoMachException::CreateStopReasonWithMachException to document these possible values, and handle them without specializing based on the target arch. rdar://123942164 --- lldb/include/lldb/Target/Thread.h | 29 ++ .../Process/Utility/StopInfoMachException.cpp | 296 +++--- .../Process/Windows/Common/ProcessWindows.cpp | 16 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 118 +++ .../Process/scripted/ScriptedThread.cpp | 9 + lldb/source/Target/Thread.cpp | 17 +- .../TestConsecutiveBreakpoints.py | 26 +- .../TestStepOverBreakpoint.py | 6 +- 8 files changed, 235 insertions(+), 282 deletions(-) diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h index c17bddf4d98b8..1e1aead896018 100644 --- a/lldb/include/lldb/Target/Thread.h +++
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
@@ -229,6 +229,17 @@ bool ScriptedThread::CalculateStopInfo() {
LLVM_PRETTY_FUNCTION, "Failed to get scripted thread stop info.",
error,
LLDBLog::Thread);
+ // If we're at a BreakpointSite, mark that we stopped there and
jasonmolenda wrote:
This comment was meant to be on ProcessGDBRemote.cpp where I removed a section
of code for a thread that had no stop signal/reason, but is sitting at a
BreakpointSite. The old comment on this:
```
// If a thread is stopped at a breakpoint site, set that as the stop
// reason even if it hasn't executed the breakpoint instruction yet.
// We will silently step over the breakpoint when we resume execution
// and miss the fact that this thread hit the breakpoint.
```
That's the old stepping behavior. The new stepping behavior is if a thread
stops at a BreakpointSite but has no signal/breakpoint hit reason, we call
`Thread::SetThreadStoppedAtUnexecutedBP(pc)`. This is done at the top of
`ProcessGDBRemote::SetThreadStopInfo` already if the thread stops at a
BreakpointSite but hasn't hit it, so there's no need for this block to still be
around.
https://github.com/llvm/llvm-project/pull/96260
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] IRMemoryMap zero address mapping fix (PR #99045)
jasonmolenda wrote: To make sure I'm clear: I don't have a problem with the basic idea of the change, although we could comment what is going on more clearly, and I'm curious about that qMemoryRegionInfo packet. But it looks like you're connecting to a device which can't allocate memory through gdb remote serial protocol (common for jtag/vm type stubs), but can report memory region info (much less common, it's an lldb extension I believe), and the memory region at address 0 is reported as being inaccessible (reasonable). So the IRMemoryMap is trying to use address 0 and that overlaps with actual addresses seen in the process, leading to confusion. Most similar environments, that can't allocate memory, don't support qMemoryRegionInfo so we pick one of our bogus addresses ranges (in higher memory) to get a better chance of avoiding actual program addresses. https://github.com/llvm/llvm-project/pull/99045 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
https://github.com/jasonmolenda closed https://github.com/llvm/llvm-project/pull/98845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/98845
>From cead9ae6de627ee64fb58a829fa3485f526a0afc Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Sun, 14 Jul 2024 16:59:51 -0700
Subject: [PATCH 1/8] [lldb] progressive progress reporting for darwin
kernel/firmware
When doing firmware/kernel debugging, it is frequent that
binaries and debug info need to be retrieved / downloaded,
and the lack of progress reports made for a poor experience,
with lldb seemingly hung while downloading things over the
network. This PR adds progress reports to the critical sites
for these use cases.
---
lldb/source/Core/DynamicLoader.cpp| 27 +++--
.../DynamicLoaderDarwinKernel.cpp | 39 ---
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp
b/lldb/source/Core/DynamicLoader.cpp
index 7871be6fc451d..a59136381c23b 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/ModuleList.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Symbol/ObjectFile.h"
#include "lldb/Target/MemoryRegionInfo.h"
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+Progress progress_memread("Reading load commands from memory",
+ prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
-if (memory_module_sp)
+if (memory_module_sp) {
uuid = memory_module_sp->GetUUID();
+ if (uuid.IsValid()) {
+prog_str << " ";
+prog_str << uuid.GetAsString();
+ }
+}
}
ModuleSpec module_spec;
module_spec.GetUUID() = uuid;
FileSpec name_filespec(name);
- if (FileSystem::Instance().Exists(name_filespec))
-module_spec.GetFileSpec() = name_filespec;
if (uuid.IsValid()) {
+Progress progress("Locating external symbol file",
+ prog_str.GetString().str());
+
// Has lldb already seen a module with this UUID?
+// Or have external lookup enabled in DebugSymbols on macOS.
if (!module_sp)
error = ModuleList::GetSharedModule(module_spec, module_sp, nullptr,
nullptr, nullptr);
diff --git
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 8d83937aab668..93eb1e7b9dea9 100644
---
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
@@ -757,6 +758,23 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+std::unique_ptr progress_up;
+if (IsKernel()) {
+ StreamString prog_str;
+ // 'mach_kernel' is a fake name we make up to find kernels
+ // that were located by the local filesystem scan.
+ if (GetName() != "mach_kernel")
+prog_str << GetName() << " ";
+ if (GetUUID().IsValid())
+prog_str << GetUUID().GetAsString() << " ";
+ if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(GetLoadAddress());
+ }
+ progress_up = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+}
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
@@ -1058,12 +1076,9 @@ void
DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
}
}
}
-
-if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS) {
- if (!m_kernel.LoadImageUsingMemoryModule(m_process)) {
+if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS)
+ if (!m_kernel.LoadImageUsingMemoryModule(m_process))
m_kernel.LoadImageAtFileAddress(m_process);
- }
-}
// The operating system plugin gets loaded and initialized in
// LoadImageUsingMemoryModule when we discover the
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/98845
>From cead9ae6de627ee64fb58a829fa3485f526a0afc Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Sun, 14 Jul 2024 16:59:51 -0700
Subject: [PATCH 1/7] [lldb] progressive progress reporting for darwin
kernel/firmware
When doing firmware/kernel debugging, it is frequent that
binaries and debug info need to be retrieved / downloaded,
and the lack of progress reports made for a poor experience,
with lldb seemingly hung while downloading things over the
network. This PR adds progress reports to the critical sites
for these use cases.
---
lldb/source/Core/DynamicLoader.cpp| 27 +++--
.../DynamicLoaderDarwinKernel.cpp | 39 ---
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp
b/lldb/source/Core/DynamicLoader.cpp
index 7871be6fc451d..a59136381c23b 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/ModuleList.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Symbol/ObjectFile.h"
#include "lldb/Target/MemoryRegionInfo.h"
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+Progress progress_memread("Reading load commands from memory",
+ prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
-if (memory_module_sp)
+if (memory_module_sp) {
uuid = memory_module_sp->GetUUID();
+ if (uuid.IsValid()) {
+prog_str << " ";
+prog_str << uuid.GetAsString();
+ }
+}
}
ModuleSpec module_spec;
module_spec.GetUUID() = uuid;
FileSpec name_filespec(name);
- if (FileSystem::Instance().Exists(name_filespec))
-module_spec.GetFileSpec() = name_filespec;
if (uuid.IsValid()) {
+Progress progress("Locating external symbol file",
+ prog_str.GetString().str());
+
// Has lldb already seen a module with this UUID?
+// Or have external lookup enabled in DebugSymbols on macOS.
if (!module_sp)
error = ModuleList::GetSharedModule(module_spec, module_sp, nullptr,
nullptr, nullptr);
diff --git
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 8d83937aab668..93eb1e7b9dea9 100644
---
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
@@ -757,6 +758,23 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+std::unique_ptr progress_up;
+if (IsKernel()) {
+ StreamString prog_str;
+ // 'mach_kernel' is a fake name we make up to find kernels
+ // that were located by the local filesystem scan.
+ if (GetName() != "mach_kernel")
+prog_str << GetName() << " ";
+ if (GetUUID().IsValid())
+prog_str << GetUUID().GetAsString() << " ";
+ if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(GetLoadAddress());
+ }
+ progress_up = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+}
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
@@ -1058,12 +1076,9 @@ void
DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
}
}
}
-
-if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS) {
- if (!m_kernel.LoadImageUsingMemoryModule(m_process)) {
+if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS)
+ if (!m_kernel.LoadImageUsingMemoryModule(m_process))
m_kernel.LoadImageAtFileAddress(m_process);
- }
-}
// The operating system plugin gets loaded and initialized in
// LoadImageUsingMemoryModule when we discover the
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
@@ -633,171 +613,142 @@ StopInfoSP
StopInfoMachException::CreateStopReasonWithMachException(
}
break;
+ // [exc_type, exc_code, exc_sub_code, exc_sub_sub_code]
+ //
+ // Instruction step:
+ // [6, 1, 0]
+ // Intel KDP [6, 3, ??]
+ // armv7 [6, 0x102, ] Same as software breakpoint!
+ //
+ // Software breakpoint:
+ // x86 [6, 2, 0]
+ // Intel KDP [6, 2, ]
+ // arm64 [6, 1, ]
+ // armv7 [6, 0x102, ] Same as instruction step!
+ //
+ // Hardware breakpoint:
+ // x86 [6, 1, , 0]
+ // x86/Rosetta not implemented, see software breakpoint
+ // arm64 [6, 1, ]
+ // armv7 not implemented, see software breakpoint
+ //
+ // Hardware watchpoint:
+ // x86 [6, 1, , 0] (both Intel hw and Rosetta)
+ // arm64 [6, 0x102, , 0]
+ // armv7 [6, 0x102, , 0]
+ //
+ // arm64 BRK instruction (imm arg not reflected in the ME)
+ // [ 6, 1, ]
+ //
+ // In order of codes mach exceptions:
+ // [6, 1, 0] - instruction step
+ // [6, 1, ] - hardware breakpoint or watchpoint
+ //
+ // [6, 2, 0] - software breakpoint
+ // [6, 2, ] - software breakpoint
+ //
+ // [6, 3] - instruction step
+ //
+ // [6, 0x102, ] armv7 instruction step
+ // [6, 0x102, ] armv7 software breakpoint
+ // [6, 0x102, , 0] arm64/armv7 watchpoint
case 6: // EXC_BREAKPOINT
{
-bool is_actual_breakpoint = false;
-bool is_trace_if_actual_breakpoint_missing = false;
-switch (cpu) {
-case llvm::Triple::x86:
-case llvm::Triple::x86_64:
- if (exc_code == 1) // EXC_I386_SGL
- {
-if (!exc_sub_code) {
- // This looks like a plain trap.
- // Have to check if there is a breakpoint here as well. When you
- // single-step onto a trap, the single step stops you not to trap.
- // Since we also do that check below, let's just use that logic.
- is_actual_breakpoint = true;
- is_trace_if_actual_breakpoint_missing = true;
-} else {
- if (StopInfoSP stop_info =
- GetStopInfoForHardwareBP(thread, target, exc_data_count,
- exc_sub_code, exc_sub_sub_code))
-return stop_info;
-}
- } else if (exc_code == 2 || // EXC_I386_BPT
- exc_code == 3) // EXC_I386_BPTFLT
- {
-// KDP returns EXC_I386_BPTFLT for trace breakpoints
-if (exc_code == 3)
- is_trace_if_actual_breakpoint_missing = true;
-
-is_actual_breakpoint = true;
-if (!pc_already_adjusted)
- pc_decrement = 1;
- }
- break;
-
-case llvm::Triple::arm:
-case llvm::Triple::thumb:
- if (exc_code == 0x102) // EXC_ARM_DA_DEBUG
- {
-// LWP_TODO: We need to find the WatchpointResource that matches
-// the address, and evaluate its Watchpoints.
-
-// It's a watchpoint, then, if the exc_sub_code indicates a
-// known/enabled data break address from our watchpoint list.
-lldb::WatchpointSP wp_sp;
-if (target)
- wp_sp = target->GetWatchpointList().FindByAddress(
- (lldb::addr_t)exc_sub_code);
-if (wp_sp && wp_sp->IsEnabled()) {
- return StopInfo::CreateStopReasonWithWatchpointID(thread,
-wp_sp->GetID());
-} else {
- is_actual_breakpoint = true;
- is_trace_if_actual_breakpoint_missing = true;
-}
- } else if (exc_code == 1) // EXC_ARM_BREAKPOINT
- {
-is_actual_breakpoint = true;
-is_trace_if_actual_breakpoint_missing = true;
- } else if (exc_code == 0) // FIXME not EXC_ARM_BREAKPOINT but a kernel
-// is currently returning this so accept it
-// as indicating a breakpoint until the
-// kernel is fixed
- {
-is_actual_breakpoint = true;
-is_trace_if_actual_breakpoint_missing = true;
- }
- break;
+bool stopped_by_hitting_breakpoint = false;
+bool stopped_by_completing_stepi = false;
+bool stopped_watchpoint = false;
+std::optional value;
+
+// exc_code 1
+if (exc_code == 1 && exc_sub_code == 0)
+ stopped_by_completing_stepi = true;
+if (exc_code == 1 && exc_sub_code != 0) {
+ stopped_by_hitting_breakpoint = true;
+ stopped_watchpoint = true;
+ value = exc_sub_code;
+}
-case llvm::Triple::aarch64_32:
-case llvm::Triple::aarch64: {
- // xnu describes three things with type EXC_BREAKPOINT:
- //
- // exc_code 0x102 [EXC_ARM_DA_DEBUG], exc_sub_code addr-of-insn
- // Watchpoint access. exc_sub_code is the address of the
- // instruction which trigged the watchpoint trap.
- // debugserver may add the watchpoint number that was triggered
- // in exc_sub_sub_code.
- //
- // exc_code 1
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
@@ -633,171 +613,142 @@ StopInfoSP
StopInfoMachException::CreateStopReasonWithMachException(
}
break;
+ // [exc_type, exc_code, exc_sub_code, exc_sub_sub_code]
+ //
+ // Instruction step:
+ // [6, 1, 0]
+ // Intel KDP [6, 3, ??]
+ // armv7 [6, 0x102, ] Same as software breakpoint!
+ //
+ // Software breakpoint:
+ // x86 [6, 2, 0]
+ // Intel KDP [6, 2, ]
+ // arm64 [6, 1, ]
+ // armv7 [6, 0x102, ] Same as instruction step!
+ //
+ // Hardware breakpoint:
+ // x86 [6, 1, , 0]
+ // x86/Rosetta not implemented, see software breakpoint
+ // arm64 [6, 1, ]
+ // armv7 not implemented, see software breakpoint
+ //
+ // Hardware watchpoint:
+ // x86 [6, 1, , 0] (both Intel hw and Rosetta)
+ // arm64 [6, 0x102, , 0]
+ // armv7 [6, 0x102, , 0]
+ //
+ // arm64 BRK instruction (imm arg not reflected in the ME)
+ // [ 6, 1, ]
+ //
+ // In order of codes mach exceptions:
+ // [6, 1, 0] - instruction step
+ // [6, 1, ] - hardware breakpoint or watchpoint
+ //
+ // [6, 2, 0] - software breakpoint
+ // [6, 2, ] - software breakpoint
+ //
+ // [6, 3] - instruction step
+ //
+ // [6, 0x102, ] armv7 instruction step
+ // [6, 0x102, ] armv7 software breakpoint
+ // [6, 0x102, , 0] arm64/armv7 watchpoint
case 6: // EXC_BREAKPOINT
{
-bool is_actual_breakpoint = false;
-bool is_trace_if_actual_breakpoint_missing = false;
-switch (cpu) {
-case llvm::Triple::x86:
-case llvm::Triple::x86_64:
- if (exc_code == 1) // EXC_I386_SGL
- {
-if (!exc_sub_code) {
- // This looks like a plain trap.
- // Have to check if there is a breakpoint here as well. When you
- // single-step onto a trap, the single step stops you not to trap.
- // Since we also do that check below, let's just use that logic.
- is_actual_breakpoint = true;
- is_trace_if_actual_breakpoint_missing = true;
-} else {
- if (StopInfoSP stop_info =
- GetStopInfoForHardwareBP(thread, target, exc_data_count,
- exc_sub_code, exc_sub_sub_code))
-return stop_info;
-}
- } else if (exc_code == 2 || // EXC_I386_BPT
- exc_code == 3) // EXC_I386_BPTFLT
- {
-// KDP returns EXC_I386_BPTFLT for trace breakpoints
-if (exc_code == 3)
- is_trace_if_actual_breakpoint_missing = true;
-
-is_actual_breakpoint = true;
-if (!pc_already_adjusted)
- pc_decrement = 1;
- }
- break;
-
-case llvm::Triple::arm:
-case llvm::Triple::thumb:
- if (exc_code == 0x102) // EXC_ARM_DA_DEBUG
- {
-// LWP_TODO: We need to find the WatchpointResource that matches
-// the address, and evaluate its Watchpoints.
-
-// It's a watchpoint, then, if the exc_sub_code indicates a
-// known/enabled data break address from our watchpoint list.
-lldb::WatchpointSP wp_sp;
-if (target)
- wp_sp = target->GetWatchpointList().FindByAddress(
- (lldb::addr_t)exc_sub_code);
-if (wp_sp && wp_sp->IsEnabled()) {
- return StopInfo::CreateStopReasonWithWatchpointID(thread,
-wp_sp->GetID());
-} else {
- is_actual_breakpoint = true;
- is_trace_if_actual_breakpoint_missing = true;
-}
- } else if (exc_code == 1) // EXC_ARM_BREAKPOINT
- {
-is_actual_breakpoint = true;
-is_trace_if_actual_breakpoint_missing = true;
- } else if (exc_code == 0) // FIXME not EXC_ARM_BREAKPOINT but a kernel
-// is currently returning this so accept it
-// as indicating a breakpoint until the
-// kernel is fixed
- {
-is_actual_breakpoint = true;
-is_trace_if_actual_breakpoint_missing = true;
- }
- break;
+bool stopped_by_hitting_breakpoint = false;
+bool stopped_by_completing_stepi = false;
+bool stopped_watchpoint = false;
+std::optional value;
+
+// exc_code 1
+if (exc_code == 1 && exc_sub_code == 0)
+ stopped_by_completing_stepi = true;
+if (exc_code == 1 && exc_sub_code != 0) {
+ stopped_by_hitting_breakpoint = true;
+ stopped_watchpoint = true;
+ value = exc_sub_code;
+}
-case llvm::Triple::aarch64_32:
-case llvm::Triple::aarch64: {
- // xnu describes three things with type EXC_BREAKPOINT:
- //
- // exc_code 0x102 [EXC_ARM_DA_DEBUG], exc_sub_code addr-of-insn
- // Watchpoint access. exc_sub_code is the address of the
- // instruction which trigged the watchpoint trap.
- // debugserver may add the watchpoint number that was triggered
- // in exc_sub_sub_code.
- //
- // exc_code 1
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
@@ -2545,6 +2546,11 @@ ModuleSP Process::ReadModuleFromMemory(const FileSpec
_spec,
ModuleSP module_sp(new Module(file_spec, ArchSpec()));
if (module_sp) {
Status error;
+std::unique_ptr progress_up;
+if (!GetCoreFile())
jasonmolenda wrote:
Oh whoops, my bad on that one, thanks. I'll change to use this instead, it's
clearer.
https://github.com/llvm/llvm-project/pull/98845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/98845
>From cead9ae6de627ee64fb58a829fa3485f526a0afc Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Sun, 14 Jul 2024 16:59:51 -0700
Subject: [PATCH 1/6] [lldb] progressive progress reporting for darwin
kernel/firmware
When doing firmware/kernel debugging, it is frequent that
binaries and debug info need to be retrieved / downloaded,
and the lack of progress reports made for a poor experience,
with lldb seemingly hung while downloading things over the
network. This PR adds progress reports to the critical sites
for these use cases.
---
lldb/source/Core/DynamicLoader.cpp| 27 +++--
.../DynamicLoaderDarwinKernel.cpp | 39 ---
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp
b/lldb/source/Core/DynamicLoader.cpp
index 7871be6fc451d..a59136381c23b 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/ModuleList.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Symbol/ObjectFile.h"
#include "lldb/Target/MemoryRegionInfo.h"
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+Progress progress_memread("Reading load commands from memory",
+ prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
-if (memory_module_sp)
+if (memory_module_sp) {
uuid = memory_module_sp->GetUUID();
+ if (uuid.IsValid()) {
+prog_str << " ";
+prog_str << uuid.GetAsString();
+ }
+}
}
ModuleSpec module_spec;
module_spec.GetUUID() = uuid;
FileSpec name_filespec(name);
- if (FileSystem::Instance().Exists(name_filespec))
-module_spec.GetFileSpec() = name_filespec;
if (uuid.IsValid()) {
+Progress progress("Locating external symbol file",
+ prog_str.GetString().str());
+
// Has lldb already seen a module with this UUID?
+// Or have external lookup enabled in DebugSymbols on macOS.
if (!module_sp)
error = ModuleList::GetSharedModule(module_spec, module_sp, nullptr,
nullptr, nullptr);
diff --git
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 8d83937aab668..93eb1e7b9dea9 100644
---
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
@@ -757,6 +758,23 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+std::unique_ptr progress_up;
+if (IsKernel()) {
+ StreamString prog_str;
+ // 'mach_kernel' is a fake name we make up to find kernels
+ // that were located by the local filesystem scan.
+ if (GetName() != "mach_kernel")
+prog_str << GetName() << " ";
+ if (GetUUID().IsValid())
+prog_str << GetUUID().GetAsString() << " ";
+ if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(GetLoadAddress());
+ }
+ progress_up = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+}
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
@@ -1058,12 +1076,9 @@ void
DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
}
}
}
-
-if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS) {
- if (!m_kernel.LoadImageUsingMemoryModule(m_process)) {
+if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS)
+ if (!m_kernel.LoadImageUsingMemoryModule(m_process))
m_kernel.LoadImageAtFileAddress(m_process);
- }
-}
// The operating system plugin gets loaded and initialized in
// LoadImageUsingMemoryModule when we discover the
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/98845
>From cead9ae6de627ee64fb58a829fa3485f526a0afc Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Sun, 14 Jul 2024 16:59:51 -0700
Subject: [PATCH 1/5] [lldb] progressive progress reporting for darwin
kernel/firmware
When doing firmware/kernel debugging, it is frequent that
binaries and debug info need to be retrieved / downloaded,
and the lack of progress reports made for a poor experience,
with lldb seemingly hung while downloading things over the
network. This PR adds progress reports to the critical sites
for these use cases.
---
lldb/source/Core/DynamicLoader.cpp| 27 +++--
.../DynamicLoaderDarwinKernel.cpp | 39 ---
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp
b/lldb/source/Core/DynamicLoader.cpp
index 7871be6fc451d..a59136381c23b 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/ModuleList.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Symbol/ObjectFile.h"
#include "lldb/Target/MemoryRegionInfo.h"
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+Progress progress_memread("Reading load commands from memory",
+ prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
-if (memory_module_sp)
+if (memory_module_sp) {
uuid = memory_module_sp->GetUUID();
+ if (uuid.IsValid()) {
+prog_str << " ";
+prog_str << uuid.GetAsString();
+ }
+}
}
ModuleSpec module_spec;
module_spec.GetUUID() = uuid;
FileSpec name_filespec(name);
- if (FileSystem::Instance().Exists(name_filespec))
-module_spec.GetFileSpec() = name_filespec;
if (uuid.IsValid()) {
+Progress progress("Locating external symbol file",
+ prog_str.GetString().str());
+
// Has lldb already seen a module with this UUID?
+// Or have external lookup enabled in DebugSymbols on macOS.
if (!module_sp)
error = ModuleList::GetSharedModule(module_spec, module_sp, nullptr,
nullptr, nullptr);
diff --git
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 8d83937aab668..93eb1e7b9dea9 100644
---
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
@@ -757,6 +758,23 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+std::unique_ptr progress_up;
+if (IsKernel()) {
+ StreamString prog_str;
+ // 'mach_kernel' is a fake name we make up to find kernels
+ // that were located by the local filesystem scan.
+ if (GetName() != "mach_kernel")
+prog_str << GetName() << " ";
+ if (GetUUID().IsValid())
+prog_str << GetUUID().GetAsString() << " ";
+ if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(GetLoadAddress());
+ }
+ progress_up = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+}
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
@@ -1058,12 +1076,9 @@ void
DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
}
}
}
-
-if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS) {
- if (!m_kernel.LoadImageUsingMemoryModule(m_process)) {
+if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS)
+ if (!m_kernel.LoadImageUsingMemoryModule(m_process))
m_kernel.LoadImageAtFileAddress(m_process);
- }
-}
// The operating system plugin gets loaded and initialized in
// LoadImageUsingMemoryModule when we discover the
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
@@ -2545,6 +2546,11 @@ ModuleSP Process::ReadModuleFromMemory(const FileSpec
_spec,
ModuleSP module_sp(new Module(file_spec, ArchSpec()));
if (module_sp) {
Status error;
+std::unique_ptr progress_up;
+if (!GetCoreFile())
jasonmolenda wrote:
The name is a little misleading, `IsLiveDebugSession` is only false for
`PostMortemProcess`. Maybe we should have a `Process::IsCorefileProcess` or
something, or I could put a comment there about what I'm doing.
https://github.com/llvm/llvm-project/pull/98845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] IRMemoryMap zero address mapping fix (PR #99045)
jasonmolenda wrote: I don't think this change is correct, or I don't understand what it is trying to do. When we're connected to a stub that can allocate memory in the target, none of this code is executed. So lldb-server/debugserver will not hit this. We send `qMemoryRegionInfo` packets to look for a block of memory that is unallocated, or has permissions no-read+no-write+no-execute. The memory region scan starts at address 0 and moves up by the size of each memory region, looking for `size` bytes between allocated memory regions, and returns that address if it is found. So I'm guessing we have (1) a target that cannot allocate memory, (2) a target that supported `qMemoryRegionInfo`, a non-accessible block of memory at address 0? (did I read this right?) and in that case, we end up picking address 0 as our host-side virtual address+memory buffer for expression data storage. Can you show what your `qMemoryRegionInfo` results are for address 0 on this target, so I can confirm I didn't misread/misunderstand the situation here? https://github.com/llvm/llvm-project/pull/99045 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][RISCV] function prologue backtrace fix (PR #99043)
https://github.com/jasonmolenda approved this pull request. Agreed, this fix is correct, thanks for the PR. The register numbering in the UnwindPlan can be any eRegisterKind, but it does need to be self consistent, and this was not. Do you have permissions to merge this PR? I can do that for you if not. https://github.com/llvm/llvm-project/pull/99043 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/98845
>From cead9ae6de627ee64fb58a829fa3485f526a0afc Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Sun, 14 Jul 2024 16:59:51 -0700
Subject: [PATCH 1/4] [lldb] progressive progress reporting for darwin
kernel/firmware
When doing firmware/kernel debugging, it is frequent that
binaries and debug info need to be retrieved / downloaded,
and the lack of progress reports made for a poor experience,
with lldb seemingly hung while downloading things over the
network. This PR adds progress reports to the critical sites
for these use cases.
---
lldb/source/Core/DynamicLoader.cpp| 27 +++--
.../DynamicLoaderDarwinKernel.cpp | 39 ---
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp
b/lldb/source/Core/DynamicLoader.cpp
index 7871be6fc451d..a59136381c23b 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/ModuleList.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Symbol/ObjectFile.h"
#include "lldb/Target/MemoryRegionInfo.h"
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+Progress progress_memread("Reading load commands from memory",
+ prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
-if (memory_module_sp)
+if (memory_module_sp) {
uuid = memory_module_sp->GetUUID();
+ if (uuid.IsValid()) {
+prog_str << " ";
+prog_str << uuid.GetAsString();
+ }
+}
}
ModuleSpec module_spec;
module_spec.GetUUID() = uuid;
FileSpec name_filespec(name);
- if (FileSystem::Instance().Exists(name_filespec))
-module_spec.GetFileSpec() = name_filespec;
if (uuid.IsValid()) {
+Progress progress("Locating external symbol file",
+ prog_str.GetString().str());
+
// Has lldb already seen a module with this UUID?
+// Or have external lookup enabled in DebugSymbols on macOS.
if (!module_sp)
error = ModuleList::GetSharedModule(module_spec, module_sp, nullptr,
nullptr, nullptr);
diff --git
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 8d83937aab668..93eb1e7b9dea9 100644
---
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
@@ -757,6 +758,23 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+std::unique_ptr progress_up;
+if (IsKernel()) {
+ StreamString prog_str;
+ // 'mach_kernel' is a fake name we make up to find kernels
+ // that were located by the local filesystem scan.
+ if (GetName() != "mach_kernel")
+prog_str << GetName() << " ";
+ if (GetUUID().IsValid())
+prog_str << GetUUID().GetAsString() << " ";
+ if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(GetLoadAddress());
+ }
+ progress_up = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+}
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
@@ -1058,12 +1076,9 @@ void
DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
}
}
}
-
-if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS) {
- if (!m_kernel.LoadImageUsingMemoryModule(m_process)) {
+if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS)
+ if (!m_kernel.LoadImageUsingMemoryModule(m_process))
m_kernel.LoadImageAtFileAddress(m_process);
- }
-}
// The operating system plugin gets loaded and initialized in
// LoadImageUsingMemoryModule when we discover the
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
jasonmolenda wrote: Thanks for the helpful feedback @clayborg pushed an update. https://github.com/llvm/llvm-project/pull/98845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/98845
>From cead9ae6de627ee64fb58a829fa3485f526a0afc Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Sun, 14 Jul 2024 16:59:51 -0700
Subject: [PATCH 1/3] [lldb] progressive progress reporting for darwin
kernel/firmware
When doing firmware/kernel debugging, it is frequent that
binaries and debug info need to be retrieved / downloaded,
and the lack of progress reports made for a poor experience,
with lldb seemingly hung while downloading things over the
network. This PR adds progress reports to the critical sites
for these use cases.
---
lldb/source/Core/DynamicLoader.cpp| 27 +++--
.../DynamicLoaderDarwinKernel.cpp | 39 ---
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp
b/lldb/source/Core/DynamicLoader.cpp
index 7871be6fc451d..a59136381c23b 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/ModuleList.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Symbol/ObjectFile.h"
#include "lldb/Target/MemoryRegionInfo.h"
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+Progress progress_memread("Reading load commands from memory",
+ prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
-if (memory_module_sp)
+if (memory_module_sp) {
uuid = memory_module_sp->GetUUID();
+ if (uuid.IsValid()) {
+prog_str << " ";
+prog_str << uuid.GetAsString();
+ }
+}
}
ModuleSpec module_spec;
module_spec.GetUUID() = uuid;
FileSpec name_filespec(name);
- if (FileSystem::Instance().Exists(name_filespec))
-module_spec.GetFileSpec() = name_filespec;
if (uuid.IsValid()) {
+Progress progress("Locating external symbol file",
+ prog_str.GetString().str());
+
// Has lldb already seen a module with this UUID?
+// Or have external lookup enabled in DebugSymbols on macOS.
if (!module_sp)
error = ModuleList::GetSharedModule(module_spec, module_sp, nullptr,
nullptr, nullptr);
diff --git
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 8d83937aab668..93eb1e7b9dea9 100644
---
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
@@ -757,6 +758,23 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+std::unique_ptr progress_up;
+if (IsKernel()) {
+ StreamString prog_str;
+ // 'mach_kernel' is a fake name we make up to find kernels
+ // that were located by the local filesystem scan.
+ if (GetName() != "mach_kernel")
+prog_str << GetName() << " ";
+ if (GetUUID().IsValid())
+prog_str << GetUUID().GetAsString() << " ";
+ if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(GetLoadAddress());
+ }
+ progress_up = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+}
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
@@ -1058,12 +1076,9 @@ void
DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
}
}
}
-
-if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS) {
- if (!m_kernel.LoadImageUsingMemoryModule(m_process)) {
+if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS)
+ if (!m_kernel.LoadImageUsingMemoryModule(m_process))
m_kernel.LoadImageAtFileAddress(m_process);
- }
-}
// The operating system plugin gets loaded and initialized in
// LoadImageUsingMemoryModule when we discover the
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+prog_str << "at 0x";
+prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+Progress progress_memread("Reading load commands from memory",
+ prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
-if (memory_module_sp)
+if (memory_module_sp) {
uuid = memory_module_sp->GetUUID();
+ if (uuid.IsValid()) {
+prog_str << " ";
+prog_str << uuid.GetAsString();
+ }
+}
}
ModuleSpec module_spec;
module_spec.GetUUID() = uuid;
FileSpec name_filespec(name);
- if (FileSystem::Instance().Exists(name_filespec))
-module_spec.GetFileSpec() = name_filespec;
if (uuid.IsValid()) {
+Progress progress("Locating external symbol file",
+ prog_str.GetString().str());
+
// Has lldb already seen a module with this UUID?
+// Or have external lookup enabled in DebugSymbols on macOS.
jasonmolenda wrote:
It's a little tricky because we do call ModuleList::GetSharedModule which will
return a module if it's already in the global module cache, and it will call
into the DebugSymbols framework on macOS, where it might call an external
program to do a slow copy of a binary to the local computer. But after that,
it then goes on to call `LocateExecutableSymbolFile`
`LocateExecutableObjectFile` looking in known local filesystem locations. If
that fails, then it calls `DownloadObjectAndSymbolFile
(force_symbol_search=true)` which can call out to an external program to do a
slow copy of a binary to the local computer.
I wanted to log one message to cover possibly all of these being run.
https://github.com/llvm/llvm-project/pull/98845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+prog_str << "at 0x";
+prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+Progress progress_memread("Reading load commands from memory",
jasonmolenda wrote:
Good suggestion. I think I'll conditionalize this progress with a `if
(!process->GetCoreFile())` -- reading a binary out of a local corefile will be
very fast. Over a live gdb-remote connection, not so much.
https://github.com/llvm/llvm-project/pull/98845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
@@ -757,11 +758,32 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+StreamString prog_str;
+// 'mach_kernel' is a fake name we make up to find kernels
+// that were located by the local filesystem scan.
+if (GetName() != "mach_kernel")
+ prog_str << GetName() << " ";
+if (GetUUID().IsValid())
+ prog_str << GetUUID().GetAsString() << " ";
+if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+ prog_str << "at 0x";
+ prog_str.PutHex64(GetLoadAddress());
+}
+std::unique_ptr progress_wp;
+if (IsKernel())
+ progress_wp = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+else
+ progress_wp = std::make_unique("Loading kext",
+ prog_str.GetString().str());
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
module_spec.GetUUID() = m_uuid;
- module_spec.GetArchitecture() = target.GetArchitecture();
+ if (!m_uuid.IsValid())
+module_spec.GetArchitecture() = target.GetArchitecture();
jasonmolenda wrote:
just to be clear, it's almost always the "environment" or "os" part of the
triple, which is nearly an entire fiction with firmware style debugging, that
is the problem. One binary will say "hey I'm iOS" and another binary that
needs to be loaded also is like "I'm something else" and lldb will reject the
module load even though the UUIDs match.
https://github.com/llvm/llvm-project/pull/98845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
@@ -757,11 +758,32 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+StreamString prog_str;
+// 'mach_kernel' is a fake name we make up to find kernels
+// that were located by the local filesystem scan.
+if (GetName() != "mach_kernel")
+ prog_str << GetName() << " ";
+if (GetUUID().IsValid())
+ prog_str << GetUUID().GetAsString() << " ";
+if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+ prog_str << "at 0x";
+ prog_str.PutHex64(GetLoadAddress());
+}
+std::unique_ptr progress_wp;
+if (IsKernel())
+ progress_wp = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+else
+ progress_wp = std::make_unique("Loading kext",
+ prog_str.GetString().str());
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
module_spec.GetUUID() = m_uuid;
- module_spec.GetArchitecture() = target.GetArchitecture();
+ if (!m_uuid.IsValid())
+module_spec.GetArchitecture() = target.GetArchitecture();
jasonmolenda wrote:
I know I shouldn't be making changes like this at the same time as adding
progress status logging, but it irked me when I saw it.
https://github.com/llvm/llvm-project/pull/98845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
@@ -757,11 +758,32 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+StreamString prog_str;
+// 'mach_kernel' is a fake name we make up to find kernels
+// that were located by the local filesystem scan.
+if (GetName() != "mach_kernel")
+ prog_str << GetName() << " ";
+if (GetUUID().IsValid())
+ prog_str << GetUUID().GetAsString() << " ";
+if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+ prog_str << "at 0x";
+ prog_str.PutHex64(GetLoadAddress());
+}
+std::unique_ptr progress_wp;
+if (IsKernel())
+ progress_wp = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+else
+ progress_wp = std::make_unique("Loading kext",
+ prog_str.GetString().str());
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
module_spec.GetUUID() = m_uuid;
- module_spec.GetArchitecture() = target.GetArchitecture();
+ if (!m_uuid.IsValid())
+module_spec.GetArchitecture() = target.GetArchitecture();
jasonmolenda wrote:
If I have a UUID, it's authoritative, whereas the ArchSpec might be
heuristically determined. I don't like setting both in a ModuleSpec if the
UUID is valid, it it noramlly fine but it's a little footgun waiting for some
unusual combination where the heuristically determined ArchSpec is not quite
the same ("compatible") with the arch of the UUID specified.
https://github.com/llvm/llvm-project/pull/98845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
jasonmolenda wrote: Ah no, I misunderstood. The Increment method of Progress is intended for use where you have one progress status display that takes multiple steps to complete. It's a separate mechanism from "finding dSYM for file x" etc. I can't use an Increment style progress reporting in this PR. https://github.com/llvm/llvm-project/pull/98845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/98845
>From cead9ae6de627ee64fb58a829fa3485f526a0afc Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Sun, 14 Jul 2024 16:59:51 -0700
Subject: [PATCH 1/2] [lldb] progressive progress reporting for darwin
kernel/firmware
When doing firmware/kernel debugging, it is frequent that
binaries and debug info need to be retrieved / downloaded,
and the lack of progress reports made for a poor experience,
with lldb seemingly hung while downloading things over the
network. This PR adds progress reports to the critical sites
for these use cases.
---
lldb/source/Core/DynamicLoader.cpp| 27 +++--
.../DynamicLoaderDarwinKernel.cpp | 39 ---
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp
b/lldb/source/Core/DynamicLoader.cpp
index 7871be6fc451d..a59136381c23b 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/ModuleList.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Symbol/ObjectFile.h"
#include "lldb/Target/MemoryRegionInfo.h"
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+Progress progress_memread("Reading load commands from memory",
+ prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
-if (memory_module_sp)
+if (memory_module_sp) {
uuid = memory_module_sp->GetUUID();
+ if (uuid.IsValid()) {
+prog_str << " ";
+prog_str << uuid.GetAsString();
+ }
+}
}
ModuleSpec module_spec;
module_spec.GetUUID() = uuid;
FileSpec name_filespec(name);
- if (FileSystem::Instance().Exists(name_filespec))
-module_spec.GetFileSpec() = name_filespec;
if (uuid.IsValid()) {
+Progress progress("Locating external symbol file",
+ prog_str.GetString().str());
+
// Has lldb already seen a module with this UUID?
+// Or have external lookup enabled in DebugSymbols on macOS.
if (!module_sp)
error = ModuleList::GetSharedModule(module_spec, module_sp, nullptr,
nullptr, nullptr);
diff --git
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 8d83937aab668..93eb1e7b9dea9 100644
---
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
@@ -757,6 +758,23 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+std::unique_ptr progress_up;
+if (IsKernel()) {
+ StreamString prog_str;
+ // 'mach_kernel' is a fake name we make up to find kernels
+ // that were located by the local filesystem scan.
+ if (GetName() != "mach_kernel")
+prog_str << GetName() << " ";
+ if (GetUUID().IsValid())
+prog_str << GetUUID().GetAsString() << " ";
+ if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(GetLoadAddress());
+ }
+ progress_up = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+}
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
@@ -1058,12 +1076,9 @@ void
DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
}
}
}
-
-if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS) {
- if (!m_kernel.LoadImageUsingMemoryModule(m_process)) {
+if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS)
+ if (!m_kernel.LoadImageUsingMemoryModule(m_process))
m_kernel.LoadImageAtFileAddress(m_process);
- }
-}
// The operating system plugin gets loaded and initialized in
// LoadImageUsingMemoryModule when we discover the
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
jasonmolenda wrote: The one thing I was uncertain of is when I load kexts -- I have a known number of kexts that I will be attempting to load -- and I wasn't sure if I should log them individually as they happen (what I did), or if I should initialize a progress category with the number of kexts that lldb will be attempting to load, so the progress of the job overall can be visualized. https://github.com/llvm/llvm-project/pull/98845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
https://github.com/jasonmolenda created
https://github.com/llvm/llvm-project/pull/98845
When doing firmware/kernel debugging, it is frequent that binaries and debug
info need to be retrieved / downloaded, and the lack of progress reports made
for a poor experience, with lldb seemingly hung while downloading things over
the network. This PR adds progress reports to the critical sites for these use
cases.
>From cead9ae6de627ee64fb58a829fa3485f526a0afc Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Sun, 14 Jul 2024 16:59:51 -0700
Subject: [PATCH] [lldb] progressive progress reporting for darwin
kernel/firmware
When doing firmware/kernel debugging, it is frequent that
binaries and debug info need to be retrieved / downloaded,
and the lack of progress reports made for a poor experience,
with lldb seemingly hung while downloading things over the
network. This PR adds progress reports to the critical sites
for these use cases.
---
lldb/source/Core/DynamicLoader.cpp| 27 +++--
.../DynamicLoaderDarwinKernel.cpp | 39 ---
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp
b/lldb/source/Core/DynamicLoader.cpp
index 7871be6fc451d..a59136381c23b 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/ModuleList.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Symbol/ObjectFile.h"
#include "lldb/Target/MemoryRegionInfo.h"
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+Progress progress_memread("Reading load commands from memory",
+ prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
-if (memory_module_sp)
+if (memory_module_sp) {
uuid = memory_module_sp->GetUUID();
+ if (uuid.IsValid()) {
+prog_str << " ";
+prog_str << uuid.GetAsString();
+ }
+}
}
ModuleSpec module_spec;
module_spec.GetUUID() = uuid;
FileSpec name_filespec(name);
- if (FileSystem::Instance().Exists(name_filespec))
-module_spec.GetFileSpec() = name_filespec;
if (uuid.IsValid()) {
+Progress progress("Locating external symbol file",
+ prog_str.GetString().str());
+
// Has lldb already seen a module with this UUID?
+// Or have external lookup enabled in DebugSymbols on macOS.
if (!module_sp)
error = ModuleList::GetSharedModule(module_spec, module_sp, nullptr,
nullptr, nullptr);
diff --git
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 8d83937aab668..93eb1e7b9dea9 100644
---
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
@@ -757,6 +758,23 @@ bool
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList _images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+std::unique_ptr progress_up;
+if (IsKernel()) {
+ StreamString prog_str;
+ // 'mach_kernel' is a fake name we make up to find kernels
+ // that were located by the local filesystem scan.
+ if (GetName() != "mach_kernel")
+prog_str << GetName() << " ";
+ if (GetUUID().IsValid())
+prog_str << GetUUID().GetAsString() << " ";
+ if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+prog_str << "at ";
+prog_str.PutHex64(GetLoadAddress());
+ }
+ progress_up = std::make_unique("Loading kernel",
+ prog_str.GetString().str());
+}
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
@@ -1058,12 +1076,9 @@ void
DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
}
}
}
-
-if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS) {
- if
[Lldb-commits] [lldb] 3478573 - [lldb] [NFC] a couple comment typeo and markup fixes.
Author: Jason Molenda
Date: 2024-07-14T10:59:46-07:00
New Revision: 3478573773d02e574524fd45c14631de5b7d10a9
URL:
https://github.com/llvm/llvm-project/commit/3478573773d02e574524fd45c14631de5b7d10a9
DIFF:
https://github.com/llvm/llvm-project/commit/3478573773d02e574524fd45c14631de5b7d10a9.diff
LOG: [lldb] [NFC] a couple comment typeo and markup fixes.
Added:
Modified:
lldb/include/lldb/Core/Progress.h
Removed:
diff --git a/lldb/include/lldb/Core/Progress.h
b/lldb/include/lldb/Core/Progress.h
index cd87be79c4f0e..421e435a9e685 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -42,8 +42,8 @@ namespace lldb_private {
///uint64_t total,
///void *baton);
///
-/// This callback will always initially be called with "completed" set to zero
-/// and "total" set to the total amount specified in the contructor. This is
+/// This callback will always initially be called with \a completed set to zero
+/// and \a total set to the total amount specified in the constructor. This is
/// considered the progress start event. As Progress::Increment() is called,
/// the callback will be called as long as the Progress::m_completed has not
/// yet exceeded the Progress::m_total. When the callback is called with
@@ -52,7 +52,7 @@ namespace lldb_private {
/// Progress::m_total, then this is considered a progress update event.
///
/// This callback will be called in the destructor if Progress::m_completed is
-/// not equal to Progress::m_total with the "completed" set to
+/// not equal to Progress::m_total with the \a completed set to
/// Progress::m_total. This ensures we always send a progress completed update
/// even if the user does not.
@@ -62,7 +62,7 @@ class Progress {
///
/// The constructor will create a unique progress reporting object and
/// immediately send out a progress update by calling the installed callback
- /// with completed set to zero out of the specified total.
+ /// with \a completed set to zero out of the specified total.
///
/// @param [in] title The title of this progress activity.
///
@@ -86,11 +86,11 @@ class Progress {
/// Destroy the progress object.
///
/// If the progress has not yet sent a completion update, the destructor
- /// will send out a notification where the completed == m_total. This ensures
- /// that we always send out a progress complete notification.
+ /// will send out a notification where the \a completed == m_total. This
+ /// ensures that we always send out a progress complete notification.
~Progress();
- /// Increment the progress and send a notification to the intalled callback.
+ /// Increment the progress and send a notification to the installed callback.
///
/// If incrementing ends up exceeding m_total, m_completed will be updated
/// to match m_total and no subsequent progress notifications will be sent.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Allow fetching of RA register when above fault handler (PR #98566)
https://github.com/jasonmolenda closed https://github.com/llvm/llvm-project/pull/98566 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Allow fetching of RA register when above fault handler (PR #98566)
jasonmolenda wrote: > Testing this is in theory possible, tricky bit is guaranteeing a frameless > function. There is the naked attribute but it's not portable > https://godbolt.org/z/s9117Gr7a. Or you could write the function in an > assembly file, or define and call it inside an inline assembly block, inside > a normal C function. That function would branch to self waiting for SIGALRM > for example. > > Maybe that has its own problems, I haven't tried it. Maybe it wouldn't > generate enough debug info for us to know that the assembly function was > there? There's (I'd argue) three parts to the unwind system. First is converting the different unwind info formats (eh_frame, debug_frame, compact unwind, arm idx, assembly instruciton scanning) into the intermediate representation of UnwindPlans. Second is the unwind engine itself, which encodes rules about which type of unwind plan to use for a given stack frame, which registers can be passed up the stack, and rules about behavior on the 0th frame or above a fault handler/sigtramp. And third are correctly fetching the register value for a row in an UnwindPlan (often, dereferencing memory offset from the Canonical Frame Address which is set in terms of another register most often) -- these often end up being dwarf expressions. That middle bit, the unwind engine logic, is hard to test today without making hand-written assembly programs that set up specific unwind scenarios with metadata (.cfi directives) about what they've done. Source level tests are at the mercy of compiler codegen and not stable, or requires capturing a corefile and object binary when the necessary conditions are achieved. But here's the idea I had the other day. With a scripted process, an ObjectFileJSON to create a fake binary with function start addresses, and a way to specify UnwindPlans for those functions, where all of the register rules would be "and the value of fp is " instead of "read stack memory to get the value of fp", I bet there's a way we could write unwind engine tests entirely in these terms. And honest, the unwind engine method has a lot of very tricky corner cases and because it's not directly tested, it's easy to make mistakes - I am genuinely not thrilled about the state of it. And without strong test infrastructure, it's going to be very intimidating to try to rewrite if anyone wanted to do that some day. This is only "shower thoughts" level detail, but it's the first time I can see a testing strategy that I actually think could work well. https://github.com/llvm/llvm-project/pull/98566 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add support for displaying `__float128` variables (PR #98369)
https://github.com/jasonmolenda edited https://github.com/llvm/llvm-project/pull/98369 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add support for displaying `__float128` variables (PR #98369)
@@ -46,6 +46,7 @@ static constexpr FormatInfo g_format_infos[] = {
{eFormatHex, 'x', "hex"},
{eFormatHexUppercase, 'X', "uppercase hex"},
{eFormatFloat, 'f', "float"},
+{eFormatFloat128, '\0', "float128"},
jasonmolenda wrote:
I'm not familiar with this part of lldb, but do we need an entry with a
character code that can't be entered? I don't think there are commands which
take the "long name" of these entries, do they? Vaguely relatedly, I wondered
about `OptionGroupFormat::ParserGDBFormatLetter` which recognizes the gdb
formatters (v.
https://sourceware.org/gdb/current/onlinedocs/gdb.html/Output-Formats.html )
but I think just "f" for "float of undetermined size" is correct as-is? This
is seen in commands like `x/2gx $fp` etc, which command aliases rewrite to a
longer `memory read`.
https://github.com/llvm/llvm-project/pull/98369
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Allow fetching of RA register when above fault handler (PR #98566)
https://github.com/jasonmolenda created
https://github.com/llvm/llvm-project/pull/98566
In RegisterContextUnwind::SavedLocationForRegister we have special logic for
retrieving the Return Address register when it has the caller's return address
in it. An example would be the lr register on AArch64.
This register is never retrieved from a newer stack frame because it is
necessarly overwritten by a normal ABI function call. We allow frame 0 to
provide its lr value to get the caller's return address, if it has not been
overwritten/saved to stack yet.
When a function is interrupted asynchronously by a POSIX signal (sigtramp), or
a fault handler more generally, the sigtramp/fault handler has the entire
register context available. In this situation, if the fault handler is frame 0,
the function that was async interrupted is frame 1 and frame 2's return address
may still be stored in lr. We need to get the lr value for frame 1 from the
fault handler in frame 0, to get the return address for frame 2.
Without this fix, a frameless function that faults in a firmware environment
(that's where we've seen this issue most commonly) hasn't spilled lr to stack,
so we need to retrieve it from the fault handler's full-register-context to
find the caller of the frameless function that faulted.
It's an unsurprising fix, all of the work was finding exactly where in
RegisterContextUnwind we were only allowing RA register use for frame 0, when
it should have been frame 0 or above a fault handler function.
rdar://127518945
>From 48a06a9d85d2ca07e50bfda9002350d3ab157ed9 Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Thu, 11 Jul 2024 17:03:13 -0700
Subject: [PATCH] [lldb] Allow fetching of RA register when above fault handler
In RegisterContextUnwind::SavedLocationForRegister we have special
logic for retrieving the Return Address register when it has
the caller's return address in it. An example would be the lr
register on AArch64.
This register is never retrieved from a newer stack frame because
it is necessarly overwritten by a normal ABI function call. We
allow frame 0 to provide its lr value to get the caller's return
address, if it has not been overwritten/saved to stack yet.
When a function is interrupted asynchronously by a POSIX signal
(sigtramp), or a fault handler more generally, the sigtramp/fault
handler has the entire register context available. In this situation,
if the fault handler is frame 0, the function that was async
interrupted is frame 1 and frame 2's return address may still be
stored in lr. We need to get the lr value for frame 1 from the
fault handler in frame 0, to get the return address for frame 2.
Without this fix, a frameless function that faults in a firmware
environment (that's where we've seen this issue most commonly)
hasn't spilled lr to stack, so we need to retrieve it from the
fault handler's full-register-context to find the caller of the
frameless function that faulted.
It's an unsurprising fix, all of the work was finding exactly where
in RegisterContextUnwind we were only allowing RA register use for
frame 0, when it should have been frame 0 or above a fault handler
function.
rdar://127518945
---
lldb/source/Target/RegisterContextUnwind.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lldb/source/Target/RegisterContextUnwind.cpp
b/lldb/source/Target/RegisterContextUnwind.cpp
index 95e8abd763d53..bc8081f4e3b31 100644
--- a/lldb/source/Target/RegisterContextUnwind.cpp
+++ b/lldb/source/Target/RegisterContextUnwind.cpp
@@ -1401,7 +1401,7 @@ RegisterContextUnwind::SavedLocationForRegister(
// it's still live in the actual register. Handle this specially.
if (!have_unwindplan_regloc && return_address_reg.IsValid() &&
- IsFrameZero()) {
+ BehavesLikeZerothFrame()) {
if (return_address_reg.GetAsKind(eRegisterKindLLDB) !=
LLDB_INVALID_REGNUM) {
lldb_private::UnwindLLDB::RegisterLocation new_regloc;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] 1988c27 - [lldb] [NFC] Update TestEarlyProcessLaunch to work on macOS 15
Author: Jason Molenda
Date: 2024-07-11T16:51:28-07:00
New Revision: 1988c27e5f4dbcf42c9a80f44bdee7ccd208a0ac
URL:
https://github.com/llvm/llvm-project/commit/1988c27e5f4dbcf42c9a80f44bdee7ccd208a0ac
DIFF:
https://github.com/llvm/llvm-project/commit/1988c27e5f4dbcf42c9a80f44bdee7ccd208a0ac.diff
LOG: [lldb] [NFC] Update TestEarlyProcessLaunch to work on macOS 15
This test sets a breakpoint on malloc, as a way to stop early in
dyld's setting up code, before the system libraries are initialized
so we can confirm that we don't fetch the Objective-C class table
before it's initialized.
In macOS 15 (macOS Sonoma), dyld doesn't call malloc any longer,
so this heuristic/trick isn't working. It does call other things
called *alloc though, so I'm changing this to use a regex breakpoint
on that, to keep the test working.
Added:
Modified:
lldb/test/API/macosx/early-process-launch/TestEarlyProcessLaunch.py
Removed:
diff --git
a/lldb/test/API/macosx/early-process-launch/TestEarlyProcessLaunch.py
b/lldb/test/API/macosx/early-process-launch/TestEarlyProcessLaunch.py
index c15abbabc2374..ff704104f7892 100644
--- a/lldb/test/API/macosx/early-process-launch/TestEarlyProcessLaunch.py
+++ b/lldb/test/API/macosx/early-process-launch/TestEarlyProcessLaunch.py
@@ -1,6 +1,5 @@
"""Test that we don't read objc class tables early in process startup."""
-
import time
import lldb
from lldbsuite.test.decorators import *
@@ -30,7 +29,14 @@ def test_early_process_launch(self):
###
### Use the types logging to detect the
diff erence.
-target, process, _, bkpt = lldbutil.run_to_name_breakpoint(self,
"malloc")
+exe = self.getBuildArtifact("a.out")
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target.IsValid())
+bkpt = target.BreakpointCreateByRegex("alloc", None)
+self.assertTrue(bkpt.IsValid())
+(target, process, thread, bkpt) = lldbutil.run_to_breakpoint_do_run(
+self, target, bkpt
+)
target.DisableAllBreakpoints()
target.BreakpointCreateByName("main")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Don't crash on malformed filesets (PR #98388)
https://github.com/jasonmolenda approved this pull request. LGTM. We'll skip any fileset entries that we can't read the filename of. https://github.com/llvm/llvm-project/pull/98388 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
jasonmolenda wrote: Thanks again for all the help @AlexK0 I pushed an update that should compile correctly with my update. It's great to hear that the testsuite looks clean -- that's the part that worried me the most about these changes. I finished an aarch64 Ubuntu testsuite run and it is clean. I think this patch is about ready for review by @jimingham when he has time. https://github.com/llvm/llvm-project/pull/96260 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/96260 >From 9b541e6a035635e26c6a24eca022de8552fa4c17 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Thu, 20 Jun 2024 17:53:17 -0700 Subject: [PATCH 1/6] [lldb] Change lldb's breakpoint handling behavior lldb today has two rules: When a thread stops at a BreakpointSite, we set the thread's StopReason to be "breakpoint hit" (regardless if we've actually hit the breakpoint, or if we've merely stopped *at* the breakpoint instruction/point and haven't tripped it yet). And second, when resuming a process, any thread sitting at a BreakpointSite is silently stepped over the BreakpointSite -- because we've already flagged the breakpoint hit when we stopped there originally. In this patch, I change lldb to only set a thread's stop reason to breakpoint-hit when we've actually executed the instruction/triggered the breakpoint. When we resume, we only silently step past a BreakpointSite that we've registered as hit. We preserve this state across inferior function calls that the user may do while stopped, etc. Also, when a user adds a new breakpoint at $pc while stopped, or changes $pc to be the address of a BreakpointSite, we will silently step past that breakpoint when the process resumes. This is purely a UX call, I don't think there's any person who wants to set a breakpoint at $pc and then hit it immediately on resuming. One non-intuitive UX from this change, but I'm convinced it is necessary: If you're stopped at a BreakpointSite that has not yet executed, you `stepi`, you will hit the breakpoint and the pc will not yet advance. This thread has not completed its stepi, and the thread plan is still on the stack. If you then `continue` the thread, lldb will now stop and say, "instruction step completed", one instruction past the BreakpointSite. You can continue a second time to resume execution. I discussed this with Jim, and trying to paper over this behavior will lead to more complicated scenarios behaving non-intuitively. And mostly it's the testsuite that was trying to instruction step past a breakpoint and getting thrown off -- and I changed those tests to expect the new behavior. The bugs driving this change are all from lldb dropping the real stop reason for a thread and setting it to breakpoint-hit when that was not the case. Jim hit one where we have an aarch64 watchpoint that triggers one instruction before a BreakpointSite. On this arch we are notified of the watchpoint hit after the instruction has been unrolled -- we disable the watchpoint, instruction step, re-enable the watchpoint and collect the new value. But now we're on a BreakpointSite so the watchpoint-hit stop reason is lost. Another was reported by ZequanWu in https://discourse.llvm.org/t/lldb-unable-to-break-at-start/78282 we attach to/launch a process with the pc at a BreakpointSite and misbehave. Caroline Tice mentioned it is also a problem they've had with putting a breakpoint on _dl_debug_state. The change to each Process plugin that does execution control is that 1. If we've stopped at a BreakpointSite (whether we hit it or not), we call Thread::SetThreadStoppedAtBreakpointSite(pc) to record the state at the point when the thread stopped. (so we can detect newly-added breakpoints, or when the pc is changed to an instruction that is a BreakpointSite) 2. When we have actually hit a breakpoint, and it is enabled for this thread, we call Thread::SetThreadHitBreakpointAtAddr(pc) so we know that it should be silently stepped past when we resume execution. When resuming, we silently step over a breakpoint if we've hit it, or if it is newly added (or the pc was changed to an existing BreakpointSite). The biggest set of changes is to StopInfoMachException where we translate a Mach Exception into a stop reason. The Mach exception codes differ in a few places depending on the target (unambiguously), and I didn't want to duplicate the new code for each target so I've tested what mach exceptions we get for each action on each target, and reorganized StopInfoMachException::CreateStopReasonWithMachException to document these possible values, and handle them without specializing based on the target arch. rdar://123942164 --- lldb/include/lldb/Target/Thread.h | 29 ++ .../Process/Utility/StopInfoMachException.cpp | 296 +++--- .../Process/Windows/Common/ProcessWindows.cpp | 16 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 118 +++ .../Process/scripted/ScriptedThread.cpp | 9 + lldb/source/Target/Thread.cpp | 17 +- .../TestConsecutiveBreakpoints.py | 26 +- .../TestStepOverBreakpoint.py | 6 +- 8 files changed, 235 insertions(+), 282 deletions(-) diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h index c17bddf4d98b8..1e1aead896018 100644 --- a/lldb/include/lldb/Target/Thread.h +++
[Lldb-commits] [lldb] [lldb][NFC] Add maybe_unused to err used in asserts (PR #98055)
https://github.com/jasonmolenda approved this pull request. LGTM. https://github.com/llvm/llvm-project/pull/98055 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][DataFormatter][NFC] Move std::map iterator formatter into LibCxxMap.cpp (PR #97687)
https://github.com/jasonmolenda approved this pull request. LGTM. https://github.com/llvm/llvm-project/pull/97687 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Improve error message for unrecognized executables (PR #97490)
https://github.com/jasonmolenda approved this pull request. This look good. I like distinguishing between "is not a known binary type" and "does not contain an architecture supported on this platform (but is a binary file that we recognize)". https://github.com/llvm/llvm-project/pull/97490 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64] Add register field enum information (PR #96887)
https://github.com/jasonmolenda approved this pull request. LGTM. https://github.com/llvm/llvm-project/pull/96887 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [ObjectFileMachO] BSS segments are loadable segments (PR #96983)
https://github.com/jasonmolenda closed https://github.com/llvm/llvm-project/pull/96983 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/96260 >From 9b541e6a035635e26c6a24eca022de8552fa4c17 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Thu, 20 Jun 2024 17:53:17 -0700 Subject: [PATCH 1/5] [lldb] Change lldb's breakpoint handling behavior lldb today has two rules: When a thread stops at a BreakpointSite, we set the thread's StopReason to be "breakpoint hit" (regardless if we've actually hit the breakpoint, or if we've merely stopped *at* the breakpoint instruction/point and haven't tripped it yet). And second, when resuming a process, any thread sitting at a BreakpointSite is silently stepped over the BreakpointSite -- because we've already flagged the breakpoint hit when we stopped there originally. In this patch, I change lldb to only set a thread's stop reason to breakpoint-hit when we've actually executed the instruction/triggered the breakpoint. When we resume, we only silently step past a BreakpointSite that we've registered as hit. We preserve this state across inferior function calls that the user may do while stopped, etc. Also, when a user adds a new breakpoint at $pc while stopped, or changes $pc to be the address of a BreakpointSite, we will silently step past that breakpoint when the process resumes. This is purely a UX call, I don't think there's any person who wants to set a breakpoint at $pc and then hit it immediately on resuming. One non-intuitive UX from this change, but I'm convinced it is necessary: If you're stopped at a BreakpointSite that has not yet executed, you `stepi`, you will hit the breakpoint and the pc will not yet advance. This thread has not completed its stepi, and the thread plan is still on the stack. If you then `continue` the thread, lldb will now stop and say, "instruction step completed", one instruction past the BreakpointSite. You can continue a second time to resume execution. I discussed this with Jim, and trying to paper over this behavior will lead to more complicated scenarios behaving non-intuitively. And mostly it's the testsuite that was trying to instruction step past a breakpoint and getting thrown off -- and I changed those tests to expect the new behavior. The bugs driving this change are all from lldb dropping the real stop reason for a thread and setting it to breakpoint-hit when that was not the case. Jim hit one where we have an aarch64 watchpoint that triggers one instruction before a BreakpointSite. On this arch we are notified of the watchpoint hit after the instruction has been unrolled -- we disable the watchpoint, instruction step, re-enable the watchpoint and collect the new value. But now we're on a BreakpointSite so the watchpoint-hit stop reason is lost. Another was reported by ZequanWu in https://discourse.llvm.org/t/lldb-unable-to-break-at-start/78282 we attach to/launch a process with the pc at a BreakpointSite and misbehave. Caroline Tice mentioned it is also a problem they've had with putting a breakpoint on _dl_debug_state. The change to each Process plugin that does execution control is that 1. If we've stopped at a BreakpointSite (whether we hit it or not), we call Thread::SetThreadStoppedAtBreakpointSite(pc) to record the state at the point when the thread stopped. (so we can detect newly-added breakpoints, or when the pc is changed to an instruction that is a BreakpointSite) 2. When we have actually hit a breakpoint, and it is enabled for this thread, we call Thread::SetThreadHitBreakpointAtAddr(pc) so we know that it should be silently stepped past when we resume execution. When resuming, we silently step over a breakpoint if we've hit it, or if it is newly added (or the pc was changed to an existing BreakpointSite). The biggest set of changes is to StopInfoMachException where we translate a Mach Exception into a stop reason. The Mach exception codes differ in a few places depending on the target (unambiguously), and I didn't want to duplicate the new code for each target so I've tested what mach exceptions we get for each action on each target, and reorganized StopInfoMachException::CreateStopReasonWithMachException to document these possible values, and handle them without specializing based on the target arch. rdar://123942164 --- lldb/include/lldb/Target/Thread.h | 29 ++ .../Process/Utility/StopInfoMachException.cpp | 296 +++--- .../Process/Windows/Common/ProcessWindows.cpp | 16 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 118 +++ .../Process/scripted/ScriptedThread.cpp | 9 + lldb/source/Target/Thread.cpp | 17 +- .../TestConsecutiveBreakpoints.py | 26 +- .../TestStepOverBreakpoint.py | 6 +- 8 files changed, 235 insertions(+), 282 deletions(-) diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h index c17bddf4d98b8..1e1aead896018 100644 --- a/lldb/include/lldb/Target/Thread.h +++
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
jasonmolenda wrote: Hi @AlexK0 I restructured this PR to track everything with a single piece of state in the Thread object, and redid all three Process plugins. I'm feeling a lot better about my changes to ProcessWindows now. I haven't tested the aarch64 ubuntu yet, I will do that tomorrow and if that looks good, I'm going to ask for more careful reviews of the PR. No rush at all, but if you have a chance to try this version on Windows I would really appreciate it. Thanks! https://github.com/llvm/llvm-project/pull/96260 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/96260 >From 9b541e6a035635e26c6a24eca022de8552fa4c17 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Thu, 20 Jun 2024 17:53:17 -0700 Subject: [PATCH 1/4] [lldb] Change lldb's breakpoint handling behavior lldb today has two rules: When a thread stops at a BreakpointSite, we set the thread's StopReason to be "breakpoint hit" (regardless if we've actually hit the breakpoint, or if we've merely stopped *at* the breakpoint instruction/point and haven't tripped it yet). And second, when resuming a process, any thread sitting at a BreakpointSite is silently stepped over the BreakpointSite -- because we've already flagged the breakpoint hit when we stopped there originally. In this patch, I change lldb to only set a thread's stop reason to breakpoint-hit when we've actually executed the instruction/triggered the breakpoint. When we resume, we only silently step past a BreakpointSite that we've registered as hit. We preserve this state across inferior function calls that the user may do while stopped, etc. Also, when a user adds a new breakpoint at $pc while stopped, or changes $pc to be the address of a BreakpointSite, we will silently step past that breakpoint when the process resumes. This is purely a UX call, I don't think there's any person who wants to set a breakpoint at $pc and then hit it immediately on resuming. One non-intuitive UX from this change, but I'm convinced it is necessary: If you're stopped at a BreakpointSite that has not yet executed, you `stepi`, you will hit the breakpoint and the pc will not yet advance. This thread has not completed its stepi, and the thread plan is still on the stack. If you then `continue` the thread, lldb will now stop and say, "instruction step completed", one instruction past the BreakpointSite. You can continue a second time to resume execution. I discussed this with Jim, and trying to paper over this behavior will lead to more complicated scenarios behaving non-intuitively. And mostly it's the testsuite that was trying to instruction step past a breakpoint and getting thrown off -- and I changed those tests to expect the new behavior. The bugs driving this change are all from lldb dropping the real stop reason for a thread and setting it to breakpoint-hit when that was not the case. Jim hit one where we have an aarch64 watchpoint that triggers one instruction before a BreakpointSite. On this arch we are notified of the watchpoint hit after the instruction has been unrolled -- we disable the watchpoint, instruction step, re-enable the watchpoint and collect the new value. But now we're on a BreakpointSite so the watchpoint-hit stop reason is lost. Another was reported by ZequanWu in https://discourse.llvm.org/t/lldb-unable-to-break-at-start/78282 we attach to/launch a process with the pc at a BreakpointSite and misbehave. Caroline Tice mentioned it is also a problem they've had with putting a breakpoint on _dl_debug_state. The change to each Process plugin that does execution control is that 1. If we've stopped at a BreakpointSite (whether we hit it or not), we call Thread::SetThreadStoppedAtBreakpointSite(pc) to record the state at the point when the thread stopped. (so we can detect newly-added breakpoints, or when the pc is changed to an instruction that is a BreakpointSite) 2. When we have actually hit a breakpoint, and it is enabled for this thread, we call Thread::SetThreadHitBreakpointAtAddr(pc) so we know that it should be silently stepped past when we resume execution. When resuming, we silently step over a breakpoint if we've hit it, or if it is newly added (or the pc was changed to an existing BreakpointSite). The biggest set of changes is to StopInfoMachException where we translate a Mach Exception into a stop reason. The Mach exception codes differ in a few places depending on the target (unambiguously), and I didn't want to duplicate the new code for each target so I've tested what mach exceptions we get for each action on each target, and reorganized StopInfoMachException::CreateStopReasonWithMachException to document these possible values, and handle them without specializing based on the target arch. rdar://123942164 --- lldb/include/lldb/Target/Thread.h | 29 ++ .../Process/Utility/StopInfoMachException.cpp | 296 +++--- .../Process/Windows/Common/ProcessWindows.cpp | 16 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 118 +++ .../Process/scripted/ScriptedThread.cpp | 9 + lldb/source/Target/Thread.cpp | 17 +- .../TestConsecutiveBreakpoints.py | 26 +- .../TestStepOverBreakpoint.py | 6 +- 8 files changed, 235 insertions(+), 282 deletions(-) diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h index c17bddf4d98b8..1e1aead896018 100644 --- a/lldb/include/lldb/Target/Thread.h +++
[Lldb-commits] [lldb] [lldb] [ObjectFileMachO] BSS segments are loadable segments (PR #96983)
https://github.com/jasonmolenda updated
https://github.com/llvm/llvm-project/pull/96983
>From 6bd566504355e8d50b9c922df9ebce18e07a726f Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Thu, 27 Jun 2024 15:34:48 -0700
Subject: [PATCH 1/2] [lldb] [ObjectFileMachO] BSS segments are loadable
segments
ObjectFileMachO::SetLoadAddress sets the address of each segment
in a binary in a Target, but it ignores segments that are not loaded
in the virtual address space. It was marking segments that were
purely BSS -- having no content in the file, but in zero-initialized
memory when running in the virtual address space -- as not-loadable,
unless they were named "DATA". This works pretty well for typical
userland binaries, but in less Darwin environments, there may be
BSS segments with other names, that ARE loadable.
I looked at the origin of SectionIsLoadable's check for this, and
it was a cleanup by Greg in 2018 where we had three different
implementations of the idea in ObjectFileMachO and one of them
skipped zero-file-size segments (BSS), which made it into the
centralized SectionIsLoadable method.
Also add some logging to the DynamicLoader log channel when loading
a binary - it's the first place I look when debugging segment address
setting bugs, and it wasn't emitting anything.
rdar://129870649
---
.../ObjectFile/Mach-O/ObjectFileMachO.cpp | 31 ---
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 2979bf69bf762..164c4409747e0 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6159,10 +6159,6 @@ Section *ObjectFileMachO::GetMachHeaderSection() {
bool ObjectFileMachO::SectionIsLoadable(const Section *section) {
if (!section)
return false;
- const bool is_dsym = (m_header.filetype == MH_DSYM);
- if (section->GetFileSize() == 0 && !is_dsym &&
- section->GetName() != GetSegmentNameDATA())
-return false;
if (section->IsThreadSpecific())
return false;
if (GetModule().get() != section->GetModule().get())
@@ -6202,6 +6198,7 @@ lldb::addr_t
ObjectFileMachO::CalculateSectionLoadAddressForMemoryImage(
bool ObjectFileMachO::SetLoadAddress(Target , lldb::addr_t value,
bool value_is_offset) {
+ Log *log(GetLog(LLDBLog::DynamicLoader));
ModuleSP module_sp = GetModule();
if (!module_sp)
return false;
@@ -6217,17 +6214,37 @@ bool ObjectFileMachO::SetLoadAddress(Target ,
lldb::addr_t value,
// malformed.
const bool warn_multiple = true;
+ if (log) {
+std::string binary_description;
+if (GetFileSpec()) {
+ binary_description += "path='";
+ binary_description += GetFileSpec().GetPath();
+ binary_description += "' ";
+}
+if (GetUUID()) {
+ binary_description += "uuid=";
+ binary_description += GetUUID().GetAsString();
+}
+LLDB_LOGF(log, "ObjectFileMachO::SetLoadAddress %s",
+ binary_description.c_str());
+ }
if (value_is_offset) {
// "value" is an offset to apply to each top level segment
for (size_t sect_idx = 0; sect_idx < num_sections; ++sect_idx) {
// Iterate through the object file sections to find all of the
// sections that size on disk (to avoid __PAGEZERO) and load them
SectionSP section_sp(section_list->GetSectionAtIndex(sect_idx));
- if (SectionIsLoadable(section_sp.get()))
+ if (SectionIsLoadable(section_sp.get())) {
+LLDB_LOGF(log,
+ "ObjectFileMachO::SetLoadAddress segment '%s' load addr is "
+ "0x%" PRIx64,
+ section_sp->GetName().AsCString(),
+ section_sp->GetFileAddress() + value);
if (target.GetSectionLoadList().SetSectionLoadAddress(
section_sp, section_sp->GetFileAddress() + value,
warn_multiple))
++num_loaded_sections;
+ }
}
} else {
// "value" is the new base address of the mach_header, adjust each
@@ -6242,6 +6259,10 @@ bool ObjectFileMachO::SetLoadAddress(Target ,
lldb::addr_t value,
CalculateSectionLoadAddressForMemoryImage(
value, mach_header_section, section_sp.get());
if (section_load_addr != LLDB_INVALID_ADDRESS) {
+ LLDB_LOGF(log,
+"ObjectFileMachO::SetLoadAddress segment '%s' load addr is
"
+"0x%" PRIx64,
+section_sp->GetName().AsCString(), section_load_addr);
if (target.GetSectionLoadList().SetSectionLoadAddress(
section_sp, section_load_addr, warn_multiple))
++num_loaded_sections;
>From 2a7b154cbdffb75d763d3f8587266c9a7c0a0eb0 Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Mon, 1 Jul 2024 17:29:41 -0700
Subject: [PATCH 2/2] Build up the log message
[Lldb-commits] [lldb] [lldb] [ObjectFileMachO] BSS segments are loadable segments (PR #96983)
jasonmolenda wrote: I was about to add a macosx/ API test to compile a program with only BSS data in the DATA segment, and I noticed I'd written `TestBSSOnlyDataSectionSliding.py` last year when I opted sections named "DATA" out of this "don't set a load address for BSS-only segments" behavior in ObjectFileMachO. This existing test is already covering the change I'm making in this PR. https://github.com/llvm/llvm-project/pull/96983 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [ObjectFileMachO] BSS segments are loadable segments (PR #96983)
https://github.com/jasonmolenda created
https://github.com/llvm/llvm-project/pull/96983
ObjectFileMachO::SetLoadAddress sets the address of each segment in a binary in
a Target, but it ignores segments that are not loaded in the virtual address
space. It was marking segments that were purely BSS -- having no content in
the file, but in zero-initialized memory when running in the virtual address
space -- as not-loadable, unless they were named "DATA". This works pretty
well for typical userland binaries, but in less Darwin environments, there may
be BSS segments with other names, that ARE loadable.
I looked at the origin of SectionIsLoadable's check for this, and it was a
cleanup by Greg in 2018 where we had three different implementations of the
idea in ObjectFileMachO and one of them skipped zero-file-size segments (BSS),
which made it into the centralized SectionIsLoadable method.
Also add some logging to the DynamicLoader log channel when loading a binary -
it's the first place I look when debugging segment address setting bugs, and it
wasn't emitting anything.
rdar://129870649
>From 6bd566504355e8d50b9c922df9ebce18e07a726f Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Thu, 27 Jun 2024 15:34:48 -0700
Subject: [PATCH] [lldb] [ObjectFileMachO] BSS segments are loadable segments
ObjectFileMachO::SetLoadAddress sets the address of each segment
in a binary in a Target, but it ignores segments that are not loaded
in the virtual address space. It was marking segments that were
purely BSS -- having no content in the file, but in zero-initialized
memory when running in the virtual address space -- as not-loadable,
unless they were named "DATA". This works pretty well for typical
userland binaries, but in less Darwin environments, there may be
BSS segments with other names, that ARE loadable.
I looked at the origin of SectionIsLoadable's check for this, and
it was a cleanup by Greg in 2018 where we had three different
implementations of the idea in ObjectFileMachO and one of them
skipped zero-file-size segments (BSS), which made it into the
centralized SectionIsLoadable method.
Also add some logging to the DynamicLoader log channel when loading
a binary - it's the first place I look when debugging segment address
setting bugs, and it wasn't emitting anything.
rdar://129870649
---
.../ObjectFile/Mach-O/ObjectFileMachO.cpp | 31 ---
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 2979bf69bf762..164c4409747e0 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6159,10 +6159,6 @@ Section *ObjectFileMachO::GetMachHeaderSection() {
bool ObjectFileMachO::SectionIsLoadable(const Section *section) {
if (!section)
return false;
- const bool is_dsym = (m_header.filetype == MH_DSYM);
- if (section->GetFileSize() == 0 && !is_dsym &&
- section->GetName() != GetSegmentNameDATA())
-return false;
if (section->IsThreadSpecific())
return false;
if (GetModule().get() != section->GetModule().get())
@@ -6202,6 +6198,7 @@ lldb::addr_t
ObjectFileMachO::CalculateSectionLoadAddressForMemoryImage(
bool ObjectFileMachO::SetLoadAddress(Target , lldb::addr_t value,
bool value_is_offset) {
+ Log *log(GetLog(LLDBLog::DynamicLoader));
ModuleSP module_sp = GetModule();
if (!module_sp)
return false;
@@ -6217,17 +6214,37 @@ bool ObjectFileMachO::SetLoadAddress(Target ,
lldb::addr_t value,
// malformed.
const bool warn_multiple = true;
+ if (log) {
+std::string binary_description;
+if (GetFileSpec()) {
+ binary_description += "path='";
+ binary_description += GetFileSpec().GetPath();
+ binary_description += "' ";
+}
+if (GetUUID()) {
+ binary_description += "uuid=";
+ binary_description += GetUUID().GetAsString();
+}
+LLDB_LOGF(log, "ObjectFileMachO::SetLoadAddress %s",
+ binary_description.c_str());
+ }
if (value_is_offset) {
// "value" is an offset to apply to each top level segment
for (size_t sect_idx = 0; sect_idx < num_sections; ++sect_idx) {
// Iterate through the object file sections to find all of the
// sections that size on disk (to avoid __PAGEZERO) and load them
SectionSP section_sp(section_list->GetSectionAtIndex(sect_idx));
- if (SectionIsLoadable(section_sp.get()))
+ if (SectionIsLoadable(section_sp.get())) {
+LLDB_LOGF(log,
+ "ObjectFileMachO::SetLoadAddress segment '%s' load addr is "
+ "0x%" PRIx64,
+ section_sp->GetName().AsCString(),
+ section_sp->GetFileAddress() + value);
if (target.GetSectionLoadList().SetSectionLoadAddress(
section_sp,
[Lldb-commits] [lldb] [lldb] Change lldb's breakpoint handling behavior (PR #96260)
jasonmolenda wrote: Thanks @AlexK0 ! After Pavel's suggestion, I need to do a minor bit of changes to the patch soon I think, for all platforms, so I'll see if this is easier to express in ProcessWindows that way. I've was thinking about @labath 's suggestion over the weekend and I agree that the way the patch is written today isn't ideal about tracking state at stop-time and resume-time, as I developed the change I had to change its behavior and I didn't rethink what I was doing. The original goal was: If we hit a breakpoint (whether it is enabled for this thread or not), when we Resume the thread, we silently instruction step past that breakpoint instruction before resuming. Then I realized the problem of "user sets a breakpoint at $pc, or changes $pc to a BreakpointSite" and I want to also silently step past the breakpoint in that case, so I added a separate variable to track that. Looking at the total requirements, the rule can be condensed to: If this thread stopped at a BreakpointSite which it did not execute, we should execute the breakpoint on Resume. In any other case, a thread sitting at a BreakpointSite should silently step past it and resume execution. So when a thread stops at a BreakpointSite that has executed (whether it is valid for this thread or not), we record nothing. When a thread stops at a BreakpointSite that has not executed, we need to record the address of that BreakpointSite. And on Resume, if the thread is still at this same address, we want to hit the breakpoint. I don't think we can store this information in the StopInfo easily, because a thread with no stop reason (e.g. a multi-threaded program that hits a breakpoint on one thread, but the others were executing normally) wouldn't have a way to record that they were sitting at a BreakpointSite that needed to be hit still. I outlined the idea of storing this data in the StopInfo to @jimingham earlier today briefly, and he agreed that we should have StopInfos around until we Resume, but I hadn't thought this through enough to account for threads with no stop reason. I'll check in with him about the details on this before I make the change, but I think I need to keep tracking this in the Thread object. https://github.com/llvm/llvm-project/pull/96260 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
