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

Yes, it's mostly stylistic differences, but there is one more important thing that we need to figure out, and that's making sure that the test only runs if we actually have XML support compiled-in. If there's isn't anything in the SB API that would tell us that (I cursory look doesn't reveal anything), then we may need to get some help from the build system for that. OTOH, maybe an new SB API for determining the lldb feature set would actually be useful...



================
Comment at: include/lldb/Target/Process.h:1958
 
+  virtual bool WriteObjectFile(llvm::ArrayRef<WriteEntry> entries,
+                               Status &error);
----------------
owenpshaw wrote:
> 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
> ```
That extra line is a bit of a nuissance. We could fix that, but as the long term solution is to use llvm::Error everywhere, i'm not motivated to do that. So if the extra line bothers you, just use llvm::Error. Then the call-size becomes:
```
if (llvm::Error E = WriteObjectFile(...))
  return Status(std::move(E));
```
The constant conversion between Status and Error is a bit annoying, but as more places start using this, it should get better.

(My main issue with your syntax is that it's not DRY)


================
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),
----------------
owenpshaw wrote:
> 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. 
I agree it likely to be cheap, but the thing you fear (2) can be easily avoided.
Note that I deliberately did not add any reference qualifications to the type in the comment above. If you take the argument as a value type, then you leave it up to the caller to decide whether a copy takes place.
If he calls it as `WriteMemoryObject(my_vector)`, then it's copied (and the original is left unmodified).
If he calls it as `WriteMemoryObject(std::move(my_vector))`, then it's moved (but that's very obvious from looking at the call-site).

So in the worst case, you get one copy, just like you would here, and in the best case, you get no copies.
In your case, the caller doesn't need the vector, so we save a copy. To me, that makes this option a clear win.
Isn't C++11 great? :D


================
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
----------------
owenpshaw wrote:
> 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.
> 
Yeah, I think I'll have to agree with you here, although this makes the whole separate-function approach look far less attractive to me.
If we are going to reuse all of that logic that then we might make it obvious that we are doing that by having a bool flag on those functions like Greg suggested a couple of comments back.


https://reviews.llvm.org/D42145