[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions
Owen Shaw via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Thu Feb 1 10:35:56 PST 2018
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);
> 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))
error = WriteObjectFile(entries);
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)`).
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();
+ // 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).
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.
More information about the lldb-commits