- User Since
- Apr 28 2017, 8:23 AM (46 w, 5 d)
Fri, Feb 23
Have no idea how to know about working around breakpoints for read and write separately. Just rely on support of Z-packets for now.
This expectation could be fair, however...
Got it. Yes, you are right. At least debugserver works around breakpoints by himself while writing memory.
I will handle this case.
May be I misunderstand your question, but I checked, that all targets of both lldb-server and debugserver read opcode from memory and save it, when enable software breakpoint.
Change ForEach return type (bool -> void)
Remove Optional from return type of FindInRange
Thu, Feb 22
Remove Guard class completely
Update according suggestions above
Use Disable|EnableBreakpointSite instead of Disable|EnableSoftwareBreakpoint.
Make BreakpointSiteList::ForEach callbacks return bool.
diff with more context
pass callbacks by const-reference
Disable breakpoints before writing memory and re-enable after.
Feb 14 2018
Sorry, =default is not applicable in these cases, of course.
Feb 9 2018
This is what I could note
Jan 23 2018
Cannot promise that I'll do it (with all tests) quickly, but I'll do.
I completely agree with your point, but why isn't enough just to return an error about breakpoints in the area user wants to write? Or to disable breakpoints forcibly?
Does this functionality really belong in the client? In case of memory reads, it's the server who patches out the breakpoint opcodes (NativeProcessProtocol::ReadMemoryWithoutTrap). I think it would make sense to do this in the same place.
Jan 22 2018
Yes, I see... There is some inconsistency between status and returned value. That is why I wondered about this https://reviews.llvm.org/D39967#984065
What about having 2 versions:
- WriteMemory(..., bool force = false), that fails if there are breakpoints in the area. Let user decide what to do.
If force == true just call DoWriteMemory without checking on breakpoints. It is needed while writing on stack, for example (and to all sections beside .text ?).
All usages should be followed with if (status.Success()).
- WriteMemorySavingBreakpoints for that cases where it is really needed. It will not return bytes_written, only status.
Jan 17 2018
Greg, Abid, if you disagree with the changes, let me know and I'll close the revision.
Jan 11 2018
Fixed comment spacing. Changed the comment slightly (words “I added this class to…” sound now like I did this, so, replaced it with passive voice).
Jan 10 2018
There is the function GetDisasmToUse in InstructionLLVMC class that can return nullptr. But this case is not handled in any usage. I suppose that caller functions cannot be invoked if !DisassemblerLLVMC::IsValid(). But it still looks dangerous for me. May be GetDisasmToUse should assert if neither m_disasm_up nor m_alternate_disasm_up exists? And may return a reference then.
Added "_up" suffix to each unique_ptr, renamed MCDisasmToolset to MCDisasmInstance.
Jan 9 2018
Added function Create that creates an instance of LLVMCDisassembler only if pass all constraints.
Moved LLVMCDisassembler declaration to .cpp file, renamed to MCDisasmToolset (is this name ok?).
Added const qualifier to some functions of the class.
I also had the courage to remove ‘_ap’ suffixes from unique pointers.
Thank you, Pavel.
Would you mind if I move LLVMCDisassembler declaration in .cpp also? It looks like perfect candidate for pimpl.
Dec 26 2017
Nov 28 2017
Now I have commit after approval access and would land this revision. May I do it now?
Nov 22 2017
When I added instructions, I didn't care about properties like isBranch, isBarrier, etc., because didn't know its purpose. But it was found that debugger cannot step over a range of instructions correctly without this knowing, thus, I've added appropriate fields to instructions.
Made diff with full paths (studying to use arcanist)...
Things that didn't require review was committed separately.
Nov 16 2017
Build for ARC fails, fix TargetInfo for this target please.
Nov 15 2017
Still have a feeling that it is not enough to clarify for function users that "if (bytes_written != bytes_to_write)" is not always criteria of error, as well as "if (error.Success())" doesn't mean that whole area was written to memory...
Same for comment in the header file.
Nov 14 2017
I have not commit access, that is why I've decided to group all typos in one patch...
Removed clang-format changes.
Sorry for wrong formatting, I've removed it.
Nov 13 2017
Nov 8 2017
Removed accidentally added file.
Use type of instruction for its fields instead of 'auto'.
I agree that matching the style in other backends is very important, but it is hard to be consistent with last, because it was written long before even c++11 was released...
Oct 12 2017
Corrupted it by merging...
Thanks for comment, Pete!
Oct 6 2017
Forgot to change decoder method name.
Sep 22 2017
Applied fixes from 2nd comment.
Added tests, fixed ARCDisassembler.cpp according suggestions.
Sep 19 2017
Hi Pete, thank you for review!
Sep 18 2017
This obvious solution works well for me and seems safe.
Jul 3 2017
Jun 30 2017
Jun 27 2017
Saying about clear intent, it would be even much more better if class name doesn't start with DWARF ;)
Jun 26 2017
Works well for me. Thank you a lot for bringing it up to scratch!
Jun 7 2017
Removed non-printable characters from DWARFCallFrameInfo::GetFDEIndex and redundant version checks from UnwindTable::Initialize.
Jun 5 2017
Jun 2 2017
Added support of DWARF <= 5. For unsupported versions GetUnwindPlan will return false and next behavior should be like without debug_frame at all. Checked it on binaries with v.2 and v.4 on x86-linux target, but have not a chance to run tests for arm-android and other. I'd appreciate it if someone, who has adjusted environment, would check it.
May 25 2017
Yes, give those binaries, please.
May 24 2017
May 23 2017
It can make sense to add
assert(eReturnStatusStarted != result);
after cmd_obj->Execute() invocation at CommandInterpreter::HandleCommand, do you think?