Bugzilla – Full Text Bug Listing
| Summary: | [disassembly] view sometimes does not scroll further up, sometimes gets into infinite loop | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| 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 | Flags: | aleherb+eclipse:
review+
|
||||||
| Target Milestone: | 7.0.2 | ||||||||
| Hardware: | All | ||||||||
| OS: | SymbianOS S60 | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
Created attachment 178243 [details] Alternative fix The "insertedStartAddress" is part of a workaround for GDB's bad responses for disassembly retrieval. See also bug 302505. I would rather not change the logic such that any inserted instruction makes the method return true, because this would potentially create regressions with GDB. Instead, I would suggest to improve the check whether the start address is part of the response. In your case the start address is not aligned with the instruction which covers that address, so we just need to check whether the start address is within this instruction range. (In reply to comment #1) > Created an attachment (id=178243) [details] > Alternative fix toni, the alternate fix does not solve the problem. in the case of edc, it still leaves "........" lines. the original patch does not. > The "insertedStartAddress" is part of a workaround for GDB's bad responses for > disassembly retrieval. See also bug 302505. i'm familiar with that bug and the testing of it, as you can see from some of the alternate proposals i had put forth prior ot your final fix relying on "insertedStartAddress". > I would rather not change the logic such that any inserted instruction makes > the method return true, because this would potentially create regressions with > GDB. not to get too far off on a tangent, but ... given my testing today, i think the changing out of "insertedStartAddress" with "insertedAnyAddress" won't have anything to do with the solution for 302505 … because i don't think the "insertedStartAddress" solution solves 302505 . - in 3 minutes of testing with your alternate patch, i got DSF gdb disassembly to go into an infinite loop, then closed the view, re-opened it, and got it to do it again at a different location. - and in fact, in about the same amount of time with what's in CDT_7_0 (i.e. without any of these patches), i got about the same results. thus, perhaps 302505 needs to be re-opened independent of this bugzilla entry. but back to the point … i don't think this fix will cause any regressions that can't already be demonstrated in CDT_7_0 as is … ... but the original patch i proposed attached to this report does solve both the problem of "........" appearing in scrolling up or down, and in several days worth of testing with these changes, i've yet to see the EDC debugger go into an infinite loop again. > In your case the start address is not aligned with the > instruction which covers that address, so we just need to check whether the > start address is within this instruction range. actually, what you say is not really the problem at all. the regular case i've seen mimics the behavior below: 1) the DSF-disassembly requests a chunk of memory, e.g. 0x1001-0x1041 . 2) in the case of EDC, the guts of the content-provider within retrieveDisassembly() will looks at 0x1000 and says "we don't know if that's a valid start address, but we do know that 0xFF0 is." and so it retrieves disassembly instructions starting at 0x0FF0-0x1041 (or perhaps a little further). 3) those instructions are passed to insertDisassembly(). it may not insert addresses from 0x0FF0-0x1001 because it may have already complete AddressRangePositions containing this code. so the loop in which insertStartAddress is the return value and the decision point for which to decide to call updateVisibleArea() may never insert 0x0FF0 . but since that's the "startAddress", it's the point of comparison. it ends up always returning false. 4) the handleComplete() code then looks at that false return and says "ok, i couldn't insert mixed instructions, i'll try non-mixed. 5a) in some cases some of the same stuff will happen, with it backing up to insert instructions at about the same place, and probably at an address picked by the DSF disassembler that's off-sync with the locations of actual instructions, and it fails again, returns false again, and starts infinite looping. 5b) in other cases, it will start disassembling at a slightly different (and in some cases, wrong) location, so that it appears that it's filled most of the disassembly (even though sometimes wrong), and then will still leave "........" bits in the disassembly. by having it actually work in the cases in which some disassembly inserted between old locations that were already in the disassembly document, and perhaps also including an instruction that's not precisely aligned with the requested startAddress … it will fill the document and short-circuit the problem of re-requesting the same memory over and over … ... well, at least for EDC. so ... i'd still like to have my original patch considered to fix this problem. (and perhaps just re-open 302505 with regard to the way to avoid the infinite loop in DSF GDB … because that's obviously still there whatever happens with this patch.) (In reply to comment #2) > 3) those instructions are passed to insertDisassembly(). it may not insert > addresses from 0x0FF0-0x1001 because it may have already complete > AddressRangePositions containing this code. so the loop in which > insertStartAddress is the return value and the decision point for which to > decide to call updateVisibleArea() may never insert 0x0FF0 . but since that's > the "startAddress", it's the point of comparison. it ends up always returning > false. This is not true. The "startAddress" is always the requested address, not the address of the first instruction returned from the disassembler. > so ... i'd still like to have my original patch considered to fix this problem. Sure, your patch seems to work with the example in 302505 and I don't have other examples at hand, so this might be as good a workaround as the original or even better. I just don't know. Where I do see a problem with this approach is: Assume we request disassembly for 0x1001 - 0x1041, the response is for 0x2000 - 0x2040, ie. there is not even an overlap between requested and returned range. In this case the new logic would return "true" if at least one of the instructions got inserted, but the requested range is still not available and it will be requested again and again through the updateVisibleArea(). That's why I chose the insertedStartAddress as the criterion whether a request was successful or not. I'd rather avoid all those heuristics, workarounds and fallbacks altogether, but that's all for GDB's sake. I assume EDC behaves much better than GDB and all this convoluted logic causes more problems than it solves. So, all in all, I am OK to commit the patch on HEAD. If you need it for 7.0.1, I would not veto against it, but I won't commit it myself. > (and perhaps just re-open 302505 with regard to the way to avoid the infinite > loop in DSF GDB … because that's obviously still there whatever happens with > this patch.) Could you provide a test case where you still see an infinite loop with DSF-GDB and create a new bug for it? I committed your patch to HEAD. (In reply to comment #3) > So, all in all, I am OK to commit the patch on HEAD. If you need it for 7.0.1, > I would not veto against it, but I won't commit it myself. I'm proposing that once the CDT 7.0.1 release is out the door I will commit it to the CDT_7_0 branch. Our team will commit to testing it with DSF-GDB to make sure to won't cause any problems in CDT 7.0.2. (In reply to comment #5) > I'm proposing that once the CDT 7.0.1 release is out the door I will commit it > to the CDT_7_0 branch. Our team will commit to testing it with DSF-GDB to make > sure to won't cause any problems in CDT 7.0.2. The fix has already been committed to cdt_7_0 as part of bug 325676. |
Created attachment 178210 [details] DisassemblyBackendDsf.java patch to change insertDisassembly() local var from insertedStartAddress to insertAnyAddress (patch works against HEAD 1.11 or CDT_7_0 1.8.2.3) the attached patch (for review) solves the following problem ... there are times when the disassembly view gets into one of two situations (hard to reproduce) 1) the top line will be "........" with no disassembly, and the view will not scroll further. debugging this shows the proper disassembly is passed to insertDisassembly(...IMixedInstruction...), but the values do not repopulate 2) after having scrolled around a bit, near one end or the other, the view will peg the CPU at 100% … under the hood, this appears caused by an infinite recursion whereby it goes through insertDisassembly() then updateVisibleArea() then retrieveDisassembly() and back through this set of calls once again. (stack is not exhausted because different jobs get these operations and the results processing.) i have attached a patch that appears to fix both of these problems for use by edc ARM. it may also solve similar problems for gdb or other disassembly code. the point of the patch is to track whether *any* disassembly has been added to the document, not just whether the start-address was added. as addressed in bugzilla 321162, variable length instruction architectures (such as x86 & ARM) that use edc (which attempts to be smart about starting a variable length disassembly by searching symbols for a known good starting point using the line-entry tables) can sometimes back-up the startAddress as requested by retrieveDisassembly(). this means at times, the requested "startAddress" passed to insertDisassembly() could be within the bounds of a single instruction (i.e. request was at 0x1001, instruction is two bytes from 0x1000 to 0x1002). in such a case, the variable insertedStartAddress will never be true, even though all of the mixed mode disassembly was properly inserted into the document. this can cause the recursion case, which can make it seem like the user can no longer scroll, or cause a user's CPU to peg at 100% and bog down the machine. by using "insertedAnyAddress" instead of "insertedStartAddress", and setting it to true if the point is ever reached where even a single instruction is added to the document, it solves the problem of returning false and causing the handleCompleted() function of the DRM passed to disassembly.getMixedInstructions(), as it does now. my testing with this patch seems to show that this solves the problem we are seeing. from my reading of what insertStartAddress was used for, i'm hoping it is in keeping with what the algorithm in insertDisassembly() is otherwise trying to do.