Community
Participate
Working Groups
The Memory Browser Find/Replace logic prefetchs data outside the specified memory range. When the user specifies a memory range for a FIND/REPLACE operation. The search logic tries to fetch before and after the current search location. Which is OK, although there are inefficiencies I will discuss below. The issue is the prefetch sizes used to not test to insure they are within the specified range of the search. The assumption is that when the user specifies a memory range they have some concept of valid lower and upper limits. If it turns out they are wrong then an appropriate error should be ( and is generated ). But if the user specifies a valid range which is good but resides next to an invalid region of memory, the prefetch below the current address or above depending on where the invalid memory is will cause error messages to occur and the valid search will fail. Also the prefetch is very inefficient. It wants to go backward 10*1024 bytes and go forward 10*1024. When the next block is to be searched it will go back 10*1024 in to space we have already fetched and searched. It should not depend on the caching of the Memory Service to be efficient. Smarter requests on prefetch should be used, enough to insure that boundary comparison is ensured, but little more is needed.
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
