RFR: 8286876: NMT.test_unaliged_block_address_vm_assert fails if using clang toolchain [v3]

[Thomas Stuefe]

On Thu, 8 Dec 2022 14:10:18 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`.
>
> Johan Sjölen has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
> 
>  - Comments
>  - Merge remote-tracking branch 'origin/master' into JDK-8286876
>  - Generate assert_block_integrity thru templated function
>  - Fix the bug

src/hotspot/share/services/mallocHeader.cpp line 36:

> 34: 
> 35: 
> 36: void MallocHeader::print_block_on_error(outputStream* st, address bad_address) {

Just noticed, this seems wrong. As I wrote before, print_block_on_error() should take two pointers, the header and the corruption point. The header before was implied by `this`, now that you made it a static method you need to pass the method pointer explicitly. 

That said, what was the reason again this was made static? We only call this now for cases where the header pointer is valid.

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

> 90:   address corruption = NULL;
> 91:   if (!is_valid_malloced_pointer(memblock, msg, sizeof(msg))) {
> 92:     fatal("NMT corruption: Block at " PTR_FORMAT ": %s", memblock, msg);

Wrong message. Its not a corruption, or at least unlikely. Suggestion: fatal("Not a valid malloc pointer: " ...)

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

> 95:   if (!header_pointer->check_block_integrity( msg, sizeof(msg), &corruption)) {
> 96:     if (corruption != NULL) {
> 97:       MallocHeader::print_block_on_error(tty, (address)memblock);

Please either pass header pointer explicitly, or just leave the original method non-statiic1 and call it like before via header_pointer.

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

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