- User Since
- Apr 28 2017, 8:23 AM (64 w, 2 d)
Thu, Jul 19
@clayborg , thank you for the feedback, but why don't you like the approach with static functions?
Thank you for pointing this problem. Though ArrayRef constructor with std::array parameter is declared as constexpr, it is not a constant expression actually because std::array::data() member function is not constexpr until C++17.
So, I reverted using std::array and added constexpr keyword to each static PropertyDefinition and OptionDefinition to ensure that constant initialization requirements remain satisfied.
Tue, Jul 10
Oops! I meant only your changes when asking to use nullptr instead of NULL;)
Some code style notes
Mon, Jul 9
Upd: Making functions static with Target* parameter allowed to rid out of checking target and architecture plugin by caller code.
Fri, Jul 6
Wed, Jun 27
Uncommented arguments and used ArchSpec::IsMIPS()
I doubt that I should create plugin for thumb also. Cannot ArchitectureArm handle it?
And what about big-endian versions?
Removed unrelated changes from the patch.
Jun 22 2018
TODO: workaround for ARM.
Jun 20 2018
Jun 7 2018
Get rid of additional structure.
Update pointers only if finalized info is passed to the function.
Jun 5 2018
Jun 4 2018
Apr 13 2018
It looks ready to land. I can do it for you, if needed.
Apr 6 2018
Feb 23 2018
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
Feb 22 2018
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!