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

[Owen Shaw via Phabricator via lldb-commits]

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