On Wed, 27 Mar 2024 05:26:09 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Undo include
>
> src/hotspot/share/utilities/debug.cpp line 680:
>
>> 678:
>> 679: // Additional "good oop" checks, separate method to not disturb
>> existing asserts.
>> 680: extern "C" bool dbg_is_good_oop_detailed(oopDesc* o) {
>
> What was the motivation for adding this? Is this copied from other HS code?
> How do we know it's doing a good enough job of verification? What happens if
> verification is not thorough enough?
On use of dbg_is_good_oop() and do we need our own dbg_is_good_oop_detailed():
The worst case is a crash if the data looks enough like an oop to pass basic
checks, but if the klass pointer or a field turns out to be a wild pointer. So
I wanted a stricter and more careful "is good oop" check, e.g. to make it
verify the klass pointer is safe, before calling is_klass(). I cannot now see
a crash when taking a valid object and examining thousands of what must be
wrong pointers in memory after it.
I kept these separate, as had seen an assert when making the existing
dbg_is_good_oop more strict. But that was an error, I had the pointer
arithmetic wrong.
This check is called by asserts in two existing places, plus the new
diagnosticCommand in this PR. With this update I'm running all of tier1 OK,
which includes ContFramePopTest.java where I could previously trigger an assert.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1551632113
