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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Feb 1 02:35:37 PST 2018

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).


More information about the lldb-commits mailing list