Page MenuHomePhabricator

owenpshaw (Owen Shaw)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 17 2017, 2:10 PM (95 w, 4 d)

Recent Activity

Mar 23 2018

owenpshaw accepted D44738: Add a test for setting the load address of a module with differing physical/virtual addresses.

Looks good to me. Thanks!

Mar 23 2018, 9:12 AM

Mar 19 2018

owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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

Mar 19 2018, 4:46 PM

Mar 9 2018

owenpshaw updated the diff for D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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

Mar 9 2018, 1:48 PM
owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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 9 2018, 8:13 AM

Mar 8 2018

owenpshaw updated the diff for D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.
  • Share some common code in LoadInMemory
Mar 8 2018, 10:18 AM

Mar 7 2018

owenpshaw reopened D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

Reopening since the previous land was reverted

Mar 7 2018, 9:58 AM
owenpshaw updated the diff for D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.
  • 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
Mar 7 2018, 9:57 AM

Feb 28 2018

owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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

Feb 28 2018, 3:54 PM
owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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 28 2018, 1:19 PM

Feb 27 2018

owenpshaw added inline comments to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.
Feb 27 2018, 10:10 PM

Feb 26 2018

owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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

Feb 26 2018, 5:33 PM
owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

@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 26 2018, 2:48 PM

Feb 22 2018

owenpshaw added inline comments to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.
Feb 22 2018, 10:47 AM

Feb 20 2018

owenpshaw updated the diff for D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

Update patch to include new @skipIfXmlSupportMissing decorator on test

Feb 20 2018, 9:49 AM

Feb 7 2018

owenpshaw added a comment to D42959: Rewrite the flaky test_restart_bug test in a more deterministic way.

Looks good to me

Feb 7 2018, 9:01 AM

Feb 2 2018

owenpshaw updated the diff for D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.
  • adjust WriteObjectFile signature to return Status and take an std::vector
  • match project formatting style
Feb 2 2018, 11:44 AM

Feb 1 2018

owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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.

Feb 1 2018, 10:35 AM
owenpshaw added inline comments to D42195: [lldb] Generic base for testing gdb-remote behavior.
Feb 1 2018, 9:58 AM

Jan 31 2018

owenpshaw updated the diff for D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.
  • 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
Jan 31 2018, 1:39 PM
owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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

Jan 31 2018, 10:16 AM

Jan 30 2018

owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory 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.

Jan 30 2018, 12:15 PM
owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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 30 2018, 9:53 AM

Jan 29 2018

owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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?

Jan 29 2018, 6:29 PM
owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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 29 2018, 2:22 PM

Jan 25 2018

owenpshaw added a comment to D42195: [lldb] Generic base for testing gdb-remote behavior.

Thanks for the help! Someone with commit access will need to land it.

Jan 25 2018, 9:51 AM

Jan 24 2018

owenpshaw updated the diff for D42195: [lldb] Generic base for testing gdb-remote behavior.
  • 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 24 2018, 10:46 AM

Jan 22 2018

owenpshaw added a comment to D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

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

Jan 22 2018, 10:33 AM
owenpshaw updated the diff for D42195: [lldb] Generic base for testing gdb-remote behavior.

Added back socket close()

Jan 22 2018, 9:58 AM

Jan 19 2018

owenpshaw updated the diff for D42195: [lldb] Generic base for testing gdb-remote behavior.
  • 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 19 2018, 9:59 AM

Jan 18 2018

owenpshaw updated the diff for D42195: [lldb] Generic base for testing gdb-remote behavior.

Updated with suggestions

Jan 18 2018, 11:57 AM

Jan 17 2018

owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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.

Jan 17 2018, 4:29 PM
owenpshaw updated the diff for D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.
  • 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
Jan 17 2018, 1:19 PM
owenpshaw created D42195: [lldb] Generic base for testing gdb-remote behavior.
Jan 17 2018, 12:53 PM
owenpshaw added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

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 17 2018, 10:56 AM

Jan 16 2018

owenpshaw created D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.
Jan 16 2018, 4:23 PM

Jan 11 2018

owenpshaw added a comment to D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

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

Jan 11 2018, 12:02 PM
owenpshaw updated the diff for D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

Updated with latest suggestions

Jan 11 2018, 11:11 AM

Jan 10 2018

owenpshaw added inline comments to D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.
Jan 10 2018, 6:25 PM
owenpshaw updated the diff for D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

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 10 2018, 2:10 PM

Jan 9 2018

owenpshaw updated the diff for D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

Fix comparison to handle cases two segments have the same PAddr

Jan 9 2018, 5:28 PM
owenpshaw updated the diff for D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

Inlined the binary segment layout and renamed the test.

Jan 9 2018, 4:42 PM
owenpshaw updated the diff for D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

New patch offsets segments according to difference from lowest PAddr.

Jan 9 2018, 2:28 PM

Jan 8 2018

owenpshaw added a comment to D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

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.

Jan 8 2018, 6:26 PM
owenpshaw added a comment to D41745: Handle O reply packets during qRcmd.

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

Jan 8 2018, 3:55 PM
owenpshaw updated the diff for D41745: Handle O reply packets during qRcmd.

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

Jan 8 2018, 3:15 PM

Jan 4 2018

owenpshaw created D41745: Handle O reply packets during qRcmd.
Jan 4 2018, 4:32 PM

Jan 3 2018

owenpshaw added a comment to D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

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 3 2018, 3:44 PM

Jan 2 2018

owenpshaw updated the diff for D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

Updated to use std::find_if

Jan 2 2018, 3:08 PM
owenpshaw updated the diff for D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

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.

Jan 2 2018, 2:21 PM
owenpshaw added a comment to D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.

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

Jan 2 2018, 12:52 PM

Dec 28 2017

owenpshaw created D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary.
Dec 28 2017, 3:30 PM

Dec 27 2017

owenpshaw closed D41459: [lld] Fix output section offset and contents when linker script uses memory region and data commands.

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

Dec 27 2017, 8:43 AM
owenpshaw accepted D41459: [lld] Fix output section offset and contents when linker script uses memory region and data commands.
Dec 27 2017, 8:42 AM

Dec 21 2017

owenpshaw updated the diff for D41459: [lld] Fix output section offset and contents when linker script uses memory region and data commands.

Thanks for the help!

Dec 21 2017, 10:19 AM

Dec 20 2017

owenpshaw created D41459: [lld] Fix output section offset and contents when linker script uses memory region and data commands.
Dec 20 2017, 11:24 AM