RFR: JDK-8256725: Metaspace: better blocktree and binlist asserts [v3]

[Richard Reingruber]

On Sat, 21 Nov 2020 09:31:30 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi,
>> 
>> may I have reviews please for this small change. 
>> 
>> To analyze JDK-8256572, I'd like to see more information in asserts for binlist and blocktree.
>> 
>> This patch:
>> 
>> - beefs up assertion messages when verifying binlist and blocktree
>> - adds a canary to the blocktree node to detect overwriters
>> - improves the blocktree printing
>> - adds a gtest death test to test the overwrite detection.
>> 
>> Thanks!
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Quick node checks in blocktree on insert/removal

Just a few minor points. In general looks good.

src/hotspot/share/memory/metaspace/binList.hpp line 161:

> 159:       assert(b->_word_size >= word_size &&
> 160:              b->_word_size == real_word_size,
> 161:              "bad block size in list[%u] (" BLOCK_FORMAT ")", index, BLOCK_FORMAT_ARGS(b));

index is signed but you use %u in the format.

src/hotspot/share/memory/metaspace/binList.hpp line 189:

> 187:       for (Block* b = _blocks[i]; b != NULL; b = b->_next, pos++) {
> 188:         assert(b->_word_size == s,
> 189:                "bad block size in list[%u] at pos %d (" BLOCK_FORMAT ")",

index is signed but you use %u in the format.

src/hotspot/share/memory/metaspace/blockTree.cpp line 112:

> 110:       // Assume a (ridiculously large) edge limit to catch cases
> 111:       //  of badly degenerated or circular trees.
> 112:       tree_assert(info.depth < 10000, "too deep (%u)", info.depth);

info.depth is signed but in the format you use %u.

src/hotspot/share/memory/metaspace/blockTree.cpp line 56:

> 54:   p2i((n) ? (n)->_right : NULL), \
> 55:   p2i((n) ? (n)->_next : NULL), \
> 56:   ((n) ? (n)->_word_size : 0)

The null check is with one exception redundant as n is dereferenced before. Also the check does not help if n is a random invalid address. I think it would be better to print the node only if the canary check was successfull and otherwise try a safe hexdump.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1339