Bugzilla – Full Text Bug Listing
| Summary: | [disassembly] add IInstruction#getSize() to fill single-instruction gaps, allow large pseudo-mnemonics | ||
|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Kirk Beitz <kirk.beitz> |
| Component: | cdt-debug-dsf | Assignee: | Anton Leherbauer <aleherb+eclipse> |
| Status: | RESOLVED FIXED | QA Contact: | Pawel Piech <pawel.1.piech> |
| Severity: | normal | ||
| Priority: | P3 | CC: | aleherb+eclipse, ken.ryall, pawel.1.piech |
| Version: | 7.0.1 | Flags: | kirk.beitz:
review?
(ken.ryall) aleherb+eclipse: review+ |
| Target Milestone: | 8.0 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Attachments: | |||
Created attachment 178900 [details]
for HEAD only: add IInstruction#getSize() + deal with side-effect of increased fMeanInstructionSize
tweaked the patch a bit with one additional item to limit the fMeanInstructionSize to not get a lot larger than 16 bytes, as used to determine how much memory to retrieve per fetch.
(the algorithm gathers 100 lines for the document before it decides it has enough, and when i scrolled a number of times in a row with the range-encompassing unreadable memory recovery patch for 325284, the mean kept increasing to the point that i ended up fetching 252000 bytes of code at a time.)
+1 for instructions which know their size. The current solution is just a nasty workaround. The problem is that extending IInstruction is a breaking API change which requires a major version increase. Even if we make it a backwards compatible change, this is still not allowed on a maintenance branch, but we can workaround this by using an internal extension interface which EDCInstruction would implement. (In reply to comment #2) > The problem is that extending IInstruction is a breaking API change which > requires a major version increase. ok. can this still be committed to HEAD as is? > Even if we make it a backwards compatible change, this is still not allowed on > a maintenance branch, but we can workaround this by using an internal extension > interface which EDCInstruction would implement. in anticipation of the change not being accepted on CDT_7_0, i had been brainstorming alternatives myself. what you suggest was one. i have another that might not require anything more than a null-check in DisassemblyBackendDsf (plus the cap on fMeanInstructionSize), and thus wouldn't CDT_7_0 additions of an internal extension or changes to EDCInstruction for the maintenance branch. i'll try to create, test and then add patches for one or both alternatives later today. thanks for the review. Created attachment 178985 [details]
patch for CDT_7_0 only: add IInstructionWithSize (extends IInstruction)
also contains changes currently only on HEAD from 324507 . those changes also do not affect API, but do go together with these changes to a degree. does not include changes currently only on HEAD from 325063; would like those changes also on CDT_7_0, but they are independent of the testing of the use of this bug.
this does not obsolete the other attachment; it is the CDT_7_0 version vs the HEAD version.
let me know if this is what you had in mind.
Created attachment 179171 [details] slightly updated patch for HEAD only: IInstruction#getSize() + use in DisassemblyBackendDsf + implementation in EDCInstruction + deal with side-effect of increased fMeanInstructionSize for maintenance purposes … a) the patch for CDT_7_0 only has been moved to separate bugzilla proposal Bug 325676 so it can be reviewed separately, and if approved, committed independently from this proposal. b) the original patch (for HEAD only) has been modified slightly, and the request for review and commit on cvs HEAD is being renewed. if accepted, this patch can be applied/committed completely independently of the patch for Bug 325676. However, because Bug 325284 (which is release branch independent) is indirectly dependent upon both that bug and this one for the same reason (namely, because it fills EDCInstruction and then depends upon it telling DisassemblyBackendDsf#insertDisassembly() the proper size of its instructions in order to avoid a runtime infinite loop) … it would probably be best to accept both together … even if they are committed at slightly different times and/or closed at slightly different times due to the requirements of release cycles. Created attachment 179737 [details]
Patch avoiding API breakage
I have modified the patch such that the API breakage is avoided using an extension interface (IInstructionWithSize) similar to the solution on the maintenance branch, but this time as public API.
To make it easier to extend the instruction API in the future, clients are encouraged to extend a new abstract class AbstractInstruction instead of implementing the interface(s) directly.
Please review this solution. Thanks.
+1 this approach looks fine to me. i applied it and tried it with the edc windows debugger, and it builds and shows the disassembly view properly. thanks! Thanks, I committed the patch to HEAD. |
Created attachment 178854 [details] add IInstruction#getSize() API change -> target: CDT 7.0.2 see attached patch for new IInstruction#getSize() a problem occurs at very small gaps between 2 previously retrieved chunks of disassembled memory: there may be only a single instruction left to retrieve, but the algorithm in DisassemblyBackendDsf#insertDisassembly() determines the size of instructions by calculating the delta between the current and next instructions … meaning when there's only one instruction, the size doesn't get calculated. and when the instrSize in that algorithm is null, it exits the loop, and the gap remains unfilled. this problem occurs very rarely in scrolling, and only slightly less rarely when stepping where the boundaries of the functions being stepped into and out of are situated near each other in a way where retrieveDisassembly() causes such a small 1-instruction gap that later needs to be filled. as such, this problem has probably gone largely unseen and unrecognized when it does occur. but the problem is exacerbated in attempting to implement a solution to reduce the number of single-line "cannot read memory" pseudo-mnemonics seen in edc by taking advantage of IInstruction in EDC to create a single pseudo-mnemonic that says instead "cannot read memory for 64 bytes" for an entire requested range. by adding "Integer getSize()" to IInstruction as found in the attached patch, insertDisassembly() can be made to quickly fetch the size if pre-calculated by the backend (and to ignore it if the returned Integer is null, as demonstrated in the patch in MIInstruction, and dealt with by checking for null in insertDisassembly() to follow the code-path of the original algorithm). by having the size in hand, a gap of a single-instruction or a large pseudo-mnemonic can be filled.