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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: include/lldb/Target/Process.h:1916
+  //------------------------------------------------------------------
+  virtual bool BeginWriteMemoryBatch() { return true; }
+
----------------
owenpshaw wrote:
> 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.
Seems like any block write that isn't writing an entire block should read the contents that won't be overwritten, create a block's worth of data to write by using the pre-existing data and adding any new data, then erase the block and and rewrite the entire block. Then in ObjectFile::Load() it would batch up any consecutive writes to minimize any block erasing...


================
Comment at: source/Symbol/ObjectFile.cpp:671
+      continue;
+    // We can skip sections like bss
+    if (section_sp->GetFileSize() == 0)
----------------
owenpshaw wrote:
> 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.
Sounds good, if gdb doesn't do it, then I am fine if we don't do it


https://reviews.llvm.org/D42145