This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by owenpshaw on Jan 16 2018, 4:21 PM.

Details

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.

Diff Detail

Event Timeline

owenpshaw created this revision.Jan 16 2018, 4:21 PM

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
1921

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
44

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
2966

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

source/Symbol/ObjectFile.cpp
671

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 requested changes to this revision.Jan 17 2018, 9:58 AM

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
1921

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.

1936

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

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

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

2812–2821

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

2966

Indeed

4618–4619

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

Very important to zero out bss

682–683

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

687–688

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

696

Remove EndWriteMemoryBatch 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().

This revision now requires changes to proceed.Jan 17 2018, 9:58 AM

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
1921

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
4618–4619

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

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 added inline comments.Jan 17 2018, 11:03 AM
include/lldb/Target/Process.h
1921

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

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

owenpshaw updated this revision to Diff 130258.Jan 17 2018, 1:19 PM
  • 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.

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

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?

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

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.

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

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.

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?

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

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.

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?

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.

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.

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

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 updated this revision to Diff 132260.Jan 31 2018, 1:36 PM
  • 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 added a comment.Feb 1 2018, 2:35 AM

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

Please run clang format over your diff.

1958

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

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

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

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.

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

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

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

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.

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

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

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

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 updated this revision to Diff 132641.Feb 2 2018, 11:41 AM
  • 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 added a comment.Feb 5 2018, 3:49 AM
  • 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 updated this revision to Diff 135087.Feb 20 2018, 9:49 AM

Update patch to include new @skipIfXmlSupportMissing decorator on test

clayborg added inline comments.Feb 22 2018, 8:53 AM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
3517

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

owenpshaw added inline comments.Feb 22 2018, 10:45 AM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
3517

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

@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 accepted this revision.Feb 26 2018, 3:35 PM

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.

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

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.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 27 2018, 2:16 PM
This revision was automatically updated to reflect the committed changes.

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.

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 added inline comments.Feb 27 2018, 5:53 PM
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
814 ↗(On Diff #136156)

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 added inline comments.Feb 27 2018, 10:09 PM
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
814 ↗(On Diff #136156)

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 added inline comments.Feb 28 2018, 11:53 AM
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
814 ↗(On Diff #136156)

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.

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.

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

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 updated this revision to Diff 137420.Mar 7 2018, 9:57 AM
  • 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 reopened this revision.Mar 7 2018, 9:57 AM

Reopening since the previous land was reverted

labath added inline comments.Mar 8 2018, 3:11 AM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
3537–3555

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 updated this revision to Diff 137598.Mar 8 2018, 10:17 AM
  • 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 added a comment.Mar 9 2018, 2:52 AM

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.

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.

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 updated this revision to Diff 137827.Mar 9 2018, 1:48 PM

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 accepted this revision.Mar 12 2018, 5:26 AM

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

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.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 20 2018, 4:59 AM
This revision was automatically updated to reflect the committed changes.