- User Since
- Dec 17 2017, 2:10 PM (87 w, 3 d)
Mar 23 2018
Looks good to me. Thanks!
Mar 19 2018
Mar 9 2018
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.
Mar 8 2018
- Share some common code in LoadInMemory
Mar 7 2018
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
Feb 28 2018
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?
Feb 27 2018
Feb 26 2018
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.
Feb 22 2018
Feb 20 2018
Update patch to include new @skipIfXmlSupportMissing decorator on test
Feb 7 2018
Looks good to me
Feb 2 2018
- adjust WriteObjectFile signature to return Status and take an std::vector
- match project formatting style
Feb 1 2018
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.
Jan 31 2018
- 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?
Jan 30 2018
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).
Jan 29 2018
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?
Jan 25 2018
Thanks for the help! Someone with commit access will need to land it.
Jan 24 2018
- 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
Jan 22 2018
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()
Jan 19 2018
- 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
Jan 18 2018
Updated with suggestions
Jan 17 2018
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.
Jan 16 2018
Jan 11 2018
Thanks for the help getting this one sorted out! I don't have commit access.
Updated with latest suggestions
Jan 10 2018
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.
Jan 9 2018
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.
Jan 8 2018
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?
Jan 4 2018
Jan 3 2018
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.
Jan 2 2018
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 :)
Dec 28 2017
Dec 27 2017
Closing. As already noted in mailing list, Rafael committed this change as r321418.
Dec 21 2017
Thanks for the help!