[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL327970: Re-land: [lldb] Use vFlash commands when writing to
target's flash memory… (authored by labath, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D42145?vs=137827&id=139106#toc
Repository:
rL LLVM
https://reviews.llvm.org/D42145
Files:
lldb/trunk/include/lldb/Core/Module.h
lldb/trunk/include/lldb/Host/XML.h
lldb/trunk/include/lldb/Symbol/ObjectFile.h
lldb/trunk/include/lldb/Target/MemoryRegionInfo.h
lldb/trunk/include/lldb/Target/Process.h
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
lldb/trunk/source/Commands/CommandObjectTarget.cpp
lldb/trunk/source/Core/Module.cpp
lldb/trunk/source/Host/common/XML.cpp
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/trunk/source/Symbol/ObjectFile.cpp
lldb/trunk/source/Target/Process.cpp
Index: lldb/trunk/source/Host/common/XML.cpp
===
--- lldb/trunk/source/Host/common/XML.cpp
+++ lldb/trunk/source/Host/common/XML.cpp
@@ -151,6 +151,18 @@
return llvm::StringRef();
}
+bool XMLNode::GetAttributeValueAsUnsigned(const char *name, uint64_t &value,
+ uint64_t fail_value, int base) const {
+#if defined(LIBXML2_DEFINED)
+ llvm::StringRef str_value = GetAttributeValue(name, "");
+#else
+ llvm::StringRef str_value;
+#endif
+ bool success = false;
+ value = StringConvert::ToUInt64(str_value.data(), fail_value, base, &success);
+ return success;
+}
+
void XMLNode::ForEachChildNode(NodeCallback const &callback) const {
#if defined(LIBXML2_DEFINED)
if (IsValid())
Index: lldb/trunk/source/Core/Module.cpp
===
--- lldb/trunk/source/Core/Module.cpp
+++ lldb/trunk/source/Core/Module.cpp
@@ -1685,7 +1685,3 @@
return false;
}
-
-Status Module::LoadInMemory(Target &target, bool set_pc) {
- return m_objfile_sp->LoadInMemory(target, set_pc);
-}
Index: lldb/trunk/source/Target/Process.cpp
===
--- lldb/trunk/source/Target/Process.cpp
+++ lldb/trunk/source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
return 0;
}
+Status Process::WriteObjectFile(std::vector entries) {
+ Status error;
+ for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+ break;
+ }
+ return error;
+}
+
#define USE_ALLOCATE_MEMORY_CACHE 1
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: lldb/trunk/source/Symbol/ObjectFile.cpp
===
--- lldb/trunk/source/Symbol/ObjectFile.cpp
+++ lldb/trunk/source/Symbol/ObjectFile.cpp
@@ -16,7 +16,6 @@
#include "lldb/Symbol/ObjectContainer.h"
#include "lldb/Symbol/SymbolFile.h"
#include "lldb/Target/Process.h"
-#include "lldb/Target/RegisterContext.h"
#include "lldb/Target/SectionLoadList.h"
#include "lldb/Target/Target.h"
#include "lldb/Utility/DataBuffer.h"
@@ -648,41 +647,31 @@
return ConstString(ss.GetString());
}
-Status ObjectFile::LoadInMemory(Target &target, bool set_pc) {
- Status error;
- ProcessSP process = target.CalculateProcess();
- if (!process)
-return Status("No Process");
- if (set_pc && !GetEntryPointAddress().IsValid())
-return Status("No entry address in object file");
-
+std::vector
+ObjectFile::GetLoadableData(Target &target) {
+ std::vector loadables;
SectionList *section_list = GetSectionList();
if (!section_list)
-return Status("No section in object file");
+return loadables;
+ // Create a list of loadable data from loadable sections
size_t section_count = section_list->GetNumSections(0);
for (size_t i = 0; i < section_count; ++i) {
+LoadableData loadable;
SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
- DataExtractor section_data;
- // We can skip sections like bss
- if (section_sp->GetFileSize() == 0)
-continue;
- section_sp->GetSectionData(section_data);
- lldb::offset_t written = process->WriteMemory(
-
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. In https://reviews.llvm.org/D42145#1034499, @labath wrote: > I like this a lot. That's the kind of change I wanted to do as a follow-up > one day. Thank you. Thanks! I don't have commit access to land this. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath accepted this revision. labath added a comment. I like this a lot. That's the kind of change I wanted to do as a follow-up one day. Thank you. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw updated this revision to Diff 137827.
owenpshaw added a comment.
Looking at LoadInMemory, it seems like the common code is actually more
appropriate in the command handler. It's checking inputs and doing the
unrelated task of setting the pc. So here's an attempt at going a little
further to simplify ObjectFile and its dependencies
- Have the command check for a valid process, write to the process, and set the
pc
- Replace ObjectFile::LoadInMemory with GetLoadableData
- Move/rename the Process::WriteEntry struct to ObjectFile::LoadableData
Thoughts?
https://reviews.llvm.org/D42145
Files:
include/lldb/Core/Module.h
include/lldb/Host/XML.h
include/lldb/Symbol/ObjectFile.h
include/lldb/Target/MemoryRegionInfo.h
include/lldb/Target/Process.h
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
source/Commands/CommandObjectTarget.cpp
source/Core/Module.cpp
source/Host/common/XML.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
source/Symbol/ObjectFile.cpp
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
return 0;
}
+Status Process::WriteObjectFile(std::vector entries) {
+ Status error;
+ for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+ break;
+ }
+ return error;
+}
+
#define USE_ALLOCATE_MEMORY_CACHE 1
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -16,7 +16,6 @@
#include "lldb/Symbol/ObjectContainer.h"
#include "lldb/Symbol/SymbolFile.h"
#include "lldb/Target/Process.h"
-#include "lldb/Target/RegisterContext.h"
#include "lldb/Target/SectionLoadList.h"
#include "lldb/Target/Target.h"
#include "lldb/Utility/DataBuffer.h"
@@ -648,41 +647,31 @@
return ConstString(ss.GetString());
}
-Status ObjectFile::LoadInMemory(Target &target, bool set_pc) {
- Status error;
- ProcessSP process = target.CalculateProcess();
- if (!process)
-return Status("No Process");
- if (set_pc && !GetEntryPointAddress().IsValid())
-return Status("No entry address in object file");
-
+std::vector
+ObjectFile::GetLoadableData(Target &target) {
+ std::vector loadables;
SectionList *section_list = GetSectionList();
if (!section_list)
-return Status("No section in object file");
+return loadables;
+ // Create a list of loadable data from loadable sections
size_t section_count = section_list->GetNumSections(0);
for (size_t i = 0; i < section_count; ++i) {
+LoadableData loadable;
SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
- DataExtractor section_data;
- // We can skip sections like bss
- if (section_sp->GetFileSize() == 0)
-continue;
- section_sp->GetSectionData(section_data);
- lldb::offset_t written = process->WriteMemory(
- addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
- if (written != section_data.GetByteSize())
-return error;
-}
- }
- if (set_pc) {
-ThreadList &thread_list = process->GetThreadList();
-ThreadSP curr_thread(thread_list.GetSelectedThread());
-RegisterContextSP reg_context(curr_thread->GetRegisterContext());
-Address file_entry = GetEntryPointAddress();
-reg_context->SetPC(file_entry.GetLoadAddress(&target));
+loadable.Dest =
+target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (loadable.Dest == LLDB_INVALID_ADDRESS)
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+ continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+loadable.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+section_data.GetByteSize());
+loadables.push_back(loadable);
}
- return error;
+ return loadables;
}
void ObjectFile::RelocateSection(lldb_private::Section *section)
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment. Ah, sorry. I didn't get where you were going with your previous comment. The ObjectFile->Process dependency is something that I think we shouldn't have, but whether it's in an header or implementation file makes little difference to me. So I think moving the #include is fine. otoh, since we wen't down the path of having a separate Process::WriteObjectFile method, and the only usage of WriteEntry is there, I can see a case for moving this struct declaration into the ObjectFile class and avoiding this issue altogether. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. It can be done that way. As I mentioned, it would require moving the Process.h import from ObjectFile.cpp to ObjectFile.h (to get the declaration of Process::WriteEntry). I'm asking how you guys feel about that. Without a clear sense of project norms, I'll always go for the less intrusive change, which in this case meant a slightly different signature instead of moving an import that wasn't necessary. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment. I thought the DoLoadInMemory function (which should probably be called differently then) could just return a vector and the actual writing would still happen in the LoadInMemory function. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw updated this revision to Diff 137598.
owenpshaw added a comment.
- Share some common code in LoadInMemory
The DoLoadInMemory implementations call target.CalculateProcess() again, but
passing Process or Process::WriteEntries to DoLoadInMemory would require either
moving the Process.h include to ObjectFile.h or using forward declarations and
pointers. Calling CalculateProcess felt like the lest intrusive option and has
little cost. Open to other options.
https://reviews.llvm.org/D42145
Files:
include/lldb/Host/XML.h
include/lldb/Symbol/ObjectFile.h
include/lldb/Target/MemoryRegionInfo.h
include/lldb/Target/Process.h
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
source/Host/common/XML.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
source/Symbol/ObjectFile.cpp
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
return 0;
}
+Status Process::WriteObjectFile(std::vector entries) {
+ Status error;
+ for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+ break;
+ }
+ return error;
+}
+
#define USE_ALLOCATE_MEMORY_CACHE 1
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -656,25 +656,11 @@
if (set_pc && !GetEntryPointAddress().IsValid())
return Status("No entry address in object file");
- SectionList *section_list = GetSectionList();
- if (!section_list)
-return Status("No section in object file");
- size_t section_count = section_list->GetNumSections(0);
- for (size_t i = 0; i < section_count; ++i) {
-SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
- DataExtractor section_data;
- // We can skip sections like bss
- if (section_sp->GetFileSize() == 0)
-continue;
- section_sp->GetSectionData(section_data);
- lldb::offset_t written = process->WriteMemory(
- addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
- if (written != section_data.GetByteSize())
-return error;
-}
- }
+ error = DoLoadInMemory(target);
+
+ if (!error.Success())
+return error;
+
if (set_pc) {
ThreadList &thread_list = process->GetThreadList();
ThreadSP curr_thread(thread_list.GetSelectedThread());
@@ -685,6 +671,32 @@
return error;
}
+Status ObjectFile::DoLoadInMemory(Target &target) {
+ SectionList *section_list = GetSectionList();
+ if (!section_list)
+return Status("No section in object file");
+
+ std::vector writeEntries;
+ // Create a list of write entries from loadable sections
+ size_t section_count = section_list->GetNumSections(0);
+ for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
+SectionSP section_sp = section_list->GetSectionAtIndex(i);
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+ continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
+ }
+ return target.CalculateProcess()->WriteObjectFile(std::move(writeEntries));
+}
+
void ObjectFile::RelocateSection(lldb_private::Section *section)
{
}
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,8 @@
size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
Status &error) override;
+ Status WriteObjectFile(std::vector entries) override;
+
size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
@@ -302,6 +304,11 @@
int64_t m_br
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added inline comments.
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3500-3518
+ // Create a list of write entries from loadable segments,
+ // using physical addresses if they aren't all null
+ size_t header_count = ParseProgramHeaders();
+ bool should_use_paddr = AnySegmentHasPhysicalAddress();
+ for (size_t i = 1; i <= header_count; ++i) {
+Process::WriteEntry entry;
+auto header = GetProgramHeaderByIndex(i);
This is the only part that's ELF-specific. Could we factor this out into a
separate function (and leave the common part in ObjectFile)? Besides reducing
duplication this will also make it easier to remove the
Target/Process/RegisterContext uses from the ObjectFile in the future, as there
will be just a single place that needs to be updated.
https://reviews.llvm.org/D42145
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw reopened this revision. owenpshaw added a comment. Reopening since the previous land was reverted https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw updated this revision to Diff 137420.
owenpshaw added a comment.
- Revert changes to SetLoadAddress (always use virtual address there)
- Override LoadInMemory in ObjectFileELF to just load segments using physical
address instead of using section load list
Passes tests, but I don't have access to hardware this week to double check
that this works in the real world.
As an effect of this change, the "--load" part of "target modules load" now
ignores the other command arguments for setting section addresses (--slide
and/or the section/addr pairs). --slide would be easy to add back, but would
probably need to become an argument to LoadInMemory. Does anyone have a clear
sense of when someone would want to use --slide in combination with --load?
What about section/add pairs? Should the --load behavior instead be spun off
into a new command?
https://reviews.llvm.org/D42145
Files:
include/lldb/Host/XML.h
include/lldb/Target/MemoryRegionInfo.h
include/lldb/Target/Process.h
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
source/Host/common/XML.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
source/Symbol/ObjectFile.cpp
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
return 0;
}
+Status Process::WriteObjectFile(std::vector entries) {
+ Status error;
+ for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+ break;
+ }
+ return error;
+}
+
#define USE_ALLOCATE_MEMORY_CACHE 1
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -659,22 +659,31 @@
SectionList *section_list = GetSectionList();
if (!section_list)
return Status("No section in object file");
+
+ std::vector writeEntries;
+
+ // Create a list of write entries from loadable sections
size_t section_count = section_list->GetNumSections(0);
for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
- DataExtractor section_data;
- // We can skip sections like bss
- if (section_sp->GetFileSize() == 0)
-continue;
- section_sp->GetSectionData(section_data);
- lldb::offset_t written = process->WriteMemory(
- addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
- if (written != section_data.GetByteSize())
-return error;
-}
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+ continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
}
+
+ error = process->WriteObjectFile(std::move(writeEntries));
+ if (!error.Success())
+return error;
+
if (set_pc) {
ThreadList &thread_list = process->GetThreadList();
ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,8 @@
size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
Status &error) override;
+ Status WriteObjectFile(std::vector entries) override;
+
size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
@@ -302,6 +304,11 @@
int64_t m_breakpoint_pc_offset;
lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
+ bool m_allow_flash_writes;
+ using FlashRangeVector = lldb_private::RangeVector;
+ using FlashRange = FlashRangeVector::Entry;
+ FlashRangeVector m_erase
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. > Since there is just one caller of this function maybe we don't even need to > that fancy. Could the LoadInMemory function do the shifting itself? > I'm thinking of something like where it takes the load bias as the argument > and then adds that to the physical address it obtains from the object file. > Thinking about that, I'm not even sure the function should be operating at > the section level. If it's going to be simulating a loader, shouldn't it be > based on program headers and segments completely (and not just use them for > obtaining the physical address)? The original LoadInMemory thread from over a year ago discusses segments vs sections a little bit: http://lists.llvm.org/pipermail/lldb-dev/2016-December/011758.html. Sounds like gdb used sections, so that was deemed ok for lldb. It also fit with the pre-existing "target modules load" command that allowed section addresses to be specified individually. At one point, Greg suggests: > Since the arguments to this command are free form, we could pass these > arguments to ObjectFile::LoadInMemory() and let each loader (ObjectFileELF > and ObjectFileMach) determine what arguments are valid But that didn't end up in the implementation. Perhaps it should, and ObjectFileELF could disallow any --load that specified sections individually. And if we're doing a special ObjectFileELF::LoadInMemory anyway, it could go ahead and use segments. Repository: rL LLVM https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment. In https://reviews.llvm.org/D42145#1022717, @owenpshaw wrote: > > And I'm not even completely clear about your case. I understand you write > > the module to the physical address, but what happens when you actually go > > around to debugging it? Is it still there or does it get > > copied/mapped/whatever to the virtual address? > > In my case it's a hardware target with ROM and RAM. Only the data section > has different virtual and physical addresses. That data section should be > loaded to its physical address in ROM so it'll be preserved across resets. > The first code to execute on boot copies the data section from ROM to its > virtual address in RAM. Ok, so it seems to me that we do want the rest of the debugger to operate on virtual addresses, as that's what you will be debugging (except when you are actually debugging the boot code itself, but we can't be correct all the time. > It's like we need the original all-virtual SectionLoadList in > ObjectFileELF::SetLoadAddress, but a different SectionLoadList for the > ObjectFile::LoadInMemory case, a function which is only used by "target > modules load --load". > > Or instead instead of a second list, maybe SectionLoadList gets a second pair > of addr<->section maps that contain physical addresses. Then > ObjectFile::LoadInMemory can ask for the physical instead of virtual. Since there is just one caller of this function maybe we don't even need to that fancy. Could the LoadInMemory function do the shifting itself? I'm thinking of something like where it takes the load bias as the argument and then adds that to the physical address it obtains from the object file. Thinking about that, I'm not even sure the function should be operating at the section level. If it's going to be simulating a loader, shouldn't it be based on program headers and segments completely (and not just use them for obtaining the physical address)? Repository: rL LLVM https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. > And I'm not even completely clear about your case. I understand you write the > module to the physical address, but what happens when you actually go around > to debugging it? Is it still there or does it get copied/mapped/whatever to > the virtual address? In my case it's a hardware target with ROM and RAM. Only the data section has different virtual and physical addresses. That data section should be loaded to its physical address in ROM so it'll be preserved across resets. The first code to execute on boot copies the data section from ROM to its virtual address in RAM. > So it seems to me the physical addresses should really be specific to the > "target modules load --load" case. I've never used that command so I don't > know if it can just use physical addresses unconditionally, or whether we > need an extra option for that "target modules load --load --physical". I > think I'll leave that decision up to you (or anyone else who has an opinion > on this). But for the other use cases, I believe we should keep using virtual > addresses. The "target modules load --load" case was originally intended to accomplish what gdb's "load" command does (see https://reviews.llvm.org/D28804). I know gdb uses the physical address when writes the sections. > So I think we should revert this and then split out the loading part of this > patch from the vFlash thingy so we can focus on the virtual/physical address > situation and how to handle that. It's like we need the original all-virtual SectionLoadList in ObjectFileELF::SetLoadAddress, but a different SectionLoadList for the ObjectFile::LoadInMemory case, a function which is only used by "target modules load --load". Or instead instead of a second list, maybe SectionLoadList gets a second pair of addr<->section maps that contain physical addresses. Then ObjectFile::LoadInMemory can ask for the physical instead of virtual. Repository: rL LLVM https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added inline comments. Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:814 value = value - header->p_vaddr; found_offset = true; owenpshaw wrote: > labath wrote: > > Ok so the issue is that here we use the virtual address to compute the load > > bias, but at line 830 we add the bias to the physical address. This breaks > > as soon as these two addresses start to differ. > > > > Changing this to use the physical address as well fixes things, but as I > > said before, I'm not sure we should be using physical addresses here. > I don't know if there's another use case besides flash load that should > definitely use the physical address, so I can't be much help answering that > question. I was mainly relying on tests to catch any issue caused by using > physical addresses. > > Could the value_is_offset flag be a tell here? Looks like that load bias is > only calculated if value_is_offset == false. Would it make sense to always > use virtual address in such a case? It wouldn't affect the "target modules > load" case, which always sets value_is_offset to true. > I don't know if there's another use case besides flash load that should > definitely use the physical address, so I can't be much help answering that > question. I was mainly relying on tests to catch any issue caused by using > physical addresses. Yeah, unfortunately we don't have tests which would catch borderline conditions like that, and as you've said, most elf files have these the same, so you couldn't have noticed that. The only reason I this is that one of the android devices we test on has a kernel whose vdso has these different. The good news is that with your test suite, we should be able to write a test for it, when we figure out what the behavior should be here. > Could the value_is_offset flag be a tell here? Looks like that load bias is > only calculated if value_is_offset == false. Would it make sense to always > use virtual address in such a case? It wouldn't affect the "target modules > load" case, which always sets value_is_offset to true. The is_offset flag is orthogonal to this. It tells you whether the value represents a bias or an absolute value. When we read the module list from the linker rendezvous structure, we get a load bias; when we read it from /proc/pid/maps, we get an absolute value. It does *not* tell you what the value is relative to. So while doing that like you suggest would fix things, I don't think it's the right thing to do. In fact, the more I think about this, the more I'm convinced using physical addresses is not the right solution here. The main user of this function is the dynamic linker plugin, which uses it to notify the debugger about modules that have already been loaded into the process. That definitely sounds like a virtual address thing. Also, if you look at the documentation of the "target modules load --slide", it explicitly states that the slide should be relative to the virtual address. And I'm not even completely clear about your case. I understand you write the module to the physical address, but what happens when you actually go around to debugging it? Is it still there or does it get copied/mapped/whatever to the virtual address? So it seems to me the physical addresses should really be specific to the "target modules load --load" case. I've never used that command so I don't know if it can just use physical addresses unconditionally, or whether we need an extra option for that "target modules load --load --physical". I think I'll leave that decision up to you (or anyone else who has an opinion on this). But for the other use cases, I believe we should keep using virtual addresses. So I think we should revert this and then split out the loading part of this patch from the vFlash thingy so we can focus on the virtual/physical address situation and how to handle that. Repository: rL LLVM https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added inline comments. Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:814 value = value - header->p_vaddr; found_offset = true; labath wrote: > Ok so the issue is that here we use the virtual address to compute the load > bias, but at line 830 we add the bias to the physical address. This breaks as > soon as these two addresses start to differ. > > Changing this to use the physical address as well fixes things, but as I said > before, I'm not sure we should be using physical addresses here. I don't know if there's another use case besides flash load that should definitely use the physical address, so I can't be much help answering that question. I was mainly relying on tests to catch any issue caused by using physical addresses. Could the value_is_offset flag be a tell here? Looks like that load bias is only calculated if value_is_offset == false. Would it make sense to always use virtual address in such a case? It wouldn't affect the "target modules load" case, which always sets value_is_offset to true. Repository: rL LLVM https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added inline comments. Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:814 value = value - header->p_vaddr; found_offset = true; Ok so the issue is that here we use the virtual address to compute the load bias, but at line 830 we add the bias to the physical address. This breaks as soon as these two addresses start to differ. Changing this to use the physical address as well fixes things, but as I said before, I'm not sure we should be using physical addresses here. Repository: rL LLVM https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment. Ok, so this is what seems to be happening: The vdso "file" has the following program headers: Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align LOAD 0x00 0xe000 0x 0x00434 0x00434 R E 0x1000 DYNAMIC0x000324 0xe324 0x0324 0x00090 0x00090 R 0x4 NOTE 0x0001c4 0xe1c4 0x01c4 0x00034 0x00034 R 0x4 GNU_EH_FRAME 0x0001f8 0xe1f8 0x01f8 0x00024 0x00024 R 0x4 Your patch makes the makes us use the physical addresses (because some of the physical addresses are non-null), but in this case that is incorrect. For this file the VirtAddr describes the correct address of the file/section/segment in memory. Also the addresses in the section headers (which is what we were using previously are correct. Going back to your patch, it's not clear to me whether using physical addresses as load addresses is a correct thing to do in general. I mean, generally, we care about the places where the object file is located in virtual memory, and in physical, so maybe we should keep use virtual addresses here, and have ObjectFile::LoadInMemory use physical addresses through some other api? What do you think? That said, on my desktop machine I get the following program headers from the vdso: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align LOAD 0x00 0x 0x 0x00d1a 0x00d1a R E 0x1000 DYNAMIC0x0002bc 0x02bc 0x02bc 0x00090 0x00090 R 0x4 NOTE 0x000560 0x0560 0x0560 0x00060 0x00060 R 0x4 GNU_EH_FRAME 0x0005c0 0x05c0 0x05c0 0x00024 0x00024 R 0x4 and things still work fine, so it may be possible to make this work by just changing how we compute the load bias (I have to look up where we do that). (However, I am still not sure about using physical addresses being the right thing here). Repository: rL LLVM https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment. It seems this messed up the computation of load addresses for symbols in the androids vdso: Symtab, num_symbols = 5: Debug symbol |Synthetic symbol ||Externally Visible ||| Index UserID DSX TypeFile Address/Value Load Address Size Flags Name --- -- --- --- -- -- -- -- -- [0] 1 X Code0xe410 0x0410 0x0008 0x0012 __kernel_rt_sigreturn [1] 2 X Code0xe400 0x0400 0x0009 0x0012 __kernel_sigreturn [2] 3 X Code0xe420 0x0420 0x0014 0x0012 __kernel_vsyscall [3] 4 X Data0x 0x 0x0011 LINUX_2.5 [4] 4 SX Code0xe40f 0x040f 0x0001 0x ___lldb_unnamed_symbol1$$[vdso] (the load address is obviously wrong, and maybe file address as well, although I'm not sure what did we put as a file address for files that are loaded from memory) I'm going to try to investigate more, but I'm writing here in case you have any idea what could be wrong. Repository: rL LLVM https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326261: [lldb] Use vFlash commands when writing to
target's flash memory regions (authored by labath, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D42145?vs=135087&id=136156#toc
Repository:
rL LLVM
https://reviews.llvm.org/D42145
Files:
lldb/trunk/include/lldb/Host/XML.h
lldb/trunk/include/lldb/Target/MemoryRegionInfo.h
lldb/trunk/include/lldb/Target/Process.h
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
lldb/trunk/source/Host/common/XML.cpp
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/trunk/source/Symbol/ObjectFile.cpp
lldb/trunk/source/Target/Process.cpp
Index: lldb/trunk/source/Target/Process.cpp
===
--- lldb/trunk/source/Target/Process.cpp
+++ lldb/trunk/source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
return 0;
}
+Status Process::WriteObjectFile(std::vector entries) {
+ Status error;
+ for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+ break;
+ }
+ return error;
+}
+
#define USE_ALLOCATE_MEMORY_CACHE 1
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: lldb/trunk/source/Symbol/ObjectFile.cpp
===
--- lldb/trunk/source/Symbol/ObjectFile.cpp
+++ lldb/trunk/source/Symbol/ObjectFile.cpp
@@ -659,22 +659,31 @@
SectionList *section_list = GetSectionList();
if (!section_list)
return Status("No section in object file");
+
+ std::vector writeEntries;
+
+ // Create a list of write entries from loadable sections
size_t section_count = section_list->GetNumSections(0);
for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
- DataExtractor section_data;
- // We can skip sections like bss
- if (section_sp->GetFileSize() == 0)
-continue;
- section_sp->GetSectionData(section_data);
- lldb::offset_t written = process->WriteMemory(
- addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
- if (written != section_data.GetByteSize())
-return error;
-}
- }
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+ continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
+ }
+
+ error = process->WriteObjectFile(std::move(writeEntries));
+ if (!error.Success())
+return error;
+
if (set_pc) {
ThreadList &thread_list = process->GetThreadList();
ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
===
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.h
@@ -383,6 +383,12 @@
RefineModuleDetailsFromNote(lldb_private::DataExtractor &data,
lldb_private::ArchSpec &arch_spec,
lldb_private::UUID &uuid);
+
+ bool AnySegmentHasPhysicalAddress();
+
+ const elf::ELFProgramHeader *GetSectionSegment(lldb::SectionSP section_sp);
+
+ lldb::addr_t GetSectionPhysicalAddress(lldb::SectionSP section_sp);
};
#endif // liblldb_ObjectFileELF_h_
Index: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -827,7 +827,7 @@
// of the sections that have SHF_ALLOC in their flag bits.
SectionSP section_sp(section_list->GetSectionAtIndex(sect_idx));
if (section_sp && section_sp->T
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
clayborg added a comment. I agree with Pavel, thanks for taking the time to get this done right and listening to the feedback that was offered. The patch is better for it. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. Thanks! Just a reminder that I don't have commit access, so someone will need to land this for me. Appreciate all the help. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath accepted this revision. labath added a comment. Sorry, I forgot about that. This looks fine as far as I am concerned. Thank you for flying lldb, and in particular, for creating the gdb-client testing framework. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. @labath, any other changes you'd like to see on this one? Skipping the test if there's no xml support seemed to be the final todo. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added inline comments. Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3478 +auto header = GetProgramHeaderByIndex(i); +if (header->p_paddr != 0) + return true; clayborg wrote: > Can't p_paddr be zero and still be valid? Would a better test be > "header->p_memsz > 0"? Yes, a 0 paddr can be valid. This check will only ignore paddr if all segments have a 0 paddr, suggesting the linker didn't populate that field. This decision is similar to what I did in llvm-objcopy for binary output, which in turn was based on GNU objcopy behavior: https://bugs.llvm.org/show_bug.cgi?id=35708#c4 https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
clayborg added inline comments. Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3478 +auto header = GetProgramHeaderByIndex(i); +if (header->p_paddr != 0) + return true; Can't p_paddr be zero and still be valid? Would a better test be "header->p_memsz > 0"? https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw updated this revision to Diff 135087.
owenpshaw added a comment.
Herald added a subscriber: arichardson.
Update patch to include new @skipIfXmlSupportMissing decorator on test
https://reviews.llvm.org/D42145
Files:
include/lldb/Host/XML.h
include/lldb/Target/MemoryRegionInfo.h
include/lldb/Target/Process.h
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
source/Host/common/XML.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
source/Symbol/ObjectFile.cpp
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
return 0;
}
+Status Process::WriteObjectFile(std::vector entries) {
+ Status error;
+ for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+ break;
+ }
+ return error;
+}
+
#define USE_ALLOCATE_MEMORY_CACHE 1
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -659,22 +659,31 @@
SectionList *section_list = GetSectionList();
if (!section_list)
return Status("No section in object file");
+
+ std::vector writeEntries;
+
+ // Create a list of write entries from loadable sections
size_t section_count = section_list->GetNumSections(0);
for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
- DataExtractor section_data;
- // We can skip sections like bss
- if (section_sp->GetFileSize() == 0)
-continue;
- section_sp->GetSectionData(section_data);
- lldb::offset_t written = process->WriteMemory(
- addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
- if (written != section_data.GetByteSize())
-return error;
-}
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+ continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
}
+
+ error = process->WriteObjectFile(std::move(writeEntries));
+ if (!error.Success())
+return error;
+
if (set_pc) {
ThreadList &thread_list = process->GetThreadList();
ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,8 @@
size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
Status &error) override;
+ Status WriteObjectFile(std::vector entries) override;
+
size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
@@ -302,6 +304,11 @@
int64_t m_breakpoint_pc_offset;
lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
+ bool m_allow_flash_writes;
+ using FlashRangeVector = lldb_private::RangeVector;
+ using FlashRange = FlashRangeVector::Entry;
+ FlashRangeVector m_erased_flash_ranges;
+
//--
// Accessors
//--
@@ -408,6 +415,12 @@
Status UpdateAutomaticSignalFiltering() override;
+ Status FlashErase(lldb::addr_t addr, size_t size);
+
+ Status FlashDone();
+
+ bool HasErased(FlashRange range);
+
private:
//--
// For ProcessGDBRemote only
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -59,6 +59,7 @@
#incl
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment. In https://reviews.llvm.org/D42145#996575, @owenpshaw wrote: > - adjust WriteObjectFile signature to return Status and take an std::vector > - match project formatting style > > I toyed with adding allow_flash to as an argument, but didn't really like > it because it seemed to bring things back to basically allowing arbitrary > flash writes via WriteMemory, and would affect all the process subclasses > when only one (gdb) actually needs it. I still like the member flag because > it keeps the flash writing very private to ProcessGDB. If we called the argument `writing_object_file` instead of `allow_flash`, then maybe we wouldn't be exposing flash details, although i'm not sure if it makes much of a difference. I guess we can keep the member flag for now... > What can I do to get the xml dependency check in the test? Not clear on who > I should talk to or where to start. Solution A would be to add some cmake code like configure_file(dotest_build_config.py.in dotest_build_config.py) which would generate the config file with the xml flag substituted. Then, somewhere from the test you could load this file and check the value. Solution B would be to add a function (static function on SBDebugger class maybe) which you could call from the test to query the properties the binary was built with. I can start a thread on lldb-dev to see what people prefer. If noone cares enough to respond, then I guess you can pick whichever option you want. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw updated this revision to Diff 132641.
owenpshaw added a comment.
- adjust WriteObjectFile signature to return Status and take an std::vector
- match project formatting style
I toyed with adding allow_flash to as an argument, but didn't really like it
because it seemed to bring things back to basically allowing arbitrary flash
writes via WriteMemory, and would affect all the process subclasses when only
one (gdb) actually needs it. I still like the member flag because it keeps the
flash writing very private to ProcessGDB.
What can I do to get the xml dependency check in the test? Not clear on who I
should talk to or where to start.
https://reviews.llvm.org/D42145
Files:
include/lldb/Host/XML.h
include/lldb/Target/MemoryRegionInfo.h
include/lldb/Target/Process.h
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
source/Host/common/XML.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
source/Symbol/ObjectFile.cpp
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
return 0;
}
+Status Process::WriteObjectFile(std::vector entries) {
+ Status error;
+ for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+ break;
+ }
+ return error;
+}
+
#define USE_ALLOCATE_MEMORY_CACHE 1
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -659,22 +659,31 @@
SectionList *section_list = GetSectionList();
if (!section_list)
return Status("No section in object file");
+
+ std::vector writeEntries;
+
+ // Create a list of write entries from loadable sections
size_t section_count = section_list->GetNumSections(0);
for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
- DataExtractor section_data;
- // We can skip sections like bss
- if (section_sp->GetFileSize() == 0)
-continue;
- section_sp->GetSectionData(section_data);
- lldb::offset_t written = process->WriteMemory(
- addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
- if (written != section_data.GetByteSize())
-return error;
-}
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+ continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
}
+
+ error = process->WriteObjectFile(std::move(writeEntries));
+ if (!error.Success())
+return error;
+
if (set_pc) {
ThreadList &thread_list = process->GetThreadList();
ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,8 @@
size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
Status &error) override;
+ Status WriteObjectFile(std::vector entries) override;
+
size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
@@ -302,6 +304,11 @@
int64_t m_breakpoint_pc_offset;
lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
+ bool m_allow_flash_writes;
+ using FlashRangeVector = lldb_private::RangeVector;
+ using FlashRange = FlashRangeVector::Entry;
+ FlashRangeVector m_erased_flash_ranges;
+
//--
// Accessors
//--
@@ -408,6 +415,12 @@
Status UpdateAutomaticSignalFiltering() override;
+ Status FlashErase(lldb::addr_t
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment. Yes, it's mostly stylistic differences, but there is one more important thing that we need to figure out, and that's making sure that the test only runs if we actually have XML support compiled-in. If there's isn't anything in the SB API that would tell us that (I cursory look doesn't reveal anything), then we may need to get some help from the build system for that. OTOH, maybe an new SB API for determining the lldb feature set would actually be useful... Comment at: include/lldb/Target/Process.h:1958 + virtual bool WriteObjectFile(llvm::ArrayRef entries, + Status &error); owenpshaw wrote: > labath wrote: > > This (return bool + by-ref Status) is a bit weird of an api. Could you just > > return Status here (but I would not be opposed to going llvm all the way > > and returning `llvm::Error`). > Open to whatever. I preferred how it made calling code a little simpler. > > ``` > if (!WriteObjectFile(entries, error)) > return error; > ``` > > rather than > > > ``` > error = WriteObjectFile(entries); > if (!error.Sucess()) > return error > ``` That extra line is a bit of a nuissance. We could fix that, but as the long term solution is to use llvm::Error everywhere, i'm not motivated to do that. So if the extra line bothers you, just use llvm::Error. Then the call-size becomes: ``` if (llvm::Error E = WriteObjectFile(...)) return Status(std::move(E)); ``` The constant conversion between Status and Error is a bit annoying, but as more places start using this, it should get better. (My main issue with your syntax is that it's not DRY) Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2807 + // memory, must happen in order of increasing address. + std::vector sortedEntries(entries); + std::stable_sort(std::begin(sortedEntries), std::end(sortedEntries), owenpshaw wrote: > labath wrote: > > Let's avoid copying the entries here. I can see two options: > > - Require that the entries are already sorted on input > > - pass them to this function as `std::vector` (and then have > > the caller call with `std::move(entries)`). > I didn't love the copy either, but figured 1) it was pretty cheap given the > expected values 2) it eliminates an unexpected modification of the passed > array. I prefer having the sort in the process file, since it's really the > only scope that knows about the sorting requirement. I agree it likely to be cheap, but the thing you fear (2) can be easily avoided. Note that I deliberately did not add any reference qualifications to the type in the comment above. If you take the argument as a value type, then you leave it up to the caller to decide whether a copy takes place. If he calls it as `WriteMemoryObject(my_vector)`, then it's copied (and the original is left unmodified). If he calls it as `WriteMemoryObject(std::move(my_vector))`, then it's moved (but that's very obvious from looking at the call-site). So in the worst case, you get one copy, just like you would here, and in the best case, you get no copies. In your case, the caller doesn't need the vector, so we save a copy. To me, that makes this option a clear win. Isn't C++11 great? :D Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2812-2821 + m_allow_flash_writes = true; + if (Process::WriteObjectFile(sortedEntries, error)) +error = FlashDone(); + else +// Even though some of the writing failed, try to send a flash done if +// some of the writing succeeded so the flash state is reset to normal, +// but don't stomp on the error status that was set in the write failure owenpshaw wrote: > labath wrote: > > This makes the control flow quite messy. The base function is not so > > complex that you have to reuse it at all costs. I think we should just > > implement the loop ourselves (and call some write function while passing > > the "allow_flash_writes" as an argument). > Guess we have different definitions of messy :) > > Wouldn't any other approach pretty much require duplicating WriteMemory, > WriteMemoryPrivate, and DoWriteMemory? > > - It seemed like the breakpoint logic in WriteMemory could be important when > writing an object to ram, so I wanted to preserve that > - The loop in WriteMemoryPrivate is fairly trivial, but why add a second one > if not needed? > - DoWriteMemory is mostly code that is common to both ram and flash writes, > especially if a ram-only version would still need to check if address is > flash so it could report an error. > Yeah, I think I'll have to agree with you here, although this makes the whole separate-function approach look far less attractive to me. If we are going to reuse all of that logic that then we might make it obvious that we are doing that by having a bool flag on those functions like Greg suggested a couple of comments back.
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. Thanks! Overall it's feeling like we're getting down to mainly differences in personal preferences and judgements. Not sure if you're looking for a discussion, which I'm happy to have, if you're just looking for changes to definitely be made. If it's the latter, I think it'd be more efficient to just hand this off so the changes can be made without a lot of back and forth. Comment at: include/lldb/Target/Process.h:1958 + virtual bool WriteObjectFile(llvm::ArrayRef entries, + Status &error); labath wrote: > This (return bool + by-ref Status) is a bit weird of an api. Could you just > return Status here (but I would not be opposed to going llvm all the way and > returning `llvm::Error`). Open to whatever. I preferred how it made calling code a little simpler. ``` if (!WriteObjectFile(entries, error)) return error; ``` rather than ``` error = WriteObjectFile(entries); if (!error.Sucess()) return error ``` Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2807 + // memory, must happen in order of increasing address. + std::vector sortedEntries(entries); + std::stable_sort(std::begin(sortedEntries), std::end(sortedEntries), labath wrote: > Let's avoid copying the entries here. I can see two options: > - Require that the entries are already sorted on input > - pass them to this function as `std::vector` (and then have the > caller call with `std::move(entries)`). I didn't love the copy either, but figured 1) it was pretty cheap given the expected values 2) it eliminates an unexpected modification of the passed array. I prefer having the sort in the process file, since it's really the only scope that knows about the sorting requirement. Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2812-2821 + m_allow_flash_writes = true; + if (Process::WriteObjectFile(sortedEntries, error)) +error = FlashDone(); + else +// Even though some of the writing failed, try to send a flash done if +// some of the writing succeeded so the flash state is reset to normal, +// but don't stomp on the error status that was set in the write failure labath wrote: > This makes the control flow quite messy. The base function is not so complex > that you have to reuse it at all costs. I think we should just implement the > loop ourselves (and call some write function while passing the > "allow_flash_writes" as an argument). Guess we have different definitions of messy :) Wouldn't any other approach pretty much require duplicating WriteMemory, WriteMemoryPrivate, and DoWriteMemory? - It seemed like the breakpoint logic in WriteMemory could be important when writing an object to ram, so I wanted to preserve that - The loop in WriteMemoryPrivate is fairly trivial, but why add a second one if not needed? - DoWriteMemory is mostly code that is common to both ram and flash writes, especially if a ram-only version would still need to check if address is flash so it could report an error. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
clayborg added a comment. In https://reviews.llvm.org/D42145#994556, @labath wrote: > I think we're slowly getting there, but we could cleanup the implementation a > bit. > > I am also not sure that the `WriteObjectFile` name really captures what this > function does, but I don't have a suggestion for a better name either Looking at the API I wouldn't immediately assume that WriteObjectFile would be flash aware and WriteMemory wouldn't... How about we add a new parameter to Process::WriteMemory that like "bool write_flash". It can default to false. Then if this is true, then we do the extra legwork to try and figure out the memory map. If false, just write the memory. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment.
I think we're slowly getting there, but we could cleanup the implementation a
bit.
I am also not sure that the `WriteObjectFile` name really captures what this
function does, but I don't have a suggestion for a better name either
Comment at: include/lldb/Target/Process.h:559
+ struct WriteEntry{
+lldb::addr_t Dest;
Please run clang format over your diff.
Comment at: include/lldb/Target/Process.h:1958
+ virtual bool WriteObjectFile(llvm::ArrayRef entries,
+ Status &error);
This (return bool + by-ref Status) is a bit weird of an api. Could you just
return Status here (but I would not be opposed to going llvm all the way and
returning `llvm::Error`).
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2807
+ // memory, must happen in order of increasing address.
+ std::vector sortedEntries(entries);
+ std::stable_sort(std::begin(sortedEntries), std::end(sortedEntries),
Let's avoid copying the entries here. I can see two options:
- Require that the entries are already sorted on input
- pass them to this function as `std::vector` (and then have the
caller call with `std::move(entries)`).
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2812-2821
+ m_allow_flash_writes = true;
+ if (Process::WriteObjectFile(sortedEntries, error))
+error = FlashDone();
+ else
+// Even though some of the writing failed, try to send a flash done if
+// some of the writing succeeded so the flash state is reset to normal,
+// but don't stomp on the error status that was set in the write failure
This makes the control flow quite messy. The base function is not so complex
that you have to reuse it at all costs. I think we should just implement the
loop ourselves (and call some write function while passing the
"allow_flash_writes" as an argument).
https://reviews.llvm.org/D42145
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw updated this revision to Diff 132260.
owenpshaw added a comment.
- Remove Begin/End memory batch API
- Add Process::WriteObjectFile(llvm::ArrayRef) method. Open to
other names since it's not necessarily specific to object files, but wanted to
somehow indicate that the method may do an uncommon write (like write to flash).
- Kept vFlashWrite as part of DoWriteMemory, and changed the is_batch_write
flag to an allow_flash_writes flag. ProcessGDB:: WriteObjectFile sets
allow_flash_writes, and then calls the regular WriteMemory. Didn't seem like
there was a need to duplicate the WriteMemory logic.
- Any other memory write attempts to flash regions will now fail
I think this gets us closer to what we're talking about, but let me know if
anything should change.
https://reviews.llvm.org/D42145
Files:
include/lldb/Host/XML.h
include/lldb/Target/MemoryRegionInfo.h
include/lldb/Target/Process.h
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
source/Host/common/XML.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
source/Symbol/ObjectFile.cpp
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,18 @@
return 0;
}
+bool Process::WriteObjectFile(llvm::ArrayRef entries,
+ Status &error) {
+ error.Clear();
+ for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+ break;
+ }
+ return error.Success();
+}
+
#define USE_ALLOCATE_MEMORY_CACHE 1
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -659,22 +659,30 @@
SectionList *section_list = GetSectionList();
if (!section_list)
return Status("No section in object file");
+
+ std::vector writeEntries;
+
+ // Create a list of write entries from loadable sections
size_t section_count = section_list->GetNumSections(0);
for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
- DataExtractor section_data;
- // We can skip sections like bss
- if (section_sp->GetFileSize() == 0)
-continue;
- section_sp->GetSectionData(section_data);
- lldb::offset_t written = process->WriteMemory(
- addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
- if (written != section_data.GetByteSize())
-return error;
-}
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+ continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
}
+
+ if (!process->WriteObjectFile(writeEntries, error))
+return error;
+
if (set_pc) {
ThreadList &thread_list = process->GetThreadList();
ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,9 @@
size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
Status &error) override;
+ bool WriteObjectFile(llvm::ArrayRef entries, Status &error)
+ override;
+
size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
@@ -302,6 +305,11 @@
int64_t m_breakpoint_pc_offset;
lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
+ bool m_allow_flash_writes;
+ using FlashRangeVector = lldb_private::RangeVector;
+ using FlashRange = FlashRangeVector::Entry;
+ FlashRangeVector m_erased_flash_ranges;
+
//--
// Accessors
//---
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. > How does the flash memory behave from the POV of the debugged process? Just tested on a couple targets and they both silently failed to change the flash memory. So no exception, but no flash write either. Unless there are objections, I'll work on some changes along the lines you described. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment. In https://reviews.llvm.org/D42145#992289, @owenpshaw wrote: > > So the main question is: do we want WriteMemory to work anywhere and always > > try to do the right thing, or return an error an the user would be expected > > to know to check the memory region you are writing to and know to call > > "Process::WriteFlash(...)". I vote to keep things simple and have > > Process::WriteMemory() just do the right thing. I am open to differing > > opinions, so let me know what you guys think. > > What are other use cases for writing to flash memory from lldb? Since I > can't think of any, I'm biased towards keeping this implementation simple and > narrowly focused on object loading, which means not worrying about preserving > the unwritten parts of erased blocks, and giving an error for other write > attempts to flash regions. How does the flash memory behave from the POV of the debugged process? E.g. if it does something like: char *Flash = /*pointer to flash memory*/ *Flash = 47; will that succeed, or will it get some exception? I'm guessing it's the latter.. If that's the case, then I'm also leaning towards *not* making WriteMemory "just work", as we cannot make the existence of flash memory transparent without e.g. fixing how breakpoint setting in the flashed region works. However, I'm not sure it should be the responsibility of the ObjectFile to know about the various memory kinds. Maybe we could still keep that in the process class. We could have the ObjectFile create a structure describing the desired memory layout (just like in the batch-WriteMemory version), and then pass it to some Process function, which would handle the flash details. (So in essence, the implementation would remain the same, you just wouldn't call the function WriteMemory, but something else). https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. > So the main question is: do we want WriteMemory to work anywhere and always > try to do the right thing, or return an error an the user would be expected > to know to check the memory region you are writing to and know to call > "Process::WriteFlash(...)". I vote to keep things simple and have > Process::WriteMemory() just do the right thing. I am open to differing > opinions, so let me know what you guys think. What are other use cases for writing to flash memory from lldb? Since I can't think of any, I'm biased towards keeping this implementation simple and narrowly focused on object loading, which means not worrying about preserving the unwritten parts of erased blocks, and giving an error for other write attempts to flash regions. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
clayborg added a comment. In https://reviews.llvm.org/D42145#991985, @owenpshaw wrote: > This discussion has got me questioning if WriteMemory is even the best place > to put the flash commands. It seems like some of the concern here is around > the behavior of arbitrary memory writes to flash regions, which isn't really > the main objective of this patch (supporting object file loading into flash). I am ok with having an extra interface on Process if needed. The ObjectFile::Load would use this new API to load things. But most targets aren't going to have flash and it would be weird if a target with a ton of RAM and no flash had to worry about that. Then ObjectFile::Load would need to be able to get memory region info, detect where flash is and where it isn't, and submit either a WriteMemory to WriteFlash command. I am ok with that if this makes things cleaner. > I did a little testing with gdb, and it doesn't do arbitrary memory writes to > flash regions. Using the set command on a flash address, for example, just > gives an error that flash writes are not allowed. Perhaps that's a better > way to go than worrying about supporting one-off flash writes. That would be fine. > > > > >> I still like my WriteMemory(ArrayRef) proposal the >> best. > > The one difference I see between a single function and begin/end is that > begin/end doesn't require all the bytes to be read into host memory up front, > and can instead read the source bytes in chunks. Perhaps that's not really a > reason for concern, though. RAM is cheap these days and if the file is large, then mmap it, No RAM required. I wouldn't worry about that for now. So the main question is: do we want WriteMemory to work anywhere and always try to do the right thing, or return an error an the user would be expected to know to check the memory region you are writing to and know to call "Process::WriteFlash(...)". I vote to keep things simple and have Process::WriteMemory() just do the right thing. I am open to differing opinions, so let me know what you guys think. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
clayborg added a comment. In https://reviews.llvm.org/D42145#991459, @owenpshaw wrote: > Thanks. What I'm struggling to reconcile are your statements that users > should not have to know how things must happen, but then that we should make > ObjectFile::Load smart so it doesn't result in an unnecessary amount of > reads/writes. If writing to flash memory effectively requires some smarts on > the caller's part, then how have we made things easier for the callers? We don't have to, we can let them submit each memory write and do things inefficiently. I am just saying it isn't too hard in ObjectFile::Load to gather large contiguous writes and submit them as one Process::WriteMemory. Pavel's suggestion that we send an array of regions would allow us to do this internally in the Process::WriteMemory(regions) call. > And at that point isn't start/end a lot simpler than requiring callers to > worry about block sizes themselves? There are no block size requirements here, WriteMemory can either do the write memory or error out. Just submit the largest contiguous write possible. Not sure what the issue is here? https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. This discussion has got me questioning if WriteMemory is even the best place to put the flash commands. It seems like some of the concern here is around the behavior of arbitrary memory writes to flash regions, which isn't really the main objective of this patch (supporting object file loading into flash). I did a little testing with gdb, and it doesn't do arbitrary memory writes to flash regions. Using the set command on a flash address, for example, just gives an error that flash writes are not allowed. Perhaps that's a better way to go than worrying about supporting one-off flash writes. > I still like my WriteMemory(ArrayRef) proposal the > best. The one difference I see between a single function and begin/end is that begin/end doesn't require all the bytes to be read into host memory up front, and can instead read the source bytes in chunks. Perhaps that's not really a reason for concern, though. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment. I still like my `WriteMemory(ArrayRef)` proposal the best. If you don't have any special writing requirements, it can be viewed as a convenience wrapper for the one-shot `WriteMemory`, and if a subclass needs special handling, it can implement it in a smarter way (while making sure that the one-shot call still "does the right thing", just in a slower way). It also makes it impossible to forget to call `EndBatch()`. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. Thanks. What I'm struggling to reconcile are your statements that users should not have to know how things must happen, but then that we should make ObjectFile::Load smart so it doesn't result in an unnecessary amount of reads/writes. If writing to flash memory effectively requires some smarts on the caller's part, then how have we made things easier for the callers? And at that point isn't start/end a lot simpler than requiring callers to worry about block sizes themselves? https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
clayborg added a comment. In https://reviews.llvm.org/D42145#979629, @owenpshaw wrote: > I'm not envisioning that anything else needs to change to use begin/end or > care it's there. I guess the way I look at it, having > ObjectFile::LoadInMemory do begin/end is basically the same as what you're > saying about having it be more process aware. > > If Process is going to introduce new concepts for ObjectFile to use either > way, isn't a high level of abstraction (batched writes) preferable to > ObjectFile needing to know the fine details of flash memory blocks or that > flash is even used? And doesn't keeping the heavy lifting in ProcessGDB make > it reusable should another case come along? > > Hope you don't mind the pushback. I think we're looking at this from > different angles, and I'm genuinely asking to help my understanding of your > thinking so hopefully we can converge on the same view. I really would like users to not have to know how things must happen and that they must use a batch start and batch end. I strongly believe we should have Process::WriteMemory just do the right thing. We can make ObjectFile::Load() be smart and make sure to batch all consecutive writes to avoid unnecessary erases. I just really want memory write to just work. No questions. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. Hi Greg, I got distracted from this one for a bit. Maybe I missed it, but do you have any thoughts on my previous comment/question about the batch API vs other options? Thanks, Owen https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment. I'm not envisioning that anything else needs to change to use begin/end or care it's there. I guess the way I look at it, having ObjectFile::LoadInMemory do begin/end is basically the same as what you're saying about having it be more process aware. If Process is going to introduce new concepts for ObjectFile to use either way, isn't a high level of abstraction (batched writes) preferable to ObjectFile needing to know the fine details of flash memory blocks or that flash is even used? And doesn't keeping the heavy lifting in ProcessGDB make it reusable should another case come along? Hope you don't mind the pushback. I think we're looking at this from different angles, and I'm genuinely asking to help my understanding of your thinking so hopefully we can converge on the same view. Comment at: include/lldb/Target/MemoryRegionInfo.h:109-110 ConstString m_name; + OptionalBool m_flash; + lldb::offset_t m_blocksize; }; clayborg wrote: > Could these two be combined into one entry? Could "m_blocksize" be > "m_flash_block_size" and if the value is zero, then it isn't flash? Sure, I can change that. Almost went that way initially, but thought the two fields made it a little clearer, and allowed for an error condition when flash is true, but the blocksize is invalid (0). Collapsing to one field means falling back to a regular memory write rather than an error. Not sure if it's a big deal either way. https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
clayborg added a comment. My objection to the BeginWriteMemoryBatch and EndWriteMemoryBatch is users must know to emit these calls when they really just want to call process.WriteMemory() and not worry about how it is done. Much like writing to read only code when setting breakpoints, we don't require the user to know that they must check the memory permissions first, set permissions to be writable and then revert them back after writing to a memory location. If we have the ability to do things correctly, then we should and we shouldn't require people to know about calls that are specific to flash. Just make it happen if you can, else return an error. So I would rather see the ObjectFile::Load become more process aware where it gets the memory region info and efficiently loads memory as needed by watching the m_blocksize than require everyone else that might want to write memory to know to start a batch and end a batch. It comes down to keeping the API simple. Comment at: include/lldb/Target/MemoryRegionInfo.h:109-110 ConstString m_name; + OptionalBool m_flash; + lldb::offset_t m_blocksize; }; Could these two be combined into one entry? Could "m_blocksize" be "m_flash_block_size" and if the value is zero, then it isn't flash? https://reviews.llvm.org/D42145 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw updated this revision to Diff 130258.
owenpshaw added a comment.
- Split testing base into separate review https://reviews.llvm.org/D42195
- Use StreamGDBRemote instead of duplicate new function
- Move qXfer:memory-map xml parsing into GDBRemoteCommunicationClient
- Include qXfer:memory-map data in GetMemoryRegionInfo, instead of creating a
separate API
I've left the batch write as-is for now because I'm not quite sold on the
solution of writing entire blocks of flash memory at time. While I agree it
could work, it feels like an overly complicated code solution that will also
generate a lot of unnecessary commands between the client and server.
Can you help me understand the objection to the begin/end design? I can't
really think of scenario besides object file load where the begin/end batching
calls would be used. So perhaps instead of ObjectFile::LoadInMemory using
Process::WriteMemory, it instead calls something like a new
Process::WriteObjectSections(std::vector, SectionLoadList) method
that's clearly only intended for object writing. The GDB process could
override and do the begin/end batching privately.
https://reviews.llvm.org/D42145
Files:
include/lldb/Host/XML.h
include/lldb/Target/MemoryRegionInfo.h
include/lldb/Target/Process.h
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
source/Host/common/XML.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
source/Symbol/ObjectFile.cpp
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -659,22 +659,47 @@
SectionList *section_list = GetSectionList();
if (!section_list)
return Status("No section in object file");
+
+ // Filter the list of loadable sections
+ std::vector loadable_sections;
size_t section_count = section_list->GetNumSections(0);
for (size_t i = 0; i < section_count; ++i) {
SectionSP section_sp = section_list->GetSectionAtIndex(i);
addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
- DataExtractor section_data;
- // We can skip sections like bss
- if (section_sp->GetFileSize() == 0)
-continue;
- section_sp->GetSectionData(section_data);
- lldb::offset_t written = process->WriteMemory(
- addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
- if (written != section_data.GetByteSize())
-return error;
+if (addr == LLDB_INVALID_ADDRESS)
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+ continue;
+loadable_sections.push_back(section_sp);
+ }
+
+ // Sort the sections by address because some writes, like those to flash
+ // memory, must happen in order of increasing address.
+ std::stable_sort(std::begin(loadable_sections), std::end(loadable_sections),
+ [&target](const SectionSP a, const SectionSP b){
+addr_t addr_a = target.GetSectionLoadList().GetSectionLoadAddress(a);
+addr_t addr_b = target.GetSectionLoadList().GetSectionLoadAddress(b);
+return addr_a < addr_b;
+ });
+
+ // Do a batch memory write to the process
+ if (!process->BeginWriteMemoryBatch())
+return Status("Could not start write memory batch");
+ for (auto §ion_sp : loadable_sections) {
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+lldb::offset_t written = process->WriteMemory(
+addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
+if (written != section_data.GetByteSize()) {
+ process->EndWriteMemoryBatch();
+ return error;
}
}
+ if (!process->EndWriteMemoryBatch())
+return Status("Could not end write memory batch");
+
if (set_pc) {
ThreadList &thread_list = process->GetThreadList();
ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,10 @@
size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
Status &error) override;
+ bool BeginWriteMemoryBatch() override;
+
+ bool EndWriteMemoryBatch() override;
+
size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
@@ -302,6 +306,11 @@
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
clayborg added inline comments.
Comment at: include/lldb/Target/Process.h:1916
+ //--
+ virtual bool BeginWriteMemoryBatch() { return true; }
+
owenpshaw wrote:
> clayborg wrote:
> > labath wrote:
> > > Instead of this begin/end combo, what would you think about a WriteMemory
> > > overload that takes a list of memory regions?
> > > Something like:
> > > ```
> > > struct WriteEntry { addr_t Dest; llvm::ArrayRef Contents; };
> > > Status WriteMemory(llvm::ArrayRef);
> > > ```
> > > Then the default implementation can just lower this into regular
> > > `WriteMemory` sequence, and the gdb-remote class can add the extra
> > > packets around it when needed.
> > Do we really need this? Can't we just take care of it inside of the
> > standard Process::WriteMemory()? This doesn't seem like something that a
> > user should have to worry about. The process plug-in should just make the
> > write happen IMHO. Let me know.
> Maybe I'm misunderstanding the flash commands, but I couldn't figure a way
> around the need to somehow indicate that several writes are batched together.
> The reason has to do with how vFlashErase must erase an entire block at a
> time.
>
> ObjectFile::LoadInMemory makes one Process::WriteMemory call per section. If
> each WriteMemory call was self-contained, and two sections are in the same
> flash block, it would go something like this:
>
> # WriteMemory (section 1)
> ## DoWriteMemory
> ### vFlashErase (block 1)
> ### vFlashWrite (section 1)
> ### vFlashDone
> # WriteMemory (section 2)
> ## DoWriteMemory
> ### vFlashErase (block 1, again)
> ### vFlashWrite (section 2)
> ### vFlashDone
>
> Wouldn't the second erase undo the first write?
>
> I found the begin/end to be the least intrusive way around, but I'm open to
> other options.
Seems like any block write that isn't writing an entire block should read the
contents that won't be overwritten, create a block's worth of data to write by
using the pre-existing data and adding any new data, then erase the block and
and rewrite the entire block. Then in ObjectFile::Load() it would batch up any
consecutive writes to minimize any block erasing...
Comment at: source/Symbol/ObjectFile.cpp:671
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
owenpshaw wrote:
> clayborg wrote:
> > labath wrote:
> > > You're not actually changing this part, but it caught my eye, and you may
> > > care about this:
> > >
> > > Is it true that we can ignore .bss here? Programs generally assume that
> > > .bss is zero-initialized, and if you just ignore it here, it can contain
> > > whatever garbage was in the memory before you loaded (?)
> > Very important to zero out bss
> For the embedded projects I've worked on, ignoring .bss here is fine and
> expected because the code always starts by zeroing out the .bss memory (and
> also copying the .data section contents from ROM to RAM, which is related to
> the physical address change).
>
> For comparison, gdb doesn't try to load the bss section either.
Sounds good, if gdb doesn't do it, then I am fine if we don't do it
https://reviews.llvm.org/D42145
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw added a comment.
Thanks for the feedback, I'm working on splitting out the testing base into
another patch and making the requested changes. My main unresolved question is
about the write batching, with details below.
Comment at: include/lldb/Target/Process.h:1916
+ //--
+ virtual bool BeginWriteMemoryBatch() { return true; }
+
clayborg wrote:
> labath wrote:
> > Instead of this begin/end combo, what would you think about a WriteMemory
> > overload that takes a list of memory regions?
> > Something like:
> > ```
> > struct WriteEntry { addr_t Dest; llvm::ArrayRef Contents; };
> > Status WriteMemory(llvm::ArrayRef);
> > ```
> > Then the default implementation can just lower this into regular
> > `WriteMemory` sequence, and the gdb-remote class can add the extra packets
> > around it when needed.
> Do we really need this? Can't we just take care of it inside of the standard
> Process::WriteMemory()? This doesn't seem like something that a user should
> have to worry about. The process plug-in should just make the write happen
> IMHO. Let me know.
Maybe I'm misunderstanding the flash commands, but I couldn't figure a way
around the need to somehow indicate that several writes are batched together.
The reason has to do with how vFlashErase must erase an entire block at a time.
ObjectFile::LoadInMemory makes one Process::WriteMemory call per section. If
each WriteMemory call was self-contained, and two sections are in the same
flash block, it would go something like this:
# WriteMemory (section 1)
## DoWriteMemory
### vFlashErase (block 1)
### vFlashWrite (section 1)
### vFlashDone
# WriteMemory (section 2)
## DoWriteMemory
### vFlashErase (block 1, again)
### vFlashWrite (section 2)
### vFlashDone
Wouldn't the second erase undo the first write?
I found the begin/end to be the least intrusive way around, but I'm open to
other options.
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4612-4613
+bool ProcessGDBRemote::GetMemoryMap(
+std::vector ®ion_list) {
+
clayborg wrote:
> Can we cache this value permanently? If so we should read it once and then
> the GDBRemoteCommunicationClient::GetMemoryRegionInfo() should try the
> qMemoryRegionInfo packet (if supported) and possibly augment the memory
> region results with this info?
>
> This should be pushed down into GDBRemoteCommunicationClient and it should
> keep a member variable that caches these results.
>
> Any reason we are using shared pointers to lldb::MemoryRegionInfo? They are
> simple structs. No need to indirect through shared pointers IMHO.
I can merge the results in GetMemoryRegionInfo().
I put the xml parsing in ProcessGDBRemote because it seemed similar to the
logic in ProcessGDBRemote::GetGDBServerRegisterInfo, which also uses
ReadExtFeature. Figured there must have been a reason for that. Easy to move
it.
And shared pointers were only because I was mimicking the
Process::GetMemoryRegions api.
Comment at: source/Symbol/ObjectFile.cpp:671
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
clayborg wrote:
> labath wrote:
> > You're not actually changing this part, but it caught my eye, and you may
> > care about this:
> >
> > Is it true that we can ignore .bss here? Programs generally assume that
> > .bss is zero-initialized, and if you just ignore it here, it can contain
> > whatever garbage was in the memory before you loaded (?)
> Very important to zero out bss
For the embedded projects I've worked on, ignoring .bss here is fine and
expected because the code always starts by zeroing out the .bss memory (and
also copying the .data section contents from ROM to RAM, which is related to
the physical address change).
For comparison, gdb doesn't try to load the bss section either.
https://reviews.llvm.org/D42145
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Very nice overall. See inlined comments. Big issues are:
- GDBRemoteCommunication::WriteEscapedBinary() is not needed as
StreamGDBRemote::PutEscapedBytes() does this already
- Remove the batch start and end functions in process if possible and have
ProcessGDBRemote::DoWriteMemory() just "do the right thing"
- Can we actually cache the results of the qXfer:memory-map for the entire
process lifetime?
- Remove the new ProcessGDBRemote::GetMemoryMapRegion() and fold into
GDBRemoteCommunicationClient::GetMemoryRegionInfo() as needed
Comment at: include/lldb/Target/Process.h:1916
+ //--
+ virtual bool BeginWriteMemoryBatch() { return true; }
+
labath wrote:
> Instead of this begin/end combo, what would you think about a WriteMemory
> overload that takes a list of memory regions?
> Something like:
> ```
> struct WriteEntry { addr_t Dest; llvm::ArrayRef Contents; };
> Status WriteMemory(llvm::ArrayRef);
> ```
> Then the default implementation can just lower this into regular
> `WriteMemory` sequence, and the gdb-remote class can add the extra packets
> around it when needed.
Do we really need this? Can't we just take care of it inside of the standard
Process::WriteMemory()? This doesn't seem like something that a user should
have to worry about. The process plug-in should just make the write happen
IMHO. Let me know.
Comment at: include/lldb/Target/Process.h:1931
+ //--
+ virtual bool EndWriteMemoryBatch() { return true; }
+
Ditto
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:277-293
+void GDBRemoteCommunication::WriteEscapedBinary(StreamString &stream,
+const void *buf,
+size_t size) {
+ const uint8_t *bytes = (const uint8_t *)buf;
+ const uint8_t *end = bytes + size;
+ const uint8_t escape = 0x7d;
+ for (; bytes != end; ++bytes) {
Remove and use StreamGDBRemote::PutEscapedBytes(). Any special encodings for
packets should be added to StreamGDBRemote.
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h:152-153
+ //--
+ static void WriteEscapedBinary(StreamString &stream, const void *buf,
+ size_t size);
+
StreamGDBRemote::PutEscapedBytes(...) does exactly this. If you are encoding a
packet, use StreamGDBRemote if you are not already, and then use
PutEscapedBytes(...).
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2803-2810
+MemoryRegionInfoSP ProcessGDBRemote::GetMemoryMapRegion(lldb::addr_t addr) {
+ if (m_memory_map.empty())
+GetMemoryMap(m_memory_map);
+ for (const auto ®ion : m_memory_map)
+if (region->GetRange().Contains(addr))
+ return region;
+ return nullptr;
Note we already have:
```
Status ProcessGDBRemote::GetMemoryRegionInfo(addr_t load_addr, MemoryRegionInfo
®ion_info);
```
Remove this function and merge your logic into
GDBRemoteCommunicationClient::GetMemoryRegionInfo(...).
Also: can you cache the results of the memory map for the entire process
lifetime? GDBRemoteCommunicationClient::GetMemoryRegionInfo() queries each time
since it can detect new regions that were just allocated at any time. I believe
the linux version will check the process memory map in "/proc/$pid/maps" so it
is always up to date. Maybe these results can be cached, but we need to ensure
we catch changes in memory layout and mapping.
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2812-2815
+bool ProcessGDBRemote::BeginWriteMemoryBatch() {
+ m_is_batched_memory_write = true;
+ return true;
+}
clayborg wrote:
> Remove and make ProcessGDBRemote::DoWriteMemory() just handle things.
Remove and make ProcessGDBRemote::DoWriteMemory() just handle things.
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2812-2821
+bool ProcessGDBRemote::BeginWriteMemoryBatch() {
+ m_is_batched_memory_write = true;
+ return true;
+}
+
+bool ProcessGDBRemote::EndWriteMemoryBatch() {
+ m_is_batched_memory_write = false;
Remove and make ProcessGDBRemote::DoWriteMemory() just handle things.
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2962
+packet.Printf("vFlashWrite:%" PRIx64 ":", addr);
+GDBRemoteCommunication::WriteEscapedBinary(packet, buf, size);
+ } else {
labath wrote:
> This function is unnecessary. You should use StreamGDBRemote::PutEscapedBytes.
In
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
labath added a comment.
First, I'd like to thank you for taking the time to create a method for testing
patches like these. I think this will be very valuable for all out-of-tree stub
support patches we will get in the future. Could I ask that you split out the
generic test-suite-stuff part of this into a separate patch. This will make it
easier to review and make sure it runs on the various buildbots that we have.
My first batch of comments is below, but I'll need to take a closer look at
this one more time.
Comment at: include/lldb/Target/Process.h:1916
+ //--
+ virtual bool BeginWriteMemoryBatch() { return true; }
+
Instead of this begin/end combo, what would you think about a WriteMemory
overload that takes a list of memory regions?
Something like:
```
struct WriteEntry { addr_t Dest; llvm::ArrayRef Contents; };
Status WriteMemory(llvm::ArrayRef);
```
Then the default implementation can just lower this into regular `WriteMemory`
sequence, and the gdb-remote class can add the extra packets around it when
needed.
Comment at:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py:43
+
+MEMORY_MAP = """
+
We will need a way to skip this test when lldb is compiled without libxml
support. This will be a bit tricky, as right now we don't have a mechanism for
passing build-configuration from cmake into dotest. I guess we will need some
equivalent of `lit.site.cfg.in`.
The thing that makes this tricky is that this will need to work from xcode as
well. As a transitional measure we could probably assume "true" if we don't
have that file, as I think the Xcode project is always configured to use libxml.
Comment at:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:13
+# Tries to find yaml2obj at the same folder as the lldb
+path = os.path.join(os.path.dirname(lldbtest_config.lldbExec), "yaml2obj")
+if os.path.exists(path):
We should also add yaml2obj as a dependency of the check-lldb target in cmake.
Comment at:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:301
+
+mydir = TestBase.compute_mydir(__file__)
+server = None
You can set `NO_DEBUG_INFO_TESTCASE = True` here, and avoid the decorators on
every test case.
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2962
+packet.Printf("vFlashWrite:%" PRIx64 ":", addr);
+GDBRemoteCommunication::WriteEscapedBinary(packet, buf, size);
+ } else {
This function is unnecessary. You should use StreamGDBRemote::PutEscapedBytes.
Comment at: source/Symbol/ObjectFile.cpp:671
+ continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
You're not actually changing this part, but it caught my eye, and you may care
about this:
Is it true that we can ignore .bss here? Programs generally assume that .bss is
zero-initialized, and if you just ignore it here, it can contain whatever
garbage was in the memory before you loaded (?)
https://reviews.llvm.org/D42145
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
owenpshaw created this revision.
owenpshaw added reviewers: clayborg, labath.
Herald added subscribers: mgorny, emaste.
When writing an object file over gdb-remote, use the vFlashErase, vFlashWrite,
and vFlashDone commands if the write address is in a flash memory region. A
bare metal target may have this kind of setup.
- Update ObjectFileELF to set load addresses using physical addresses. A
typical case may be a data section with a physical address in ROM and a virtual
address in RAM, which should be loaded to the ROM address.
- Add support for querying the target's qXfer:memory-map, which contains
information about flash memory regions, leveraging MemoryRegionInfo data
structures with minor modifications
- Update ProcessGDBRemote to use vFlash commands in DoWriteMemory when the
target address is in a flash region
- Add a new foundation for testing gdb-remote behaviors by using a mock server
that can respond however the test requires
Original discussion at
http://lists.llvm.org/pipermail/lldb-dev/2018-January/013093.html
---
A few questions...
1. Leveraging MemoryRegionInfo seemed like the way to go, since
qXfer:memory-map results are similar to qMemoryRegionInfo results. But should
GetMemoryRegionInfo() be changed to use the qXfer results instead of having the
separate new function I added?
2. Are the new gdb-remote python tests moving in the right direction? I can
add more cases, but wanted to first verify the foundation was acceptable. It's
similar to some code in the tools/lldb-server tests, but seemed different
enough to justify its own base.
https://reviews.llvm.org/D42145
Files:
include/lldb/Host/XML.h
include/lldb/Target/MemoryRegionInfo.h
include/lldb/Target/Process.h
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/a.yaml
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
source/Host/common/XML.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
source/Symbol/ObjectFile.cpp
unittests/Process/gdb-remote/CMakeLists.txt
unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
Index: unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
===
--- /dev/null
+++ unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
@@ -0,0 +1,54 @@
+//===-- GDBRemoteCommunicationTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include
+
+#include "GDBRemoteTestUtils.h"
+#include "lldb/Utility/StreamString.h"
+
+using namespace lldb_private::process_gdb_remote;
+using namespace lldb_private;
+
+TEST(GDBRemoteCommunicationTest, WriteEscapedBinary) {
+ StreamString escaped;
+
+ // Nothing gets escaped
+ // Verify null and other control chars don't cause problems
+ const uint8_t data[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07};
+ GDBRemoteCommunication::WriteEscapedBinary(escaped, data, sizeof(data));
+ ASSERT_EQ(sizeof(data), escaped.GetSize());
+ ASSERT_EQ(0x00, escaped.GetString().data()[0]);
+ ASSERT_EQ(0x03, escaped.GetString().data()[3]);
+ ASSERT_EQ(0x07, escaped.GetString().data()[7]);
+
+ // 0x23 and 0x24 should be escaped
+ escaped.Clear();
+ const uint8_t data2[] = {0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27};
+ GDBRemoteCommunication::WriteEscapedBinary(escaped, data2, sizeof(data));
+ ASSERT_EQ(sizeof(data) + 2, escaped.GetSize());
+ ASSERT_EQ(0x20, escaped.GetString().data()[0]);
+ ASSERT_EQ(0x7d, escaped.GetString().data()[3]);
+ ASSERT_EQ(0x23 ^ 0x20, escaped.GetString().data()[4]);
+ ASSERT_EQ(0x7d, escaped.GetString().data()[5]);
+ ASSERT_EQ(0x24 ^ 0x20, escaped.GetString().data()[6]);
+ ASSERT_EQ(0x25, escaped.GetString().data()[7]);
+ ASSERT_EQ(0x27, escaped.GetString().data()[9]);
+
+ // 0x7d should be escaped
+ escaped.Clear();
+ const uint8_t data3[] = {0x7b, 0x74, 0x65, 0x73, 0x74, 0x7d};
+ GDBRemoteCommunication::WriteEscapedBinary(escaped, data3, sizeof(data));
+ ASSERT_EQ(sizeof(data) + 1, escaped.GetSize());
+ ASSERT_EQ(0x7b, escaped.GetString().data()[0]);
+ ASSERT_EQ(0x74, escaped.GetString().data()[1]);
+ ASSERT_EQ(0x65, e
