[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 for the feedback, I'm working on splitting out the testing base into another patch and making the requested changes.  My main unresolved question is about the write batching, with details below.



================
Comment at: include/lldb/Target/Process.h:1916
+  //------------------------------------------------------------------
+  virtual bool BeginWriteMemoryBatch() { return true; }
+
----------------
clayborg wrote:
> labath wrote:
> > Instead of this begin/end combo, what would you think about a WriteMemory overload that takes a list of memory regions?
> > Something like:
> > ```
> > struct WriteEntry { addr_t Dest; llvm::ArrayRef<uint8_t> Contents; };
> > Status WriteMemory(llvm::ArrayRef<WriteEntry>);
> > ```
> > Then the default implementation can just lower this into regular `WriteMemory` sequence, and the gdb-remote class can add the extra packets around it when needed.
> Do we really need this? Can't we just take care of it inside of the standard Process::WriteMemory()? This doesn't seem like something that a user should have to worry about. The process plug-in should just make the write happen IMHO. Let me know.
Maybe I'm misunderstanding the flash commands, but I couldn't figure a way around the need to somehow indicate that several writes are batched together.  The reason has to do with how vFlashErase must erase an entire block at a time.

ObjectFile::LoadInMemory makes one Process::WriteMemory call per section.  If each WriteMemory call was self-contained, and two sections are in the same flash block, it would go something like this:

# WriteMemory (section 1)
## DoWriteMemory
### vFlashErase (block 1)
### vFlashWrite (section 1)
### vFlashDone
# WriteMemory (section 2)
## DoWriteMemory
### vFlashErase (block 1, again)
### vFlashWrite (section 2)
### vFlashDone

Wouldn't the second erase undo the first write?

I found the begin/end to be the least intrusive way around, but I'm open to other options.


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4612-4613
 
+bool ProcessGDBRemote::GetMemoryMap(
+    std::vector<lldb::MemoryRegionInfoSP> &region_list) {
+
----------------
clayborg wrote:
> Can we cache this value permanently? If so we should read it once and then the GDBRemoteCommunicationClient::GetMemoryRegionInfo() should try the qMemoryRegionInfo packet (if supported) and possibly augment the memory region results with this info?
> 
> This should be pushed down into GDBRemoteCommunicationClient and it should keep a member variable that caches these results.
> 
> Any reason we are using shared pointers to lldb::MemoryRegionInfo? They are simple structs. No need to indirect through shared pointers IMHO.
I can merge the results in GetMemoryRegionInfo().

I put the xml parsing in ProcessGDBRemote because it seemed similar to the logic in ProcessGDBRemote::GetGDBServerRegisterInfo, which also uses ReadExtFeature.  Figured there must have been a reason for that.  Easy to move it.

And shared pointers were only because I was mimicking the Process::GetMemoryRegions api.


================
Comment at: source/Symbol/ObjectFile.cpp:671
+      continue;
+    // We can skip sections like bss
+    if (section_sp->GetFileSize() == 0)
----------------
clayborg wrote:
> labath wrote:
> > You're not actually changing this part, but it caught my eye, and you may care about this:
> > 
> > Is it true that we can ignore .bss here? Programs generally assume that .bss is zero-initialized, and if you just ignore it here, it can contain whatever garbage was in the memory before you loaded (?)
> Very important to zero out bss
For the embedded projects I've worked on, ignoring .bss here is fine and expected because the code always starts by zeroing out the .bss memory (and also copying the .data section contents from ROM to RAM, which is related to the physical address change).

For comparison, gdb doesn't try to load the bss section either.


https://reviews.llvm.org/D42145