Looks good to me. Thanks!

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.

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

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.

• Share some common code in LoadInMemory

Reopening since the previous land was reverted

• 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

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

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?

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

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

Update patch to include new @skipIfXmlSupportMissing decorator on test

Looks good to me

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

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.

• 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

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

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.

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

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?

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 for the help! Someone with commit access will need to land it.

• Updated condition for yaml2obj dependency, now based on LLDB_BUILT_STANDALONE
• Find yaml2obj in the same directory as clang
• Move yaml2obj code into lldbtest.py so it's available to all tests

Just wanted to check in on this one since it hasn't landed yet. Is there anything else I need to do?

Added back socket close()

• Updated to use TestBase.process() instead of a new member
• Added yaml2obj dependency. It it okay to be an unconditional dependency?
• I can put the socket close() back in, but had removed it because python should close it automatically on garbage collection, so it didn't seem to matter either way

Updated with suggestions

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.

• 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

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.

Thanks for the help getting this one sorted out! I don't have commit access.

Updated with latest suggestions

Updated according to suggestions. I didn't change to using member pointers, but can if you prefer. While I usually agree that de-duplication is best, in this case I feel like it makes the calling code more difficult to decipher in exchange for little gain in de-duplication.

Fix comparison to handle cases two segments have the same PAddr

Inlined the binary segment layout and renamed the test.

New patch offsets segments according to difference from lowest PAddr.

If I ignore alignment for binary output and offset segments based on their distance from the lowest PAddr, a few tests break. I think the tests need to change, but looking for other opinions before proceeding.

Thanks! I don't have commit access, so someone needs to commit this for me, right?

Updated patch with new function name suggested by @clayborg. Added unit test and changed to llvm::function_ref as suggested by @labath.

Looks like binutils doesn't bother with any alignment, and just offsets everything from the lowest PAddr, which seems reasonable given that in the embedded case the binary will likely be loaded on a device as-is at that lowest PAddr. I can update the patch to do this when using PAddr instead of calling align.

Updated to use std::find_if

Updated patch to only use PAddr for binary output, and to populate PAddr with VAddr if every segment has a 0 value for PAddr initially. Added new tests instead modifying existing alignment test.

Still learning the process here, so let me know how you'd like to proceed. I really only opened this to get a discussion going because there wasn't activity on the bug after a week or so...maybe too impatient, after all it was the holidays :)

Closing. As already noted in mailing list, Rafael committed this change as r321418.

Thanks for the help!