Bugzilla – Full Text Bug Listing
| Summary: | The Memory Browser Find/Replace logic prefetchs data outside the specified memory range | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Randy Rohrbach <Randy.Rohrbach> | ||||||||
| Component: | cdt-memory | Assignee: | Randy Rohrbach <Randy.Rohrbach> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | Ted Williams <ted> | ||||||||
| Severity: | major | ||||||||||
| Priority: | P3 | CC: | john.cortell, Randy.Rohrbach | ||||||||
| Version: | 7.0 | Flags: | Randy.Rohrbach:
review?
(pawel.1.piech) Randy.Rohrbach: review? |
||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Randy Rohrbach
I am going to take this one. I have a patch which does a better job. Randy Created attachment 170485 [details]
Changes the Find/Replace memory fetch routine to respect memory boundaries
Changes the Find/Replace memory fetch routine to respect memory boundaries.
Also uses smarter limits so the minimal overlap in memory fetches is used.
John Please take a look at this proposed fix for this issue. I may have over though it here, so a second set of eyes would be good. The issue is pretty important though, so I would like to get this in for 7.0 if we can. Any critique you have is appreciated. I have not committed this to HEAD yet. Thanks Randy Pawel Please take a look at this as well. This is for 7.0, but is one of many fixes I would like to backport in to the 6.0.x stream for the 3.2.2 or 3.2.3 release of the Workbench product. Randy Randy, I'm having difficulties understanding the problem. It may be a terminology thing or an accidental misrepresentation in your description. Can you provide an example using specific addresses and ranges to explain what the issue is? john Sure. Memory ranges are as follows. Invalid memory 0x00000000 - 0x0001FFFF Valid memory 0x00020000 - 0x000207FF Invalid memory 0x00020800 - 0x0002FFFF We construct a search starting at 0x00020200 - 0x000203FF. The current code will try and prefetch 10K backwards and 10K forwards of the 0x00020200. This will run in to the invalid memory regions. We have this test case occuring in our test group when they are working with Linux applications with narrow image data space. Randy (In reply to comment #6) Thanks. That helped a lot. OK, so I tried a simple scenario and the search now does nothing. The following test if ( range.compareTo(searchPhraseLength) >= 0 ) { return Status.OK_STATUS; } resolves to true and no search logic is exercised. I simply open the Find/Replace Memory dialog and enter "0x1234" in the Find field "0x00020200" in Range/Start address "0x000203FF" in Range/End address John OK. Thanks. I made that quick change at the end after all the other changes and forgot to retest it. Stupid/stupid/stupid. Before it was calculating a negative range. I'll change and retest thoroughly. Let me get back to you. Randy Created attachment 170779 [details]
Updated patch which calculates the range correctly
John Here is the updated version. I tested with some pretty large arrays. Searching in various directions and search phrase sizes. I did run in to some form of memory leak when running with console tracing on, but this is a different issue from the search. Randy Some initial comments before I dig deeper... 1. Why are we doubling the pre-fetch size (line 816?). Now that we're searching in only one direction (for any given call), I don't think we should be doubling the pre-fetch amount. 2. Why is the pre-fetch amount not adjusted to take into account the number of bytes the caller wants? I understand the objective of the pre-fetch is to optimize--reduce read calls. But if the caller asks for 5K, why are we blindly taking on another 10K? We should just tack on 5K in that case. If the caller asks for 2K, we tack on 8K. Basically, the optimization should be centered around using a minimum fetch window, not just blindly tacking on a fixed additional number of bytes. 3. The check at line 864 treats 'end' as exclusive, but the fetchSize calculation at line 868 treats it as inclusive. The very first thing to do here is to make it clear in the javadoc for FindReplaceDialog.getBytesFromAddress() what the inclusive/exclusive nature of 'start' and 'end' are. This is so important in these sort of functions, but it's very often not documented, leaving readers to try to figure out the nature by looking at what the code does. The ambiguity is particular troublesome when the implementation itself is buggy, as I believe it is in this case. And for that matter, every parameter in FindReplaceDialog.getBytesFromAddress() should be documented. They are far from intuitive when you consider the entire set of parameters. 4. I believe the name of the 'currentAddress' parameter is confusing. The state of the address being the 'current' one is a state solely of the caller. It has no meaning within the callee. It's simply the address the caller wants bytes from, and as such, simply 'address' seems appropriate. Created attachment 175623 [details]
Combined patch for 315020/321407/321408
Combined patch which resolves multiple bugzilla issues.
315020
321407
321408
I checked the changes in to HEAD and 7.0.x Pawel please review Randy |