[lldb] Use vFlash commands when writing to target's flash memory regions

[owenpshaw]

Summary

When writing an object file over gdb-remote, use the vFlashErase, vFlashWrite, and vFlashDone commands if the write address is in a flash memory region. A bare metal target may have this kind of setup.

• Update ObjectFileELF to set load addresses using physical addresses. A typical case may be a data section with a physical address in ROM and a virtual address in RAM, which should be loaded to the ROM address.
• Add support for querying the target's qXfer:memory-map, which contains information about flash memory regions, leveraging MemoryRegionInfo data structures with minor modifications
• Update ProcessGDBRemote to use vFlash commands in DoWriteMemory when the target address is in a flash region
• Add a new foundation for testing gdb-remote behaviors by using a mock server that can respond however the test requires

Original discussion at http://lists.llvm.org/pipermail/lldb-dev/2018-January/013093.html

---

A few questions...

1. Leveraging MemoryRegionInfo seemed like the way to go, since qXfer:memory-map results are similar to qMemoryRegionInfo results. But should GetMemoryRegionInfo() be changed to use the qXfer results instead of having the separate new function I added?
2. Are the new gdb-remote python tests moving in the right direction? I can add more cases, but wanted to first verify the foundation was acceptable. It's similar to some code in the tools/lldb-server tests, but seemed different enough to justify its own base.

[labath]

First, I'd like to thank you for taking the time to create a method for testing patches like these. I think this will be very valuable for all out-of-tree stub support patches we will get in the future. Could I ask that you split out the generic test-suite-stuff part of this into a separate patch. This will make it easier to review and make sure it runs on the various buildbots that we have.

My first batch of comments is below, but I'll need to take a closer look at this one more time.

include/lldb/Target/Process.h

1916 ↗

(On Diff #130054)

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.

packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py

43 ↗

(On Diff #130054)

We will need a way to skip this test when lldb is compiled without libxml support. This will be a bit tricky, as right now we don't have a mechanism for passing build-configuration from cmake into dotest. I guess we will need some equivalent of lit.site.cfg.in.

The thing that makes this tricky is that this will need to work from xcode as well. As a transitional measure we could probably assume "true" if we don't have that file, as I think the Xcode project is always configured to use libxml.

packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py

13 ↗

(On Diff #130054)

We should also add yaml2obj as a dependency of the check-lldb target in cmake.

301 ↗

(On Diff #130054)

You can set NO_DEBUG_INFO_TESTCASE = True here, and avoid the decorators on every test case.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

2962 ↗

(On Diff #130054)

This function is unnecessary. You should use StreamGDBRemote::PutEscapedBytes.

source/Symbol/ObjectFile.cpp

671 ↗

(On Diff #130054)

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 (?)

[clayborg]

Very nice overall. See inlined comments. Big issues are:

• GDBRemoteCommunication::WriteEscapedBinary() is not needed as StreamGDBRemote::PutEscapedBytes() does this already
• Remove the batch start and end functions in process if possible and have ProcessGDBRemote::DoWriteMemory() just "do the right thing"
• Can we actually cache the results of the qXfer:memory-map for the entire process lifetime?
• Remove the new ProcessGDBRemote::GetMemoryMapRegion() and fold into GDBRemoteCommunicationClient::GetMemoryRegionInfo() as needed

include/lldb/Target/Process.h

1916 ↗

(On Diff #130054)

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.

1931 ↗

(On Diff #130054)

Ditto

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

277–293 ↗

(On Diff #130054)

Remove and use StreamGDBRemote::PutEscapedBytes(). Any special encodings for packets should be added to StreamGDBRemote.

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h

152–153 ↗

(On Diff #130054)

StreamGDBRemote::PutEscapedBytes(...) does exactly this. If you are encoding a packet, use StreamGDBRemote if you are not already, and then use PutEscapedBytes(...).

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

2803–2810 ↗

(On Diff #130054)

Note we already have:

Status ProcessGDBRemote::GetMemoryRegionInfo(addr_t load_addr, MemoryRegionInfo &region_info);

Remove this function and merge your logic into GDBRemoteCommunicationClient::GetMemoryRegionInfo(...).

Also: can you cache the results of the memory map for the entire process lifetime? GDBRemoteCommunicationClient::GetMemoryRegionInfo() queries each time since it can detect new regions that were just allocated at any time. I believe the linux version will check the process memory map in "/proc/$pid/maps" so it is always up to date. Maybe these results can be cached, but we need to ensure we catch changes in memory layout and mapping.

2812–2815 ↗

(On Diff #130054)

Remove and make ProcessGDBRemote::DoWriteMemory() just handle things.

2812–2821 ↗

(On Diff #130054)

Remove and make ProcessGDBRemote::DoWriteMemory() just handle things.

2962 ↗

(On Diff #130054)

Indeed

4612–4613 ↗

(On Diff #130054)

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.

source/Symbol/ObjectFile.cpp

671 ↗

(On Diff #130054)

Very important to zero out bss

687–688 ↗

(On Diff #130054)

Would like Process::BeginWriteMemoryBatch() to go away if possible

696 ↗

(On Diff #130054)

Remove EndWriteMemoryBatch if possible.

700–701 ↗

(On Diff #130054)

Would like Process::EndWriteMemoryBatch() to go away if possible

unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp

23 ↗

(On Diff #130054)

StreamGDBRemote::PutEscapedBytes() already does all of this so all of this code can go away. If StreamGDBRemote::PutEscapedBytes() isn't tested, then switch this to test StreamGDBRemote::PutEscapedBytes().

[owenpshaw]

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.

include/lldb/Target/Process.h

1916 ↗

(On Diff #130054)

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:

1. WriteMemory (section 1)
   1. DoWriteMemory
      1. vFlashErase (block 1)
      2. vFlashWrite (section 1)
      3. vFlashDone
2. WriteMemory (section 2)
   1. DoWriteMemory
      1. vFlashErase (block 1, again)
      2. vFlashWrite (section 2)
      3. 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.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4612–4613 ↗

(On Diff #130054)

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.

source/Symbol/ObjectFile.cpp

671 ↗

(On Diff #130054)

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.

[clayborg]

include/lldb/Target/Process.h

1916 ↗

(On Diff #130054)

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

source/Symbol/ObjectFile.cpp

671 ↗

(On Diff #130054)

Sounds good, if gdb doesn't do it, then I am fine if we don't do it

[owenpshaw]

• Split testing base into separate review https://reviews.llvm.org/D42195
• Use StreamGDBRemote instead of duplicate new function
• Move qXfer:memory-map xml parsing into GDBRemoteCommunicationClient
• Include qXfer:memory-map data in GetMemoryRegionInfo, instead of creating a separate API

I've left the batch write as-is for now because I'm not quite sold on the solution of writing entire blocks of flash memory at time. While I agree it could work, it feels like an overly complicated code solution that will also generate a lot of unnecessary commands between the client and server.

Can you help me understand the objection to the begin/end design? I can't really think of scenario besides object file load where the begin/end batching calls would be used. So perhaps instead of ObjectFile::LoadInMemory using Process::WriteMemory, it instead calls something like a new Process::WriteObjectSections(std::vector<SectionSP>, SectionLoadList) method that's clearly only intended for object writing. The GDB process could override and do the begin/end batching privately.

[clayborg]

My objection to the BeginWriteMemoryBatch and EndWriteMemoryBatch is users must know to emit these calls when they really just want to call process.WriteMemory() and not worry about how it is done. Much like writing to read only code when setting breakpoints, we don't require the user to know that they must check the memory permissions first, set permissions to be writable and then revert them back after writing to a memory location.

If we have the ability to do things correctly, then we should and we shouldn't require people to know about calls that are specific to flash. Just make it happen if you can, else return an error. So I would rather see the ObjectFile::Load become more process aware where it gets the memory region info and efficiently loads memory as needed by watching the m_blocksize than require everyone else that might want to write memory to know to start a batch and end a batch. It comes down to keeping the API simple.

include/lldb/Target/MemoryRegionInfo.h

109–110 ↗

(On Diff #130258)

Could these two be combined into one entry? Could "m_blocksize" be "m_flash_block_size" and if the value is zero, then it isn't flash?

[owenpshaw]

I'm not envisioning that anything else needs to change to use begin/end or care it's there. I guess the way I look at it, having ObjectFile::LoadInMemory do begin/end is basically the same as what you're saying about having it be more process aware.

If Process is going to introduce new concepts for ObjectFile to use either way, isn't a high level of abstraction (batched writes) preferable to ObjectFile needing to know the fine details of flash memory blocks or that flash is even used? And doesn't keeping the heavy lifting in ProcessGDB make it reusable should another case come along?

Hope you don't mind the pushback. I think we're looking at this from different angles, and I'm genuinely asking to help my understanding of your thinking so hopefully we can converge on the same view.

include/lldb/Target/MemoryRegionInfo.h

109–110 ↗

(On Diff #130258)

Sure, I can change that. Almost went that way initially, but thought the two fields made it a little clearer, and allowed for an error condition when flash is true, but the blocksize is invalid (0). Collapsing to one field means falling back to a regular memory write rather than an error. Not sure if it's a big deal either way.

[owenpshaw]

Hi Greg, I got distracted from this one for a bit. Maybe I missed it, but do you have any thoughts on my previous comment/question about the batch API vs other options?

Thanks,
Owen

[clayborg]

In D42145#979629, @owenpshaw wrote:

I'm not envisioning that anything else needs to change to use begin/end or care it's there. I guess the way I look at it, having ObjectFile::LoadInMemory do begin/end is basically the same as what you're saying about having it be more process aware.

If Process is going to introduce new concepts for ObjectFile to use either way, isn't a high level of abstraction (batched writes) preferable to ObjectFile needing to know the fine details of flash memory blocks or that flash is even used? And doesn't keeping the heavy lifting in ProcessGDB make it reusable should another case come along?

Hope you don't mind the pushback. I think we're looking at this from different angles, and I'm genuinely asking to help my understanding of your thinking so hopefully we can converge on the same view.

I really would like users to not have to know how things must happen and that they must use a batch start and batch end. I strongly believe we should have Process::WriteMemory just do the right thing. We can make ObjectFile::Load() be smart and make sure to batch all consecutive writes to avoid unnecessary erases. I just really want memory write to just work. No questions.

[owenpshaw]

Thanks. What I'm struggling to reconcile are your statements that users should not have to know how things must happen, but then that we should make ObjectFile::Load smart so it doesn't result in an unnecessary amount of reads/writes. If writing to flash memory effectively requires some smarts on the caller's part, then how have we made things easier for the callers? And at that point isn't start/end a lot simpler than requiring callers to worry about block sizes themselves?

[labath]

I still like my WriteMemory(ArrayRef<MemoryWriteDescriptor>) proposal the best. If you don't have any special writing requirements, it can be viewed as a convenience wrapper for the one-shot WriteMemory, and if a subclass needs special handling, it can implement it in a smarter way (while making sure that the one-shot call still "does the right thing", just in a slower way).

It also makes it impossible to forget to call EndBatch().

[owenpshaw]

This discussion has got me questioning if WriteMemory is even the best place to put the flash commands. It seems like some of the concern here is around the behavior of arbitrary memory writes to flash regions, which isn't really the main objective of this patch (supporting object file loading into flash).

I did a little testing with gdb, and it doesn't do arbitrary memory writes to flash regions. Using the set command on a flash address, for example, just gives an error that flash writes are not allowed. Perhaps that's a better way to go than worrying about supporting one-off flash writes.

---

I still like my WriteMemory(ArrayRef<MemoryWriteDescriptor>) proposal the best.

The one difference I see between a single function and begin/end is that begin/end doesn't require all the bytes to be read into host memory up front, and can instead read the source bytes in chunks. Perhaps that's not really a reason for concern, though.

[clayborg]

In D42145#991459, @owenpshaw wrote:

Thanks. What I'm struggling to reconcile are your statements that users should not have to know how things must happen, but then that we should make ObjectFile::Load smart so it doesn't result in an unnecessary amount of reads/writes. If writing to flash memory effectively requires some smarts on the caller's part, then how have we made things easier for the callers?

We don't have to, we can let them submit each memory write and do things inefficiently. I am just saying it isn't too hard in ObjectFile::Load to gather large contiguous writes and submit them as one Process::WriteMemory. Pavel's suggestion that we send an array of regions would allow us to do this internally in the Process::WriteMemory(regions) call.

And at that point isn't start/end a lot simpler than requiring callers to worry about block sizes themselves?

There are no block size requirements here, WriteMemory can either do the write memory or error out. Just submit the largest contiguous write possible. Not sure what the issue is here?

[clayborg]

In D42145#991985, @owenpshaw wrote:

This discussion has got me questioning if WriteMemory is even the best place to put the flash commands. It seems like some of the concern here is around the behavior of arbitrary memory writes to flash regions, which isn't really the main objective of this patch (supporting object file loading into flash).

I am ok with having an extra interface on Process if needed. The ObjectFile::Load would use this new API to load things. But most targets aren't going to have flash and it would be weird if a target with a ton of RAM and no flash had to worry about that. Then ObjectFile::Load would need to be able to get memory region info, detect where flash is and where it isn't, and submit either a WriteMemory to WriteFlash command. I am ok with that if this makes things cleaner.

I did a little testing with gdb, and it doesn't do arbitrary memory writes to flash regions. Using the set command on a flash address, for example, just gives an error that flash writes are not allowed. Perhaps that's a better way to go than worrying about supporting one-off flash writes.

That would be fine.

---

I still like my WriteMemory(ArrayRef<MemoryWriteDescriptor>) proposal the best.

The one difference I see between a single function and begin/end is that begin/end doesn't require all the bytes to be read into host memory up front, and can instead read the source bytes in chunks. Perhaps that's not really a reason for concern, though.

RAM is cheap these days and if the file is large, then mmap it, No RAM required. I wouldn't worry about that for now.

So the main question is: do we want WriteMemory to work anywhere and always try to do the right thing, or return an error an the user would be expected to know to check the memory region you are writing to and know to call "Process::WriteFlash(...)". I vote to keep things simple and have Process::WriteMemory() just do the right thing. I am open to differing opinions, so let me know what you guys think.

[owenpshaw]

So the main question is: do we want WriteMemory to work anywhere and always try to do the right thing, or return an error an the user would be expected to know to check the memory region you are writing to and know to call "Process::WriteFlash(...)". I vote to keep things simple and have Process::WriteMemory() just do the right thing. I am open to differing opinions, so let me know what you guys think.

What are other use cases for writing to flash memory from lldb? Since I can't think of any, I'm biased towards keeping this implementation simple and narrowly focused on object loading, which means not worrying about preserving the unwritten parts of erased blocks, and giving an error for other write attempts to flash regions.

[labath]

In D42145#992289, @owenpshaw wrote:

So the main question is: do we want WriteMemory to work anywhere and always try to do the right thing, or return an error an the user would be expected to know to check the memory region you are writing to and know to call "Process::WriteFlash(...)". I vote to keep things simple and have Process::WriteMemory() just do the right thing. I am open to differing opinions, so let me know what you guys think.

What are other use cases for writing to flash memory from lldb? Since I can't think of any, I'm biased towards keeping this implementation simple and narrowly focused on object loading, which means not worrying about preserving the unwritten parts of erased blocks, and giving an error for other write attempts to flash regions.

How does the flash memory behave from the POV of the debugged process? E.g. if it does something like:

char *Flash = /*pointer to flash memory*/
*Flash = 47;

will that succeed, or will it get some exception? I'm guessing it's the latter.. If that's the case, then I'm also leaning towards *not* making WriteMemory "just work", as we cannot make the existence of flash memory transparent without e.g. fixing how breakpoint setting in the flashed region works.

However, I'm not sure it should be the responsibility of the ObjectFile to know about the various memory kinds. Maybe we could still keep that in the process class. We could have the ObjectFile create a structure describing the desired memory layout (just like in the batch-WriteMemory version), and then pass it to some Process function, which would handle the flash details. (So in essence, the implementation would remain the same, you just wouldn't call the function WriteMemory, but something else).

[owenpshaw]

How does the flash memory behave from the POV of the debugged process?

Just tested on a couple targets and they both silently failed to change the flash memory. So no exception, but no flash write either.

Unless there are objections, I'll work on some changes along the lines you described.

[owenpshaw]

• Remove Begin/End memory batch API
• Add Process::WriteObjectFile(llvm::ArrayRef<WriteEntry>) method. Open to other names since it's not necessarily specific to object files, but wanted to somehow indicate that the method may do an uncommon write (like write to flash).
• Kept vFlashWrite as part of DoWriteMemory, and changed the is_batch_write flag to an allow_flash_writes flag. ProcessGDB:: WriteObjectFile sets allow_flash_writes, and then calls the regular WriteMemory. Didn't seem like there was a need to duplicate the WriteMemory logic.
• Any other memory write attempts to flash regions will now fail

I think this gets us closer to what we're talking about, but let me know if anything should change.

[labath]

I think we're slowly getting there, but we could cleanup the implementation a bit.

I am also not sure that the WriteObjectFile name really captures what this function does, but I don't have a suggestion for a better name either....

include/lldb/Target/Process.h

559 ↗

(On Diff #132260)

Please run clang format over your diff.

1958 ↗

(On Diff #132260)

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

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

2807 ↗

(On Diff #132260)

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

2812–2821 ↗

(On Diff #132260)

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

[clayborg]

In D42145#994556, @labath wrote:

I think we're slowly getting there, but we could cleanup the implementation a bit.

I am also not sure that the WriteObjectFile name really captures what this function does, but I don't have a suggestion for a better name either....

Looking at the API I wouldn't immediately assume that WriteObjectFile would be flash aware and WriteMemory wouldn't... How about we add a new parameter to Process::WriteMemory that like "bool write_flash". It can default to false. Then if this is true, then we do the extra legwork to try and figure out the memory map. If false, just write the memory.

[owenpshaw]

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.

include/lldb/Target/Process.h

1958 ↗

(On Diff #132260)

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

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

2807 ↗

(On Diff #132260)

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.

2812–2821 ↗

(On Diff #132260)

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.

[labath]

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

include/lldb/Target/Process.h

1958 ↗

(On Diff #132260)

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)

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

2807 ↗

(On Diff #132260)

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

2812–2821 ↗

(On Diff #132260)

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.

[owenpshaw]

• adjust WriteObjectFile signature to return Status and take an std::vector
• match project formatting style

I toyed with adding allow_flash to as an argument, but didn't really like it because it seemed to bring things back to basically allowing arbitrary flash writes via WriteMemory, and would affect all the process subclasses when only one (gdb) actually needs it. I still like the member flag because it keeps the flash writing very private to ProcessGDB.

What can I do to get the xml dependency check in the test? Not clear on who I should talk to or where to start.

[labath]

In D42145#996575, @owenpshaw wrote:

• adjust WriteObjectFile signature to return Status and take an std::vector
• match project formatting style

I toyed with adding allow_flash to as an argument, but didn't really like it because it seemed to bring things back to basically allowing arbitrary flash writes via WriteMemory, and would affect all the process subclasses when only one (gdb) actually needs it. I still like the member flag because it keeps the flash writing very private to ProcessGDB.

If we called the argument writing_object_file instead of allow_flash, then maybe we wouldn't be exposing flash details, although i'm not sure if it makes much of a difference. I guess we can keep the member flag for now...

What can I do to get the xml dependency check in the test? Not clear on who I should talk to or where to start.

Solution A would be to add some cmake code like

configure_file(dotest_build_config.py.in dotest_build_config.py)

which would generate the config file with the xml flag substituted. Then, somewhere from the test you could load this file and check the value.

Solution B would be to add a function (static function on SBDebugger class maybe) which you could call from the test to query the properties the binary was built with.

I can start a thread on lldb-dev to see what people prefer. If noone cares enough to respond, then I guess you can pick whichever option you want.

[owenpshaw]

Update patch to include new @skipIfXmlSupportMissing decorator on test

[clayborg]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

3478 ↗

(On Diff #135087)

Can't p_paddr be zero and still be valid? Would a better test be "header->p_memsz > 0"?

[owenpshaw]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

3478 ↗

(On Diff #135087)

Yes, a 0 paddr can be valid. This check will only ignore paddr if all segments have a 0 paddr, suggesting the linker didn't populate that field.

This decision is similar to what I did in llvm-objcopy for binary output, which in turn was based on GNU objcopy behavior: https://bugs.llvm.org/show_bug.cgi?id=35708#c4

[owenpshaw]

@labath, any other changes you'd like to see on this one? Skipping the test if there's no xml support seemed to be the final todo.

[labath]

Sorry, I forgot about that. This looks fine as far as I am concerned. Thank you for flying lldb, and in particular, for creating the gdb-client testing framework.

[owenpshaw]

Thanks! Just a reminder that I don't have commit access, so someone will need to land this for me. Appreciate all the help.

[clayborg]

I agree with Pavel, thanks for taking the time to get this done right and listening to the feedback that was offered. The patch is better for it.

[labath]

It seems this messed up the computation of load addresses for symbols in the androids vdso:

Symtab, num_symbols = 5:
               Debug symbol
               |Synthetic symbol
               ||Externally Visible
               |||
Index   UserID DSX Type            File Address/Value Load Address       Size               Flags      Name
------- ------ --- --------------- ------------------ ------------------ ------------------ ---------- ----------------------------------
[    0]      1   X Code            0x00000000ffffe410 0x00000410 0x0000000000000008 0x00000012 __kernel_rt_sigreturn
[    1]      2   X Code            0x00000000ffffe400 0x00000400 0x0000000000000009 0x00000012 __kernel_sigreturn
[    2]      3   X Code            0x00000000ffffe420 0x00000420 0x0000000000000014 0x00000012 __kernel_vsyscall
[    3]      4   X Data            0x0000000000000000                    0x0000000000000000 0x00000011 LINUX_2.5
[    4]      4  SX Code            0x00000000ffffe40f 0x0000040f 0x0000000000000001 0x00000000 ___lldb_unnamed_symbol1$$[vdso]

(the load address is obviously wrong, and maybe file address as well, although I'm not sure what did we put as a file address for files that are loaded from memory)

I'm going to try to investigate more, but I'm writing here in case you have any idea what could be wrong.

[labath]

Ok, so this is what seems to be happening:
The vdso "file" has the following program headers:

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000000 0xffffe000 0x00000000 0x00434 0x00434 R E 0x1000
  DYNAMIC        0x000324 0xffffe324 0x00000324 0x00090 0x00090 R   0x4
  NOTE           0x0001c4 0xffffe1c4 0x000001c4 0x00034 0x00034 R   0x4
  GNU_EH_FRAME   0x0001f8 0xffffe1f8 0x000001f8 0x00024 0x00024 R   0x4

Your patch makes the makes us use the physical addresses (because some of the physical addresses are non-null), but in this case that is incorrect. For this file the VirtAddr describes the correct address of the file/section/segment in memory. Also the addresses in the section headers (which is what we were using previously are correct.

Going back to your patch, it's not clear to me whether using physical addresses as load addresses is a correct thing to do in general. I mean, generally, we care about the places where the object file is located in virtual memory, and in physical, so maybe we should keep use virtual addresses here, and have ObjectFile::LoadInMemory use physical addresses through some other api? What do you think?

That said, on my desktop machine I get the following program headers from the vdso:

Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
LOAD           0x000000 0x00000000 0x00000000 0x00d1a 0x00d1a R E 0x1000
DYNAMIC        0x0002bc 0x000002bc 0x000002bc 0x00090 0x00090 R   0x4
NOTE           0x000560 0x00000560 0x00000560 0x00060 0x00060 R   0x4
GNU_EH_FRAME   0x0005c0 0x000005c0 0x000005c0 0x00024 0x00024 R   0x4

and things still work fine, so it may be possible to make this work by just changing how we compute the load bias (I have to look up where we do that).
(However, I am still not sure about using physical addresses being the right thing here).

[labath]

lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

814

Ok so the issue is that here we use the virtual address to compute the load bias, but at line 830 we add the bias to the physical address. This breaks as soon as these two addresses start to differ.

Changing this to use the physical address as well fixes things, but as I said before, I'm not sure we should be using physical addresses here.

[owenpshaw]

lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

814

I don't know if there's another use case besides flash load that should definitely use the physical address, so I can't be much help answering that question. I was mainly relying on tests to catch any issue caused by using physical addresses.

Could the value_is_offset flag be a tell here? Looks like that load bias is only calculated if value_is_offset == false. Would it make sense to always use virtual address in such a case? It wouldn't affect the "target modules load" case, which always sets value_is_offset to true.

[labath]

lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

814

I don't know if there's another use case besides flash load that should definitely use the physical address, so I can't be much help answering that question. I was mainly relying on tests to catch any issue caused by using physical addresses.

Yeah, unfortunately we don't have tests which would catch borderline conditions like that, and as you've said, most elf files have these the same, so you couldn't have noticed that. The only reason I this is that one of the android devices we test on has a kernel whose vdso has these different.

The good news is that with your test suite, we should be able to write a test for it, when we figure out what the behavior should be here.

Could the value_is_offset flag be a tell here? Looks like that load bias is only calculated if value_is_offset == false. Would it make sense to always use virtual address in such a case? It wouldn't affect the "target modules load" case, which always sets value_is_offset to true.

The is_offset flag is orthogonal to this. It tells you whether the value represents a bias or an absolute value. When we read the module list from the linker rendezvous structure, we get a load bias; when we read it from /proc/pid/maps, we get an absolute value. It does *not* tell you what the value is relative to. So while doing that like you suggest would fix things, I don't think it's the right thing to do.

In fact, the more I think about this, the more I'm convinced using physical addresses is not the right solution here. The main user of this function is the dynamic linker plugin, which uses it to notify the debugger about modules that have already been loaded into the process. That definitely sounds like a virtual address thing. Also, if you look at the documentation of the "target modules load --slide", it explicitly states that the slide should be relative to the virtual address.

And I'm not even completely clear about your case. I understand you write the module to the physical address, but what happens when you actually go around to debugging it? Is it still there or does it get copied/mapped/whatever to the virtual address?

So it seems to me the physical addresses should really be specific to the "target modules load --load" case. I've never used that command so I don't know if it can just use physical addresses unconditionally, or whether we need an extra option for that "target modules load --load --physical". I think I'll leave that decision up to you (or anyone else who has an opinion on this). But for the other use cases, I believe we should keep using virtual addresses.

So I think we should revert this and then split out the loading part of this patch from the vFlash thingy so we can focus on the virtual/physical address situation and how to handle that.

[owenpshaw]

And I'm not even completely clear about your case. I understand you write the module to the physical address, but what happens when you actually go around to debugging it? Is it still there or does it get copied/mapped/whatever to the virtual address?

In my case it's a hardware target with ROM and RAM. Only the data section has different virtual and physical addresses. That data section should be loaded to its physical address in ROM so it'll be preserved across resets. The first code to execute on boot copies the data section from ROM to its virtual address in RAM.

So it seems to me the physical addresses should really be specific to the "target modules load --load" case. I've never used that command so I don't know if it can just use physical addresses unconditionally, or whether we need an extra option for that "target modules load --load --physical". I think I'll leave that decision up to you (or anyone else who has an opinion on this). But for the other use cases, I believe we should keep using virtual addresses.

The "target modules load --load" case was originally intended to accomplish what gdb's "load" command does (see https://reviews.llvm.org/D28804). I know gdb uses the physical address when writes the sections.

So I think we should revert this and then split out the loading part of this patch from the vFlash thingy so we can focus on the virtual/physical address situation and how to handle that.

It's like we need the original all-virtual SectionLoadList in ObjectFileELF::SetLoadAddress, but a different SectionLoadList for the ObjectFile::LoadInMemory case, a function which is only used by "target modules load --load".

Or instead instead of a second list, maybe SectionLoadList gets a second pair of addr<->section maps that contain physical addresses. Then ObjectFile::LoadInMemory can ask for the physical instead of virtual.

[labath]

In D42145#1022717, @owenpshaw wrote:

And I'm not even completely clear about your case. I understand you write the module to the physical address, but what happens when you actually go around to debugging it? Is it still there or does it get copied/mapped/whatever to the virtual address?

In my case it's a hardware target with ROM and RAM. Only the data section has different virtual and physical addresses. That data section should be loaded to its physical address in ROM so it'll be preserved across resets. The first code to execute on boot copies the data section from ROM to its virtual address in RAM.

Ok, so it seems to me that we do want the rest of the debugger to operate on virtual addresses, as that's what you will be debugging (except when you are actually debugging the boot code itself, but we can't be correct all the time.

It's like we need the original all-virtual SectionLoadList in ObjectFileELF::SetLoadAddress, but a different SectionLoadList for the ObjectFile::LoadInMemory case, a function which is only used by "target modules load --load".

Or instead instead of a second list, maybe SectionLoadList gets a second pair of addr<->section maps that contain physical addresses. Then ObjectFile::LoadInMemory can ask for the physical instead of virtual.

Since there is just one caller of this function maybe we don't even need to that fancy. Could the LoadInMemory function do the shifting itself?
I'm thinking of something like where it takes the load bias as the argument and then adds that to the physical address it obtains from the object file. Thinking about that, I'm not even sure the function should be operating at the section level. If it's going to be simulating a loader, shouldn't it be based on program headers and segments completely (and not just use them for obtaining the physical address)?

[owenpshaw]

Since there is just one caller of this function maybe we don't even need to that fancy. Could the LoadInMemory function do the shifting itself?
I'm thinking of something like where it takes the load bias as the argument and then adds that to the physical address it obtains from the object file. Thinking about that, I'm not even sure the function should be operating at the section level. If it's going to be simulating a loader, shouldn't it be based on program headers and segments completely (and not just use them for obtaining the physical address)?

The original LoadInMemory thread from over a year ago discusses segments vs sections a little bit: http://lists.llvm.org/pipermail/lldb-dev/2016-December/011758.html. Sounds like gdb used sections, so that was deemed ok for lldb. It also fit with the pre-existing "target modules load" command that allowed section addresses to be specified individually. At one point, Greg suggests:

Since the arguments to this command are free form, we could pass these arguments to ObjectFile::LoadInMemory() and let each loader (ObjectFileELF and ObjectFileMach) determine what arguments are valid

But that didn't end up in the implementation. Perhaps it should, and ObjectFileELF could disallow any --load that specified sections individually. And if we're doing a special ObjectFileELF::LoadInMemory anyway, it could go ahead and use segments.

[owenpshaw]

• Revert changes to SetLoadAddress (always use virtual address there)
• Override LoadInMemory in ObjectFileELF to just load segments using physical address instead of using section load list

Passes tests, but I don't have access to hardware this week to double check that this works in the real world.

As an effect of this change, the "--load" part of "target modules load" now ignores the other command arguments for setting section addresses (--slide and/or the section/addr pairs). --slide would be easy to add back, but would probably need to become an argument to LoadInMemory. Does anyone have a clear sense of when someone would want to use --slide in combination with --load? What about section/add pairs? Should the --load behavior instead be spun off into a new command?

[owenpshaw]

Reopening since the previous land was reverted

[labath]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

3500–3518 ↗

(On Diff #137420)

This is the only part that's ELF-specific. Could we factor this out into a separate function (and leave the common part in ObjectFile)? Besides reducing duplication this will also make it easier to remove the Target/Process/RegisterContext uses from the ObjectFile in the future, as there will be just a single place that needs to be updated.

[owenpshaw]

• Share some common code in LoadInMemory

The DoLoadInMemory implementations call target.CalculateProcess() again, but passing Process or Process::WriteEntries to DoLoadInMemory would require either moving the Process.h include to ObjectFile.h or using forward declarations and pointers. Calling CalculateProcess felt like the lest intrusive option and has little cost. Open to other options.

[labath]

I thought the DoLoadInMemory function (which should probably be called differently then) could just return a vector<WriteEntry> and the actual writing would still happen in the LoadInMemory function.

[owenpshaw]

It can be done that way. As I mentioned, it would require moving the Process.h import from ObjectFile.cpp to ObjectFile.h (to get the declaration of Process::WriteEntry). I'm asking how you guys feel about that. Without a clear sense of project norms, I'll always go for the less intrusive change, which in this case meant a slightly different signature instead of moving an import that wasn't necessary.

[labath]

Ah, sorry. I didn't get where you were going with your previous comment. The ObjectFile->Process dependency is something that I think we shouldn't have, but whether it's in an header or implementation file makes little difference to me. So I think moving the #include is fine.

otoh, since we wen't down the path of having a separate Process::WriteObjectFile method, and the only usage of WriteEntry is there, I can see a case for moving this struct declaration into the ObjectFile class and avoiding this issue altogether.

[owenpshaw]

Looking at LoadInMemory, it seems like the common code is actually more appropriate in the command handler. It's checking inputs and doing the unrelated task of setting the pc. So here's an attempt at going a little further to simplify ObjectFile and its dependencies

• Have the command check for a valid process, write to the process, and set the pc
• Replace ObjectFile::LoadInMemory with GetLoadableData
• Move/rename the Process::WriteEntry struct to ObjectFile::LoadableData

Thoughts?

[labath]

I like this a lot. That's the kind of change I wanted to do as a follow-up one day. Thank you.

[owenpshaw]

In D42145#1034499, @labath wrote:

I like this a lot. That's the kind of change I wanted to do as a follow-up one day. Thank you.

Thanks! I don't have commit access to land this.

lldb/trunk/include/lldb/Host/XML.h

public:
 
   XMLNode GetSibling() const;
 
   XMLNode GetChild() const;
 
   llvm::StringRef GetAttributeValue(const char *name,
                                     const char *fail_value = nullptr) const;
 
+  bool GetAttributeValueAsUnsigned(const char *name, uint64_t &value,
+                                   uint64_t fail_value = 0, int base = 0) const;
+
   XMLNode FindFirstChildElementWithName(const char *name) const;
 
   XMLNode GetElementForPath(const NamePath &path);
 
   //----------------------------------------------------------------------
   // Iterate through all sibling nodes of any type
   //----------------------------------------------------------------------
   void ForEachSiblingNode(NodeCallback const &callback) const;

lldb/trunk/include/lldb/Target/MemoryRegionInfo.h

 class MemoryRegionInfo {
 public:
   typedef Range<lldb::addr_t, lldb::addr_t> RangeType;
 
   enum OptionalBool { eDontKnow = -1, eNo = 0, eYes = 1 };
 
   MemoryRegionInfo()
       : m_range(), m_read(eDontKnow), m_write(eDontKnow), m_execute(eDontKnow),
-        m_mapped(eDontKnow) {}
+        m_mapped(eDontKnow), m_flash(eDontKnow), m_blocksize(0) {}
 
   ~MemoryRegionInfo() {}
 
   RangeType &GetRange() { return m_range; }
 
   void Clear() {
     m_range.Clear();
     m_read = m_write = m_execute = eDontKnow;
public:
   void SetWritable(OptionalBool val) { m_write = val; }
 
   void SetExecutable(OptionalBool val) { m_execute = val; }
 
   void SetMapped(OptionalBool val) { m_mapped = val; }
 
   void SetName(const char *name) { m_name = ConstString(name); }
 
+  OptionalBool GetFlash() const { return m_flash; }
+
+  void SetFlash(OptionalBool val) { m_flash = val; }
+
+  lldb::offset_t GetBlocksize() const { return m_blocksize; }
+
+  void SetBlocksize(lldb::offset_t blocksize) { m_blocksize = blocksize; }
+
   //----------------------------------------------------------------------
   // Get permissions as a uint32_t that is a mask of one or more bits from
   // the lldb::Permissions
   //----------------------------------------------------------------------
   uint32_t GetLLDBPermissions() const {
     uint32_t permissions = 0;
     if (m_read)
       permissions |= lldb::ePermissionsReadable;

 
 protected:
   RangeType m_range;
   OptionalBool m_read;
   OptionalBool m_write;
   OptionalBool m_execute;
   OptionalBool m_mapped;
   ConstString m_name;
+  OptionalBool m_flash;
+  lldb::offset_t m_blocksize;
 };
 }
 
 namespace llvm {
 template <>
 struct format_provider<lldb_private::MemoryRegionInfo::OptionalBool> {
   static void format(const lldb_private::MemoryRegionInfo::OptionalBool &B,
                      raw_ostream &OS, StringRef Options) {

lldb/trunk/include/lldb/Target/Process.h

public:
   // These two functions fill out the Broadcaster interface:
 
   static ConstString &GetStaticBroadcasterClass();
 
   ConstString &GetBroadcasterClass() const override {
     return GetStaticBroadcasterClass();
   }
 
+  struct WriteEntry {
+    lldb::addr_t Dest;
+    llvm::ArrayRef<uint8_t> Contents;
+  };
+
 //------------------------------------------------------------------
 /// A notification structure that can be used by clients to listen
 /// for changes in a process's lifetime.
 ///
 /// @see RegisterNotificationCallbacks (const Notifications&)
 /// @see UnregisterNotificationCallbacks (const Notifications&)
 //------------------------------------------------------------------
 #ifndef SWIG
public:
   virtual lldb::addr_t DoAllocateMemory(size_t size, uint32_t permissions,
                                         Status &error) {
     error.SetErrorStringWithFormat(
         "error: %s does not support allocating in the debug process",
         GetPluginName().GetCString());
     return LLDB_INVALID_ADDRESS;
   }
 
+  virtual Status WriteObjectFile(std::vector<WriteEntry> entries);
+
   //------------------------------------------------------------------
   /// The public interface to allocating memory in the process.
   ///
   /// This function will allocate memory in the process's address
   /// space.  This can't rely on the generic function calling mechanism,
   /// since that requires this function.
   ///
   /// @param[in] size

lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py

+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestGDBRemoteLoad(GDBRemoteTestBase):
+
+    def test_ram_load(self):
+        """Test loading an object file to a target's ram"""
+        target = self.createTarget("a.yaml")
+        process = self.connect(target)
+        self.dbg.HandleCommand("target modules load -l -s0")
+        self.assertPacketLogContains([
+                "M1000,4:c3c3c3c3",
+                "M1004,2:3232"
+                ])
+
+    @skipIfXmlSupportMissing
+    def test_flash_load(self):
+        """Test loading an object file to a target's flash memory"""
+
+        class Responder(MockGDBServerResponder):
+            def qSupported(self, client_supported):
+                return "PacketSize=3fff;QStartNoAckMode+;qXfer:memory-map:read+"
+
+            def qXferRead(self, obj, annex, offset, length):
+                if obj == "memory-map":
+                    return (self.MEMORY_MAP[offset:offset + length],
+                            offset + length < len(self.MEMORY_MAP))
+                return None, False
+
+            def other(self, packet):
+                if packet[0:11] == "vFlashErase":
+                    return "OK"
+                if packet[0:11] == "vFlashWrite":
+                    return "OK"
+                if packet == "vFlashDone":
+                    return "OK"
+                return ""
+
+            MEMORY_MAP = """<?xml version="1.0"?>
+<memory-map>
+  <memory type="ram" start="0x0" length="0x1000"/>
+  <memory type="flash" start="0x1000" length="0x1000">
+    <property name="blocksize">0x100</property>
+  </memory>
+  <memory type="ram" start="0x2000" length="0x1D400"/>
+</memory-map>
+"""
+
+        self.server.responder = Responder()
+        target = self.createTarget("a.yaml")
+        process = self.connect(target)
+        self.dbg.HandleCommand("target modules load -l -s0")
+        self.assertPacketLogContains([
+                "vFlashErase:1000,100",
+                "vFlashWrite:1000:\xc3\xc3\xc3\xc3",
+                "vFlashWrite:1004:\x32\x32",
+                "vFlashDone"
+                ])

lldb/trunk/source/Host/common/XML.cpp

#else
   attr_value = fail_value;
 #endif
   if (attr_value)
     return llvm::StringRef(attr_value);
   else
     return llvm::StringRef();
 }
 
+bool XMLNode::GetAttributeValueAsUnsigned(const char *name, uint64_t &value,
+                                          uint64_t fail_value, int base) const {
+#if defined(LIBXML2_DEFINED)
+  llvm::StringRef str_value = GetAttributeValue(name, "");
+#else
+  llvm::StringRef str_value;
+#endif
+  bool success = false;
+  value = StringConvert::ToUInt64(str_value.data(), fail_value, base, &success);
+  return success;
+}
+
 void XMLNode::ForEachChildNode(NodeCallback const &callback) const {
 #if defined(LIBXML2_DEFINED)
   if (IsValid())
     GetChild().ForEachSiblingNode(callback);
 #endif
 }
 
 void XMLNode::ForEachChildElement(NodeCallback const &callback) const {

lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.h

private:
   const elf::ELFDynamic *FindDynamicSymbol(unsigned tag);
 
   unsigned PLTRelocationType();
 
   static lldb_private::Status
   RefineModuleDetailsFromNote(lldb_private::DataExtractor &data,
                               lldb_private::ArchSpec &arch_spec,
                               lldb_private::UUID &uuid);
+
+  bool AnySegmentHasPhysicalAddress();
+
+  const elf::ELFProgramHeader *GetSectionSegment(lldb::SectionSP section_sp);
+
+  lldb::addr_t GetSectionPhysicalAddress(lldb::SectionSP section_sp);
 };
 
 #endif // liblldb_ObjectFileELF_h_

lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

if (section_list) {
         for (size_t i = 1, count = GetProgramHeaderCount(); i <= count; ++i) {
           const elf::ELFProgramHeader *header = GetProgramHeaderByIndex(i);
           if (header == nullptr)
             continue;
 
           if (header->p_type != PT_LOAD || header->p_offset != 0)
             continue;
 
           value = value - header->p_vaddr;
           found_offset = true;
           break;
         }
         if (!found_offset)
           return false;
       }
 
       const size_t num_sections = section_list->GetSize();
       size_t sect_idx = 0;
 
       for (sect_idx = 0; sect_idx < num_sections; ++sect_idx) {
         // Iterate through the object file sections to find all
         // of the sections that have SHF_ALLOC in their flag bits.
         SectionSP section_sp(section_list->GetSectionAtIndex(sect_idx));
         if (section_sp && section_sp->Test(SHF_ALLOC)) {
-          lldb::addr_t load_addr = section_sp->GetFileAddress();
+          lldb::addr_t load_addr = GetSectionPhysicalAddress(section_sp);
           // We don't want to update the load address of a section with type
           // eSectionTypeAbsoluteAddress as they already have the absolute load
           // address
           // already specified
           if (section_sp->GetType() != eSectionTypeAbsoluteAddress)
             load_addr += value;
 
           // On 32-bit systems the load address have to fit into 4 bytes. The
if (auto Error = Decompressor->decompress(
            size_t(buffer_sp->GetByteSize())})) {
     LLDB_LOG_ERROR(log, std::move(Error), "Decompression of section {0} failed",
                    section->GetName());
     return result;
   }
   section_data.SetData(buffer_sp);
   return buffer_sp->GetByteSize();
 }
+
+bool ObjectFileELF::AnySegmentHasPhysicalAddress() {
+  size_t header_count = ParseProgramHeaders();
+  for (size_t i = 1; i <= header_count; ++i) {
+    auto header = GetProgramHeaderByIndex(i);
+    if (header->p_paddr != 0)
+      return true;
+  }
+  return false;
+}
+
+const elf::ELFProgramHeader *
+ObjectFileELF::GetSectionSegment(SectionSP section_sp) {
+  auto section_size = section_sp->GetFileSize();
+  if (section_size == 0)
+    section_size = 1;
+  size_t header_count = ParseProgramHeaders();
+  for (size_t i = 1; i <= header_count; ++i) {
+    auto header = GetProgramHeaderByIndex(i);
+    if (section_sp->GetFileOffset() >= header->p_offset &&
+        section_sp->GetFileOffset() + section_size <=
+            header->p_offset + header->p_filesz)
+      return header;
+  }
+  return nullptr;
+}
+
+addr_t ObjectFileELF::GetSectionPhysicalAddress(SectionSP section_sp) {
+  auto segment = GetSectionSegment(section_sp);
+  if (segment == nullptr)
+    return section_sp->GetFileAddress();
+  if (segment->p_type != PT_LOAD)
+    return LLDB_INVALID_ADDRESS;
+  auto base_address =
+      AnySegmentHasPhysicalAddress() ? segment->p_paddr : segment->p_vaddr;
+  return base_address + (section_sp->GetFileOffset() - segment->p_offset);
+}

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

public:
   bool GetEchoSupported();
 
   bool GetQPassSignalsSupported();
 
   bool GetAugmentedLibrariesSVR4ReadSupported();
 
   bool GetQXferFeaturesReadSupported();
 
+  bool GetQXferMemoryMapReadSupported();
+
   LazyBool SupportsAllocDeallocMemory() // const
   {
     // Uncomment this to have lldb pretend the debug server doesn't respond to
     // alloc/dealloc memory packets.
     // m_supports_alloc_dealloc_memory = lldb_private::eLazyBoolNo;
     return m_supports_alloc_dealloc_memory;
   }
 
protected:
   LazyBool m_supports_p;
   LazyBool m_supports_x;
   LazyBool m_avoid_g_packets;
   LazyBool m_supports_QSaveRegisterState;
   LazyBool m_supports_qXfer_auxv_read;
   LazyBool m_supports_qXfer_libraries_read;
   LazyBool m_supports_qXfer_libraries_svr4_read;
   LazyBool m_supports_qXfer_features_read;
+  LazyBool m_supports_qXfer_memory_map_read;
   LazyBool m_supports_augmented_libraries_svr4_read;
   LazyBool m_supports_jThreadExtendedInfo;
   LazyBool m_supports_jLoadedDynamicLibrariesInfos;
   LazyBool m_supports_jGetSharedCacheInfo;
   LazyBool m_supports_QPassSignals;
   LazyBool m_supports_error_string_reply;
 
   bool m_supports_qProcessInfoPID : 1, m_supports_qfProcessInfo : 1,
uint32_t m_gdb_server_version; // from reply to qGDBServerVersion, zero if
                                  // qGDBServerVersion is not supported
   std::chrono::seconds m_default_packet_timeout;
   uint64_t m_max_packet_size;        // as returned by qSupported
   std::string m_qSupported_response; // the complete response to qSupported
 
   bool m_supported_async_json_packets_is_valid;
   lldb_private::StructuredData::ObjectSP m_supported_async_json_packets_sp;
 
+  std::vector<MemoryRegionInfo> m_qXfer_memory_map;
+  bool m_qXfer_memory_map_loaded;
+
   bool GetCurrentProcessInfo(bool allow_lazy_pid = true);
 
   bool GetGDBServerVersion();
 
   // Given the list of compression types that the remote debug stub can support,
   // possibly enable compression if we find an encoding we can handle.
   void MaybeEnableCompression(std::vector<std::string> supported_compressions);
 
   bool DecodeProcessInfoResponse(StringExtractorGDBRemote &response,
                                  ProcessInstanceInfo &process_info);
 
   void OnRunPacketSent(bool first) override;
 
   PacketResult SendThreadSpecificPacketAndWaitForResponse(
       lldb::tid_t tid, StreamString &&payload,
       StringExtractorGDBRemote &response, bool send_async);
 
   Status SendGetTraceDataPacket(StreamGDBRemote &packet, lldb::user_id_t uid,
                                 lldb::tid_t thread_id,
                                 llvm::MutableArrayRef<uint8_t> &buffer,
                                 size_t offset);
 
+  Status LoadQXferMemoryMap();
+
+  Status GetQXferMemoryMapRegionInfo(lldb::addr_t addr,
+                                     MemoryRegionInfo &region);
+
 private:
   DISALLOW_COPY_AND_ASSIGN(GDBRemoteCommunicationClient);
 };
 
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif // liblldb_GDBRemoteCommunicationClient_h_

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

 // C++ Includes
 #include <numeric>
 #include <sstream>
 
 // Other libraries and framework includes
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/State.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/Host/XML.h"
 #include "lldb/Interpreter/Args.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/JSON.h"
 #include "lldb/Utility/LLDBAssert.h"
: GDBRemoteClientBase("gdb-remote.client", "gdb-remote.client.rx_packet"),
       m_prepare_for_reg_writing_reply(eLazyBoolCalculate),
       m_supports_p(eLazyBoolCalculate), m_supports_x(eLazyBoolCalculate),
       m_avoid_g_packets(eLazyBoolCalculate),
       m_supports_QSaveRegisterState(eLazyBoolCalculate),
       m_supports_qXfer_auxv_read(eLazyBoolCalculate),
       m_supports_qXfer_libraries_read(eLazyBoolCalculate),
       m_supports_qXfer_libraries_svr4_read(eLazyBoolCalculate),
       m_supports_qXfer_features_read(eLazyBoolCalculate),
+      m_supports_qXfer_memory_map_read(eLazyBoolCalculate),
       m_supports_augmented_libraries_svr4_read(eLazyBoolCalculate),
       m_supports_jThreadExtendedInfo(eLazyBoolCalculate),
       m_supports_jLoadedDynamicLibrariesInfos(eLazyBoolCalculate),
       m_supports_jGetSharedCacheInfo(eLazyBoolCalculate),
       m_supports_QPassSignals(eLazyBoolCalculate),
       m_supports_error_string_reply(eLazyBoolCalculate),
       m_supports_qProcessInfoPID(true), m_supports_qfProcessInfo(true),
       m_supports_qUserName(true), m_supports_qGroupName(true),
       m_supports_qThreadStopInfo(true), m_supports_z0(true),
       m_supports_z1(true), m_supports_z2(true), m_supports_z3(true),
       m_supports_z4(true), m_supports_QEnvironment(true),
       m_supports_QEnvironmentHexEncoded(true), m_supports_qSymbol(true),
       m_qSymbol_requests_done(false), m_supports_qModuleInfo(true),
       m_supports_jThreadsInfo(true), m_supports_jModulesInfo(true),
       m_curr_pid(LLDB_INVALID_PROCESS_ID), m_curr_tid(LLDB_INVALID_THREAD_ID),
       m_curr_tid_run(LLDB_INVALID_THREAD_ID),
       m_num_supported_hardware_watchpoints(0), m_host_arch(), m_process_arch(),
       m_os_version_major(UINT32_MAX), m_os_version_minor(UINT32_MAX),
       m_os_version_update(UINT32_MAX), m_os_build(), m_os_kernel(),
       m_hostname(), m_gdb_server_name(), m_gdb_server_version(UINT32_MAX),
       m_default_packet_timeout(0), m_max_packet_size(0),
       m_qSupported_response(), m_supported_async_json_packets_is_valid(false),
-      m_supported_async_json_packets_sp() {}
+      m_supported_async_json_packets_sp(), m_qXfer_memory_map(),
+      m_qXfer_memory_map_loaded(false) {}
 
 //----------------------------------------------------------------------
 // Destructor
 //----------------------------------------------------------------------
 GDBRemoteCommunicationClient::~GDBRemoteCommunicationClient() {
   if (IsConnected())
     Disconnect();
 }

 
 bool GDBRemoteCommunicationClient::GetQXferFeaturesReadSupported() {
   if (m_supports_qXfer_features_read == eLazyBoolCalculate) {
     GetRemoteQSupported();
   }
   return m_supports_qXfer_features_read == eLazyBoolYes;
 }
 
+bool GDBRemoteCommunicationClient::GetQXferMemoryMapReadSupported() {
+  if (m_supports_qXfer_memory_map_read == eLazyBoolCalculate) {
+    GetRemoteQSupported();
+  }
+  return m_supports_qXfer_memory_map_read == eLazyBoolYes;
+}
+
 uint64_t GDBRemoteCommunicationClient::GetRemoteMaxPacketSize() {
   if (m_max_packet_size == 0) {
     GetRemoteQSupported();
   }
   return m_max_packet_size;
 }
 
 bool GDBRemoteCommunicationClient::QueryNoAckModeSupported() {
if (did_exec == false) {
     m_supports_memory_region_info = eLazyBoolCalculate;
     m_prepare_for_reg_writing_reply = eLazyBoolCalculate;
     m_attach_or_wait_reply = eLazyBoolCalculate;
     m_avoid_g_packets = eLazyBoolCalculate;
     m_supports_qXfer_auxv_read = eLazyBoolCalculate;
     m_supports_qXfer_libraries_read = eLazyBoolCalculate;
     m_supports_qXfer_libraries_svr4_read = eLazyBoolCalculate;
     m_supports_qXfer_features_read = eLazyBoolCalculate;
+    m_supports_qXfer_memory_map_read = eLazyBoolCalculate;
     m_supports_augmented_libraries_svr4_read = eLazyBoolCalculate;
     m_supports_qProcessInfoPID = true;
     m_supports_qfProcessInfo = true;
     m_supports_qUserName = true;
     m_supports_qGroupName = true;
     m_supports_qThreadStopInfo = true;
     m_supports_z0 = true;
     m_supports_z1 = true;

 
 void GDBRemoteCommunicationClient::GetRemoteQSupported() {
   // Clear out any capabilities we expect to see in the qSupported response
   m_supports_qXfer_auxv_read = eLazyBoolNo;
   m_supports_qXfer_libraries_read = eLazyBoolNo;
   m_supports_qXfer_libraries_svr4_read = eLazyBoolNo;
   m_supports_augmented_libraries_svr4_read = eLazyBoolNo;
   m_supports_qXfer_features_read = eLazyBoolNo;
+  m_supports_qXfer_memory_map_read = eLazyBoolNo;
   m_max_packet_size = UINT64_MAX; // It's supposed to always be there, but if
                                   // not, we assume no limit
 
   // build the qSupported packet
   std::vector<std::string> features = {"xmlRegisters=i386,arm,mips"};
   StreamString packet;
   packet.PutCString("qSupported");
   for (uint32_t i = 0; i < features.size(); ++i) {
if (SendPacketAndWaitForResponse(packet.GetString(), response,
     if (::strstr(response_cstr, "augmented-libraries-svr4-read")) {
       m_supports_qXfer_libraries_svr4_read = eLazyBoolYes; // implied
       m_supports_augmented_libraries_svr4_read = eLazyBoolYes;
     }
     if (::strstr(response_cstr, "qXfer:libraries:read+"))
       m_supports_qXfer_libraries_read = eLazyBoolYes;
     if (::strstr(response_cstr, "qXfer:features:read+"))
       m_supports_qXfer_features_read = eLazyBoolYes;
+    if (::strstr(response_cstr, "qXfer:memory-map:read+"))
+      m_supports_qXfer_memory_map_read = eLazyBoolYes;
 
     // Look for a list of compressions in the features list e.g.
     // qXfer:features:read+;PacketSize=20000;qEcho+;SupportedCompressions=zlib-deflate,lzma
     const char *features_list = ::strstr(response_cstr, "qXfer:features:");
     if (features_list) {
       const char *compressions =
           ::strstr(features_list, "SupportedCompressions=");
       if (compressions) {
if (m_supports_memory_region_info != eLazyBoolNo) {
     m_supports_memory_region_info = eLazyBoolYes;
     char packet[64];
     const int packet_len = ::snprintf(
         packet, sizeof(packet), "qMemoryRegionInfo:%" PRIx64, (uint64_t)addr);
     assert(packet_len < (int)sizeof(packet));
     UNUSED_IF_ASSERT_DISABLED(packet_len);
     StringExtractorGDBRemote response;
     if (SendPacketAndWaitForResponse(packet, response, false) ==
-        PacketResult::Success) {
+            PacketResult::Success &&
+        response.GetResponseType() == StringExtractorGDBRemote::eResponse) {
       llvm::StringRef name;
       llvm::StringRef value;
       addr_t addr_value = LLDB_INVALID_ADDRESS;
       bool success = true;
       bool saw_permissions = false;
       while (success && response.GetNameColonValue(name, value)) {
         if (name.equals("start")) {
           if (!value.getAsInteger(16, addr_value))
if (m_supports_memory_region_info != eLazyBoolNo) {
     } else {
       m_supports_memory_region_info = eLazyBoolNo;
     }
   }
 
   if (m_supports_memory_region_info == eLazyBoolNo) {
     error.SetErrorString("qMemoryRegionInfo is not supported");
   }
-  if (error.Fail())
+
+  // Try qXfer:memory-map:read to get region information not included in
+  // qMemoryRegionInfo
+  MemoryRegionInfo qXfer_region_info;
+  Status qXfer_error = GetQXferMemoryMapRegionInfo(addr, qXfer_region_info);
+
+  if (error.Fail()) {
+    // If qMemoryRegionInfo failed, but qXfer:memory-map:read succeeded,
+    // use the qXfer result as a fallback
+    if (qXfer_error.Success()) {
+      region_info = qXfer_region_info;
+      error.Clear();
+    } else {
       region_info.Clear();
+    }
+  } else if (qXfer_error.Success()) {
+    // If both qMemoryRegionInfo and qXfer:memory-map:read succeeded, and if
+    // both regions are the same range, update the result to include the
+    // flash-memory information that is specific to the qXfer result.
+    if (region_info.GetRange() == qXfer_region_info.GetRange()) {
+      region_info.SetFlash(qXfer_region_info.GetFlash());
+      region_info.SetBlocksize(qXfer_region_info.GetBlocksize());
+    }
+  }
+  return error;
+}
+
+Status GDBRemoteCommunicationClient::GetQXferMemoryMapRegionInfo(
+    lldb::addr_t addr, MemoryRegionInfo &region) {
+  Status error = LoadQXferMemoryMap();
+  if (!error.Success())
+    return error;
+  for (const auto &map_region : m_qXfer_memory_map) {
+    if (map_region.GetRange().Contains(addr)) {
+      region = map_region;
+      return error;
+    }
+  }
+  error.SetErrorString("Region not found");
+  return error;
+}
+
+Status GDBRemoteCommunicationClient::LoadQXferMemoryMap() {
+
+  Status error;
+
+  if (m_qXfer_memory_map_loaded)
+    // Already loaded, return success
+    return error;
+
+  if (!XMLDocument::XMLEnabled()) {
+    error.SetErrorString("XML is not supported");
+    return error;
+  }
+
+  if (!GetQXferMemoryMapReadSupported()) {
+    error.SetErrorString("Memory map is not supported");
+    return error;
+  }
+
+  std::string xml;
+  lldb_private::Status lldberr;
+  if (!ReadExtFeature(ConstString("memory-map"), ConstString(""), xml,
+                      lldberr)) {
+    error.SetErrorString("Failed to read memory map");
+    return error;
+  }
+
+  XMLDocument xml_document;
+
+  if (!xml_document.ParseMemory(xml.c_str(), xml.size())) {
+    error.SetErrorString("Failed to parse memory map xml");
+    return error;
+  }
+
+  XMLNode map_node = xml_document.GetRootElement("memory-map");
+  if (!map_node) {
+    error.SetErrorString("Invalid root node in memory map xml");
+    return error;
+  }
+
+  m_qXfer_memory_map.clear();
+
+  map_node.ForEachChildElement([this](const XMLNode &memory_node) -> bool {
+    if (!memory_node.IsElement())
+      return true;
+    if (memory_node.GetName() != "memory")
+      return true;
+    auto type = memory_node.GetAttributeValue("type", "");
+    uint64_t start;
+    uint64_t length;
+    if (!memory_node.GetAttributeValueAsUnsigned("start", start))
+      return true;
+    if (!memory_node.GetAttributeValueAsUnsigned("length", length))
+      return true;
+    MemoryRegionInfo region;
+    region.GetRange().SetRangeBase(start);
+    region.GetRange().SetByteSize(length);
+    if (type == "rom") {
+      region.SetReadable(MemoryRegionInfo::eYes);
+      this->m_qXfer_memory_map.push_back(region);
+    } else if (type == "ram") {
+      region.SetReadable(MemoryRegionInfo::eYes);
+      region.SetWritable(MemoryRegionInfo::eYes);
+      this->m_qXfer_memory_map.push_back(region);
+    } else if (type == "flash") {
+      region.SetFlash(MemoryRegionInfo::eYes);
+      memory_node.ForEachChildElement(
+          [&region](const XMLNode &prop_node) -> bool {
+            if (!prop_node.IsElement())
+              return true;
+            if (prop_node.GetName() != "property")
+              return true;
+            auto propname = prop_node.GetAttributeValue("name", "");
+            if (propname == "blocksize") {
+              uint64_t blocksize;
+              if (prop_node.GetElementTextAsUnsigned(blocksize))
+                region.SetBlocksize(blocksize);
+            }
+            return true;
+          });
+      this->m_qXfer_memory_map.push_back(region);
+    }
+    return true;
+  });
+
+  m_qXfer_memory_map_loaded = true;
+
   return error;
 }
 
 Status GDBRemoteCommunicationClient::GetWatchpointSupportInfo(uint32_t &num) {
   Status error;
 
   if (m_supports_watchpoint_support_info == eLazyBoolYes) {
     num = m_num_supported_hardware_watchpoints;

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

public:
   void WillPublicStop() override;
 
   //------------------------------------------------------------------
   // Process Memory
   //------------------------------------------------------------------
   size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
                       Status &error) override;
 
+  Status WriteObjectFile(std::vector<WriteEntry> entries) override;
+
   size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
                        Status &error) override;
 
   lldb::addr_t DoAllocateMemory(size_t size, uint32_t permissions,
                                 Status &error) override;
 
   Status GetMemoryRegionInfo(lldb::addr_t load_addr,
                              MemoryRegionInfo &region_info) override;
protected:
   MMapMap m_addr_to_mmap_size;
   lldb::BreakpointSP m_thread_create_bp_sp;
   bool m_waiting_for_attach;
   bool m_destroy_tried_resuming;
   lldb::CommandObjectSP m_command_sp;
   int64_t m_breakpoint_pc_offset;
   lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
 
+  bool m_allow_flash_writes;
+  using FlashRangeVector = lldb_private::RangeVector<lldb::addr_t, size_t>;
+  using FlashRange = FlashRangeVector::Entry;
+  FlashRangeVector m_erased_flash_ranges;
+
   //----------------------------------------------------------------------
   // Accessors
   //----------------------------------------------------------------------
   bool IsRunning(lldb::StateType state) {
     return state == lldb::eStateRunning || IsStepping(state);
   }
 
   bool IsStepping(lldb::StateType state) {
protected:
 
   lldb::ModuleSP LoadModuleAtAddress(const FileSpec &file,
                                      lldb::addr_t link_map,
                                      lldb::addr_t base_addr,
                                      bool value_is_offset);
 
   Status UpdateAutomaticSignalFiltering() override;
 
+  Status FlashErase(lldb::addr_t addr, size_t size);
+
+  Status FlashDone();
+
+  bool HasErased(FlashRange range);
+
 private:
   //------------------------------------------------------------------
   // For ProcessGDBRemote only
   //------------------------------------------------------------------
   std::string m_partial_profile_data;
   std::map<uint64_t, uint32_t> m_thread_id_to_used_usec_map;
   uint64_t m_last_signals_version = 0;
 

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

 #include "lldb/Interpreter/OptionGroupBoolean.h"
 #include "lldb/Interpreter/OptionGroupUInt64.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Interpreter/Property.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/ABI.h"
 #include "lldb/Target/DynamicLoader.h"
+#include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SystemRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/TargetList.h"
 #include "lldb/Target/ThreadPlanCallFunction.h"
 #include "lldb/Utility/CleanUp.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
: Process(target_sp, listener_sp),
           Listener::MakeListener("lldb.process.gdb-remote.async-listener")),
       m_async_thread_state_mutex(), m_thread_ids(), m_thread_pcs(),
       m_jstopinfo_sp(), m_jthreadsinfo_sp(), m_continue_c_tids(),
       m_continue_C_tids(), m_continue_s_tids(), m_continue_S_tids(),
       m_max_memory_size(0), m_remote_stub_max_memory_size(0),
       m_addr_to_mmap_size(), m_thread_create_bp_sp(),
       m_waiting_for_attach(false), m_destroy_tried_resuming(false),
       m_command_sp(), m_breakpoint_pc_offset(0),
-      m_initial_tid(LLDB_INVALID_THREAD_ID) {
+      m_initial_tid(LLDB_INVALID_THREAD_ID), m_allow_flash_writes(false),
+      m_erased_flash_ranges() {
   m_async_broadcaster.SetEventName(eBroadcastBitAsyncThreadShouldExit,
                                    "async thread should exit");
   m_async_broadcaster.SetEventName(eBroadcastBitAsyncContinue,
                                    "async thread continue");
   m_async_broadcaster.SetEventName(eBroadcastBitAsyncThreadDidExit,
                                    "async thread did exit");
 
   Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_ASYNC));
else
           "unexpected response to GDB server memory read packet '%s': '%s'",
           packet, response.GetStringRef().c_str());
   } else {
     error.SetErrorStringWithFormat("failed to send packet: '%s'", packet);
   }
   return 0;
 }
 
+Status ProcessGDBRemote::WriteObjectFile(std::vector<WriteEntry> entries) {
+  Status error;
+  // Sort the entries by address because some writes, like those to flash
+  // memory, must happen in order of increasing address.
+  std::stable_sort(
+      std::begin(entries), std::end(entries),
+      [](const WriteEntry a, const WriteEntry b) { return a.Dest < b.Dest; });
+  m_allow_flash_writes = true;
+  error = Process::WriteObjectFile(entries);
+  if (error.Success())
+    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
+    // since that's the one we want to report back.
+    FlashDone();
+  m_allow_flash_writes = false;
+  return error;
+}
+
+bool ProcessGDBRemote::HasErased(FlashRange range) {
+  auto size = m_erased_flash_ranges.GetSize();
+  for (size_t i = 0; i < size; ++i)
+    if (m_erased_flash_ranges.GetEntryAtIndex(i)->Contains(range))
+      return true;
+  return false;
+}
+
+Status ProcessGDBRemote::FlashErase(lldb::addr_t addr, size_t size) {
+  Status status;
+
+  MemoryRegionInfo region;
+  status = GetMemoryRegionInfo(addr, region);
+  if (!status.Success())
+    return status;
+
+  // The gdb spec doesn't say if erasures are allowed across multiple regions,
+  // but we'll disallow it to be safe and to keep the logic simple by worring
+  // about only one region's block size.  DoMemoryWrite is this function's
+  // primary user, and it can easily keep writes within a single memory region
+  if (addr + size > region.GetRange().GetRangeEnd()) {
+    status.SetErrorString("Unable to erase flash in multiple regions");
+    return status;
+  }
+
+  uint64_t blocksize = region.GetBlocksize();
+  if (blocksize == 0) {
+    status.SetErrorString("Unable to erase flash because blocksize is 0");
+    return status;
+  }
+
+  // Erasures can only be done on block boundary adresses, so round down addr
+  // and round up size
+  lldb::addr_t block_start_addr = addr - (addr % blocksize);
+  size += (addr - block_start_addr);
+  if ((size % blocksize) != 0)
+    size += (blocksize - size % blocksize);
+
+  FlashRange range(block_start_addr, size);
+
+  if (HasErased(range))
+    return status;
+
+  // We haven't erased the entire range, but we may have erased part of it.
+  // (e.g., block A is already erased and range starts in A and ends in B).
+  // So, adjust range if necessary to exclude already erased blocks.
+  if (!m_erased_flash_ranges.IsEmpty()) {
+    // Assuming that writes and erasures are done in increasing addr order,
+    // because that is a requirement of the vFlashWrite command.  Therefore,
+    // we only need to look at the last range in the list for overlap.
+    const auto &last_range = *m_erased_flash_ranges.Back();
+    if (range.GetRangeBase() < last_range.GetRangeEnd()) {
+      auto overlap = last_range.GetRangeEnd() - range.GetRangeBase();
+      // overlap will be less than range.GetByteSize() or else HasErased() would
+      // have been true
+      range.SetByteSize(range.GetByteSize() - overlap);
+      range.SetRangeBase(range.GetRangeBase() + overlap);
+    }
+  }
+
+  StreamString packet;
+  packet.Printf("vFlashErase:%" PRIx64 ",%" PRIx64, range.GetRangeBase(),
+                (uint64_t)range.GetByteSize());
+
+  StringExtractorGDBRemote response;
+  if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetString(), response,
+                                              true) ==
+      GDBRemoteCommunication::PacketResult::Success) {
+    if (response.IsOKResponse()) {
+      m_erased_flash_ranges.Insert(range, true);
+    } else {
+      if (response.IsErrorResponse())
+        status.SetErrorStringWithFormat("flash erase failed for 0x%" PRIx64,
+                                        addr);
+      else if (response.IsUnsupportedResponse())
+        status.SetErrorStringWithFormat("GDB server does not support flashing");
+      else
+        status.SetErrorStringWithFormat(
+            "unexpected response to GDB server flash erase packet '%s': '%s'",
+            packet.GetData(), response.GetStringRef().c_str());
+    }
+  } else {
+    status.SetErrorStringWithFormat("failed to send packet: '%s'",
+                                    packet.GetData());
+  }
+  return status;
+}
+
+Status ProcessGDBRemote::FlashDone() {
+  Status status;
+  // If we haven't erased any blocks, then we must not have written anything
+  // either, so there is no need to actually send a vFlashDone command
+  if (m_erased_flash_ranges.IsEmpty())
+    return status;
+  StringExtractorGDBRemote response;
+  if (m_gdb_comm.SendPacketAndWaitForResponse("vFlashDone", response, true) ==
+      GDBRemoteCommunication::PacketResult::Success) {
+    if (response.IsOKResponse()) {
+      m_erased_flash_ranges.Clear();
+    } else {
+      if (response.IsErrorResponse())
+        status.SetErrorStringWithFormat("flash done failed");
+      else if (response.IsUnsupportedResponse())
+        status.SetErrorStringWithFormat("GDB server does not support flashing");
+      else
+        status.SetErrorStringWithFormat(
+            "unexpected response to GDB server flash done packet: '%s'",
+            response.GetStringRef().c_str());
+    }
+  } else {
+    status.SetErrorStringWithFormat("failed to send flash done packet");
+  }
+  return status;
+}
+
 size_t ProcessGDBRemote::DoWriteMemory(addr_t addr, const void *buf,
                                        size_t size, Status &error) {
   GetMaxMemorySize();
   // M and m packets take 2 bytes for 1 byte of memory
   size_t max_memory_size = m_max_memory_size / 2;
   if (size > max_memory_size) {
     // Keep memory read sizes down to a sane limit. This function will be
     // called multiple times in order to complete the task by
     // lldb_private::Process so it is ok to do this.
     size = max_memory_size;
   }
 
-  StreamString packet;
+  StreamGDBRemote packet;
+
+  MemoryRegionInfo region;
+  Status region_status = GetMemoryRegionInfo(addr, region);
+
+  bool is_flash =
+      region_status.Success() && region.GetFlash() == MemoryRegionInfo::eYes;
+
+  if (is_flash) {
+    if (!m_allow_flash_writes) {
+      error.SetErrorString("Writing to flash memory is not allowed");
+      return 0;
+    }
+    // Keep the write within a flash memory region
+    if (addr + size > region.GetRange().GetRangeEnd())
+      size = region.GetRange().GetRangeEnd() - addr;
+    // Flash memory must be erased before it can be written
+    error = FlashErase(addr, size);
+    if (!error.Success())
+      return 0;
+    packet.Printf("vFlashWrite:%" PRIx64 ":", addr);
+    packet.PutEscapedBytes(buf, size);
+  } else {
     packet.Printf("M%" PRIx64 ",%" PRIx64 ":", addr, (uint64_t)size);
     packet.PutBytesAsRawHex8(buf, size, endian::InlHostByteOrder(),
                              endian::InlHostByteOrder());
+  }
   StringExtractorGDBRemote response;
   if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetString(), response,
                                               true) ==
       GDBRemoteCommunication::PacketResult::Success) {
     if (response.IsOKResponse()) {
       error.Clear();
       return size;
     } else if (response.IsErrorResponse())

lldb/trunk/source/Symbol/ObjectFile.cpp

Status ObjectFile::LoadInMemory(Target &target, bool set_pc) {
   if (!process)
     return Status("No Process");
   if (set_pc && !GetEntryPointAddress().IsValid())
     return Status("No entry address in object file");
 
   SectionList *section_list = GetSectionList();
   if (!section_list)
     return Status("No section in object file");
+
+  std::vector<Process::WriteEntry> writeEntries;
+
+  // Create a list of write entries from loadable sections
   size_t section_count = section_list->GetNumSections(0);
   for (size_t i = 0; i < section_count; ++i) {
+    Process::WriteEntry entry;
     SectionSP section_sp = section_list->GetSectionAtIndex(i);
-    addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-    if (addr != LLDB_INVALID_ADDRESS) {
-      DataExtractor section_data;
-      // We can skip sections like bss
-      if (section_sp->GetFileSize() == 0)
-        continue;
-      section_sp->GetSectionData(section_data);
-      lldb::offset_t written = process->WriteMemory(
-          addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
-      if (written != section_data.GetByteSize())
-        return error;
-    }
+    entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+    if (entry.Dest == LLDB_INVALID_ADDRESS)
+      continue;
+    DataExtractor section_data;
+    entry.Contents = llvm::ArrayRef<uint8_t>(section_data.GetDataStart(),
+                                             section_data.GetByteSize());
+    writeEntries.push_back(entry);
   }
+
+  error = process->WriteObjectFile(std::move(writeEntries));
+  if (!error.Success())
+    return error;
+
   if (set_pc) {
     ThreadList &thread_list = process->GetThreadList();
     ThreadSP curr_thread(thread_list.GetSelectedThread());
     RegisterContextSP reg_context(curr_thread->GetRegisterContext());
     Address file_entry = GetEntryPointAddress();
     reg_context->SetPC(file_entry.GetLoadAddress(&target));
   }
   return error;

lldb/trunk/source/Target/Process.cpp

if (byte_size == 0) {
     }
   } else {
     error.SetErrorStringWithFormat(
         "byte size of %u is too large for integer scalar type", byte_size);
   }
   return 0;
 }
 
+Status Process::WriteObjectFile(std::vector<WriteEntry> entries) {
+  Status error;
+  for (const auto &Entry : entries) {
+    WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+                error);
+    if (!error.Success())
+      break;
+  }
+  return error;
+}
+
 #define USE_ALLOCATE_MEMORY_CACHE 1
 addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
                                Status &error) {
   if (GetPrivateState() != eStateStopped)
     return LLDB_INVALID_ADDRESS;
 
 #if defined(USE_ALLOCATE_MEMORY_CACHE)
   return m_allocated_memory_cache.AllocateMemory(size, permissions, error);