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`.

Hi Johan,

> 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.

Is it allowed to do that? This surprises me. We use is_aligned(this) in other places too, e.g. in oopStorage. So maybe this needs a more general solution. @kimbarrett?

> 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*`.

Hm. I kind of like to have these as separate discrete operations. I have to think a bit.

> 
> I also changed the definition of `malloc_header`:
> 
> ```c++
> // old
>     return (MallocHeader*)((char*)memblock - sizeof(MallocHeader));
> // new
>     return &(((MallocHeader*)memblock)[-1]);
> ```
> 

okay

> 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`.

It is, but that looks like a regression. It was not the original idea. `this` points to the start of the block, `bad_address` was supposed to point out the point of corruption, which could also be the footer. And in theory also different parts of the header (e.g. for 32-bit, where you have two header canaries, the canary address). There is even logic in print_block_on_error to hexdump two disjunct areas in case the point of corruption is farther away than what one hex dump would cover.

See the explanation, code, and example in the original PR description: https://github.com/openjdk/jdk/pull/5952. 

I see this regressed with "JDK-8292071: NMT: move MallocHeader to its own header and inline header checks". And I managed to break it myself apparently :-(. But the original variant was much nicer and produced better output.

My preference would be to keep handing in two pointers, header address and corruption point, and to re-instate the old behavior. MallocHeader::check_block_integrity already returns the corruption point. The fix is very simple:


--- a/src/hotspot/share/services/mallocHeader.inline.hpp
+++ b/src/hotspot/share/services/mallocHeader.inline.hpp
@@ -55,9 +55,7 @@ inline void MallocHeader::assert_block_integrity() const {
   char msg[256];
   address corruption = NULL;
   if (!check_block_integrity(msg, sizeof(msg), &corruption)) {
-    if (corruption != NULL) {
-      print_block_on_error(tty, (address)this);
-    }
+    print_block_on_error(tty, corruption != nullptr ? corruption : (address)this);
     fatal("NMT corruption: Block at " PTR_FORMAT ": %s", p2i(this), msg);
   }
 }

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

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