This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL327970: Re-land: [lldb] Use vFlash commands when writing to
targets flash memory… (authored by labath, committed by ).
owenpshaw added a comment.
In https://reviews.llvm.org/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.
https://reviews.llvm.org/D42145
labath accepted this revision.
labath added a comment.
I like this a lot. That's the kind of change I wanted to do as a follow-up one
day. Thank you.
https://reviews.llvm.org/D42145
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
owenpshaw updated this revision to Diff 137827.
owenpshaw added a comment.
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
labath added a comment.
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
owenpshaw added a comment.
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
labath added a comment.
I thought the DoLoadInMemory function (which should probably be called
differently then) could just return a vector and the actual writing
would still happen in the LoadInMemory function.
https://reviews.llvm.org/D42145
owenpshaw updated this revision to Diff 137598.
owenpshaw added a comment.
- 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
labath added inline comments.
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3500-3518
+ // Create a list of write entries from loadable segments,
+ // using physical addresses if they aren't all null
+ size_t header_count = ParseProgramHeaders();
+ bool
owenpshaw reopened this revision.
owenpshaw added a comment.
Reopening since the previous land was reverted
https://reviews.llvm.org/D42145
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
owenpshaw updated this revision to Diff 137420.
owenpshaw added a comment.
- 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
owenpshaw added a comment.
> 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
labath added a comment.
In https://reviews.llvm.org/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
owenpshaw added a comment.
> 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
labath added inline comments.
Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:814
value = value - header->p_vaddr;
found_offset = true;
owenpshaw wrote:
> labath wrote:
> > Ok so the issue is that here we use the
owenpshaw added inline comments.
Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:814
value = value - header->p_vaddr;
found_offset = true;
labath wrote:
> Ok so the issue is that here we use the virtual address to
labath added inline comments.
Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:814
value = value - header->p_vaddr;
found_offset = true;
Ok so the issue is that here we use the virtual address to compute the load
labath added a comment.
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 0x00 0xe000 0x 0x00434 0x00434 R E 0x1000
labath added a comment.
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
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326261: [lldb] Use vFlash commands when writing to
targets flash memory regions (authored by labath, committed by ).
clayborg added a comment.
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.
https://reviews.llvm.org/D42145
___
lldb-commits mailing list
owenpshaw added a comment.
Thanks! Just a reminder that I don't have commit access, so someone will need
to land this for me. Appreciate all the help.
https://reviews.llvm.org/D42145
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
labath accepted this revision.
labath added a comment.
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.
https://reviews.llvm.org/D42145
owenpshaw added a comment.
@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.
https://reviews.llvm.org/D42145
___
lldb-commits mailing list
owenpshaw added inline comments.
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3478
+auto header = GetProgramHeaderByIndex(i);
+if (header->p_paddr != 0)
+ return true;
clayborg wrote:
> Can't p_paddr be zero and still be valid? Would
clayborg added inline comments.
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3478
+auto header = GetProgramHeaderByIndex(i);
+if (header->p_paddr != 0)
+ return true;
Can't p_paddr be zero and still be valid? Would a better test be
owenpshaw updated this revision to Diff 135087.
owenpshaw added a comment.
Herald added a subscriber: arichardson.
Update patch to include new @skipIfXmlSupportMissing decorator on test
https://reviews.llvm.org/D42145
Files:
include/lldb/Host/XML.h
include/lldb/Target/MemoryRegionInfo.h
labath added a comment.
In https://reviews.llvm.org/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
owenpshaw updated this revision to Diff 132641.
owenpshaw added a comment.
- 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
labath added a comment.
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
owenpshaw added a comment.
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
clayborg added a comment.
In https://reviews.llvm.org/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
labath added a comment.
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
Comment at:
owenpshaw updated this revision to Diff 132260.
owenpshaw added a comment.
- Remove Begin/End memory batch API
- Add Process::WriteObjectFile(llvm::ArrayRef) method. Open to
other names since it's not necessarily specific to object files, but wanted to
somehow indicate that the method may do
owenpshaw added a comment.
> 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
labath added a comment.
In https://reviews.llvm.org/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
clayborg added a comment.
In https://reviews.llvm.org/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
clayborg added a comment.
In https://reviews.llvm.org/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
owenpshaw added a comment.
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
owenpshaw added a comment.
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
clayborg added a comment.
In https://reviews.llvm.org/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
owenpshaw added a comment.
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
https://reviews.llvm.org/D42145
___
owenpshaw added a comment.
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
clayborg added a comment.
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
owenpshaw updated this revision to Diff 130258.
owenpshaw added a comment.
- 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
clayborg added inline comments.
Comment at: include/lldb/Target/Process.h:1916
+ //--
+ virtual bool BeginWriteMemoryBatch() { return true; }
+
owenpshaw wrote:
> clayborg wrote:
> > labath wrote:
owenpshaw added a comment.
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.
Comment at: include/lldb/Target/Process.h:1916
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Very nice overall. See inlined comments. Big issues are:
- GDBRemoteCommunication::WriteEscapedBinary() is not needed as
StreamGDBRemote::PutEscapedBytes() does this already
-
labath added a comment.
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
owenpshaw created this revision.
owenpshaw added reviewers: clayborg, labath.
Herald added subscribers: mgorny, emaste.
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
50 matches
Mail list logo
