[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Greg Clayton via Phabricator via lldb-commits]

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 &region : m_memory_map)
+    if (region->GetRange().Contains(addr))
+      return region;
+  return nullptr;
----------------
Note we already have: 

```
Status ProcessGDBRemote::GetMemoryRegionInfo(addr_t load_addr, MemoryRegionInfo &region_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> &region_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 &section_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