RFR: JDK-8304815: Use NMT for more precise hs_err location printing [v4]

[Johan Sjölen]

On Fri, 24 Mar 2023 11:40:45 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   More small fixes
>
> src/hotspot/share/services/mallocTracker.cpp line 253:
> 
>> 251:       }
>> 252:     }
>> 253:     candidate --;
> 
> I rewrote the loop, trying to do as many up front checks as possible and add more comments. It works exactly the same, but I thought this was a bit easier for a first reader to go through. If you like some or all of it, please apply it!
> 
> ```c++
> for (; candidate >= last_candidate; candidate--) {
>   if (!os::is_readable_pointer(candidate)) {
>     // Probably OOB, give up
>     return false;
>   }
>   if (!candidate->looks_valid()) {
>     // This is definitely not a header, go on to the next candidate.
>     continue;
>   }
> 
>   // fudge factor:
>   // We don't report blocks for which p is clearly outside of. That would cause us to return true and possibly prevent
>   // subsequent tests of p, see os::print_location(). But if p is just outside of the found block, this may be a
>   // narrow oob error and we'd like to know that.
>   const int fudge = 8;
>   const address start_block = (address)candidate;
>   const address start_payload = (address)(candidate + 1);
>   const address end_payload = start_payload + candidate->size();
>   const address end_payload_plus_fudge = end_payload + fudge;
>   if (!(addr >= start_block && addr < end_payload_plus_fudge)) {
>     // The address of the pointer is not within what the header recorded.
>     if (candidate->is_live()) {
>       // The candidate is live, meaning we found a header
>       // of some allocation that is not part of this pointer.
>       // Then we can give up, we're clearly reading into some other payload.
>       return false;
>     } else {
>       // The candidate is dead, so this may be a remnant of an older freed block.
>       // We continue searching.
>       continue;
>     }
>   }
> 
>   // We've found a reasonable candidate. Print the info.
>   const char* where = nullptr;
>   if (addr < start_payload) {
>     where = "into header of";
>   } else if (addr < end_payload) {
>     where = "into";
>   } else {
>     where = "just outside of";
>   }
>   st->print_cr(PTR_FORMAT " %s %s malloced block starting at " PTR_FORMAT ", size " SIZE_FORMAT ", tag %s",
>                p2i(p), where,
>                (candidate->is_dead() ? "dead" : "live"),
>                p2i(candidate + 1), // lets print the payload start, not the header
>                candidate->size(), NMTUtil::flag_to_enum_name(candidate->flags()));
>   if (MemTracker::tracking_level() == NMT_detail) {
>     NativeCallStack ncs;
>     if (candidate->get_stack(ncs)) {
>       ncs.print_on(st);
>       st->cr();
>     }
>   }
>   return true;
> }

PS. I haven't compiled it :).

> test/hotspot/gtest/nmt/test_nmt_locationprinting.cpp line 31:
> 
>> 29: #include "unittest.hpp"
>> 30: 
>> 31: //#define LOG_PLEASE
> 
> Outdated?

Or meant as a hint?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13162#discussion_r1147469499
PR Review Comment: https://git.openjdk.org/jdk/pull/13162#discussion_r1147472097