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

[Pavel Labath via Phabricator via lldb-commits]

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<WriteEntry> 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<WriteEntry> 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<WriteEntry>` (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