RFR(XS) 8230731: SA tests fail with "Windbg Error: ReadVirtual failed"

[serguei.spitsyn at oracle.com]

Hi Chris,

LGTM++

Thanks,
Serguei


On 4/15/20 13:42, Alex Menkov wrote:
> Hi Chris,
>
> The fix looks good.
>
> --alex
>
> On 04/15/2020 10:28, Chris Plummer wrote:
>> Hello,
>>
>> Please review the following:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8230731
>> http://cr.openjdk.java.net/~cjplummer/8230731/webrev.00/index.html
>>
>> SA reads in memory from the target process as needed. The lowest 
>> level API that reads in a page of memory on windows is 
>> WindbgDebuggerLocal.readBytesFromProcess0(). A typical stack when 
>> reading in a page looks like:
>>
>>          at 
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.windbg.WindbgDebuggerLocal.readBytesFromProcess0(Native 
>> Method)
>>          at 
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.windbg.WindbgDebuggerLocal.readBytesFromProcess(WindbgDebuggerLocal.java:482) 
>>
>>          at 
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.DebuggerBase$Fetcher.fetchPage(DebuggerBase.java:80) 
>>
>>          at 
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.PageCache.getPage(PageCache.java:178) 
>>
>>          at 
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.PageCache.getData(PageCache.java:63) 
>>
>>          at 
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.DebuggerBase.readBytes(DebuggerBase.java:225) 
>>
>>          at 
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.DebuggerBase.readCInteger(DebuggerBase.java:383) 
>>
>>          at 
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.DebuggerBase.readAddressValue(DebuggerBase.java:462) 
>>
>>          at 
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.windbg.WindbgDebuggerLocal.readAddress(WindbgDebuggerLocal.java:308) 
>>
>>          at 
>> jdk.hotspot.agent/sun.jvm.hotspot.debugger.windbg.WindbgAddress.getAddressAt(WindbgAddress.java:72) 
>>
>>
>> So readBytesFromProcess() and readBytesFromProcess0() are the only 
>> platform dependent bits, but all platforms implement them. Since SA 
>> does a lot of guess work to determine the validity of whatever it is 
>> looking at, this can result in an attempt to read at an address that 
>> is not even in the process. This is suppose to result in an 
>> AddressException, and then the SA code is suppose to handle that 
>> properly (assuming it was doing something where it wasn't sure if the 
>> address was valid).
>>
>> In the above stack trace, the AddressException (actually 
>> UnmappedAddressException) is suppose to be thrown by 
>> PageCache.getData(). This is suppose happen when a null result from 
>> readBytesFromProcess0() works its way up the call chain. This is how 
>> it has been working on all platforms...except windows. It's been 
>> throwing a DebuggerException from readBytesFromProcess0() if it 
>> failed. No one up the call chain knows how to handle it, so it ends 
>> up aborting whatever SA command was being executed.
>>
>> The right thing for readBytesFromProcess0() to do if it cannot read 
>> in the page is to return null like it does on all other platforms. 
>> It's expected that sometimes an attempt to read from an invalid 
>> address will be made, and null should be returned when this happens.
>>
>> With this fix, some tests that got "ReadVirtual failed", like 
>> ClhsdbScanOops, now pass. Others fail for different reasons because 
>> they do not expect the AddressException any more than they expected 
>> the DebuggerException. 8242787 [1] is one such reason for these 
>> failures, and will be fixed next.
>>
>> Note I could not get ClhsdbPstack.java to fail, which was mentioned 
>> in the CR a few times recently. I tried many 100s of times both with 
>> and without the fix and never saw it fail. However, looking at the 
>> PStack code, it looks like it will still print the exception (now an 
>> UnmappedAddressException instead of DebuggerException), and then 
>> continue on with the next thread to priont. But since it is no longer 
>> a DebuggerException, the test should pass (there is code in 
>> ClhsdbLauncher that makes it fail if it sees a DebuggerException). 
>> This relates to the email I just sent out yesterday regarding whether 
>> or not it is acceptable that sometimes SA can't print a thread's 
>> stack trace. I think it is, and this is an example case.
>>
>> This also fixes 8001227 [2], which I will close as a dup once I push.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8242787
>> [2] https://bugs.openjdk.java.net/browse/JDK-8001227
>>
>> thanks,
>>
>> Chris
>>
>>