RFR: 8286876: NMT.test_unaliged_block_address_vm_assert fails if using clang toolchain

[Thomas Stuefe]

On Thu, 1 Dec 2022 19:47:16 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

> This fixes a bug by removing some UB which Clang used in order to make a test fail. Specifically, Clang optimized the `is_aligned` call in ` MallocHeader::assert_block_integrity` to `true`, because an unaligned `this` is  UB. This is fixed by making a static method, essentially. The new code liberally uses `reinterpret_cast<uintptr_t>()`, this is a specifically mentioned as legal use on https://en.cppreference.com/w/cpp/language/reinterpret_cast .
> 
> **The idea is essentially:** Make sure the pointer looks OK before casting it to a `MallocHeader*`.
> 
> I also changed the definition of `malloc_header`:
> 
> ```c++
> // old
>     return (MallocHeader*)((char*)memblock - sizeof(MallocHeader));
> // new
>     return &(((MallocHeader*)memblock)[-1]);
> 
> 
> The previous definition was UB, because you shouldn't cast from a pointer with smaller alignment to one with larger alignment requirements.
> 
> I also made `MallocHeader::print_block_on_error` static, as the `this` argument was always equal to the `bad_address`.

src/hotspot/share/services/mallocHeader.hpp line 135:

> 133:   // if this is not the case.
> 134:   // Returns true if the memblock looks OK.
> 135:   inline static bool check_pointer_integrity(uintptr_t header, char* msg, size_t msglen, address* p_corruption);

This now cuts out functionality from MallocTracker::print_pointer_information(), which had been using check_block_integrity to make sure a pointer is safe to print. Now it resolves the header right away without checking the pointer.

src/hotspot/share/services/mallocHeader.hpp line 139:

> 137:   // If block is broken, print out a report to tty (optionally with
> 138:   // hex dump surrounding the broken block), then trigger a fatal error
> 139:   inline static MallocHeader* assert_block_integrity(const void* memblock);

Proposal: rename to `malloc_header_with_checks()`, and possibly move the existing `malloc_header` here?

src/hotspot/share/services/mallocHeader.inline.hpp line 88:

> 86: inline MallocHeader* MallocHeader::assert_block_integrity(const void* memblock) {
> 87:   char msg[256];
> 88:   address corruption = NULL;

Note that it is not necessary to test the supposed malloc header pointer for validity. You can also do the test on the memblock itself. Since that one must always be malloc-aligned, and the malloc header is 16 bytes, it does not matter on which pointer you do the test.

src/hotspot/share/services/mallocTracker.hpp line 322:

> 320:   static inline const MallocHeader* malloc_header(const void *memblock) {
> 321:     assert(memblock != NULL, "NULL pointer");
> 322:     return (const MallocHeader*)((const char*)memblock - sizeof(MallocHeader));

Pre-existing, but I feel like these should really live in MallocHeader, as companions to your new assert function.

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

PR: https://git.openjdk.org/jdk/pull/11465