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<uint8_t> Contents; };
> Status WriteMemory(llvm::ArrayRef<WriteEntry>);
> ```
> 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.
Indeed
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4612-4613
+bool ProcessGDBRemote::GetMemoryMap(
+ std::vector<lldb::MemoryRegionInfoSP> ®ion_list) {
+
----------------
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.
================
Comment at: source/Symbol/ObjectFile.cpp:671
+ continue;
+ // We can skip sections like bss
+ if (section_sp->GetFileSize() == 0)
----------------
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
================
Comment at: source/Symbol/ObjectFile.cpp:687-688
+ // 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) {
----------------
Would like Process::BeginWriteMemoryBatch() to go away if possible
================
Comment at: source/Symbol/ObjectFile.cpp:696
+ if (written != section_data.GetByteSize()) {
+ process->EndWriteMemoryBatch();
+ return error;
----------------
Remove EndWriteMemoryBatch if possible.
================
Comment at: source/Symbol/ObjectFile.cpp:700-701
}
+ if (!process->EndWriteMemoryBatch())
+ return Status("Could not end write memory batch");
+
----------------
Would like Process::EndWriteMemoryBatch() to go away if possible
================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp:23
+ const uint8_t data[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07};
+ GDBRemoteCommunication::WriteEscapedBinary(escaped, data, sizeof(data));
+ ASSERT_EQ(sizeof(data), escaped.GetSize());
----------------
StreamGDBRemote::PutEscapedBytes() already does all of this so all of this code
can go away. If StreamGDBRemote::PutEscapedBytes() isn't tested, then switch
this to test StreamGDBRemote::PutEscapedBytes().
https://reviews.llvm.org/D42145
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
