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

[Thomas Stuefe]

On Mon, 5 Dec 2022 13:16:37 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 incrementally with one additional commit since the last revision:
> 
>   Generate assert_block_integrity thru templated function

Hi @jdksjolen,

I tried to reproduce it with clang 10 and 12 (have no time to install 14), but no such luck. Your tiny reproducer (nice, btw) generates the correct alignment code even for -O3:


  7 0000000000000000 <_ZN1A1fEv>:
  8    0:   40 f6 c7 07             test   dil,0x7
  9    4:   0f 94 c0                sete   al
 10    7:   c3                      ret


But your repro output is very clear, I don't doubt it.

I still feel this is a general problem though. We use `is_aligned()` to check alignment. This is not an operation that should be optimized away. 

Clang optimizing alignment checks away should hit not only `is_aligned(this)`, but all `is_aligned(xxx)` calls in hotspot with xxx being a pointer to a data structure with pointer-sized members. 

Hotspot rolls its own memory management in many places. So we use placement new a lot: metaspace, hotspot arenas, java heap. When we do, we could err. If we err, we want to know. So alignment tests should always work. `is_aligned` not working is like a smoke detector not working because fire is not allowed.



----

Your patch is mostly fine, but I would like some small reshuffling and renaming. In the end, I'd like:

1) a  public API that checks a *malloced* pointer (the payload pointer) for validity e.g.:


static bool MallocHeader::is_valid_malloc_pointer(const void* payload, char* msg, size_t msglen) {
  // range check > 1K
  // (future RFE?) validity checks if we know the range of valid virtual addresse
  // alignment check
  // (future RFE?) possibly, SafeFetch check
}


Notes:
	- we don't need to know anything about MallocHeader at this point. If a payload pointer is correctly malloc-aligned, its MallocHeader will be correctly aligned as well.
	- there is no reason for this API to return a corruption address. And there is no reason to hex-dump the block either since we are sure the address cannot point to a valid block. This is usually a case of something misinterpreted as a pointer, and printing the block at that location is usually just misleading.
		
2) A public API that checks a MallocHeader* for validity, minus the pointer tests. We already have this: your variant of MallocHeader::check_block_integrity().

4) A compound operation like your new `assert_block_integrity()`, that does all checks, asserts, and resolves the header. I like clearly named functions though and call it something like `malloc_header_with_checks` or `resolve_checked` (since it now does more than just asserting):


static MallocHeader* MallocHeader::resolve_checked(void* payload) {
	if (!MallocHeader::is_valid_malloc_pointer(payload, msg, sizeof(msg)) {
	  fatal("NMT corruption: Block at " PTR_FORMAT ": %s", header, msg);
	}
	MallocHeader* hdr = MemTracker::malloc_header(payload);
	if (!p->check_block_integrity(msg, sizeof(msg), &corruption)) {
		// print block, fatal out
	}
	return hdr;
}


This is almost what you already have, but a bit more versatile:
- Majority of cases we use your new compound API to resolve a payload pointer to a header, with all checks and nice error printouts.
- Where we really want to do checks ourselves - eg in debug.cpp, in pp() - we have the building blocks to do so (is_valid_malloc_pointer and check_block_integrity). We can then do our own error reporting.

What do you think?

Cheers Thomas

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

> 110: inline const MallocHeader* MallocHeader::assert_block_integrity(const void* memblock) {
> 111:   return MallocHeader::assert_block_integrity_internal<const void*, const MallocHeader*>(memblock);
> 112: }

I usually just const_cast to implement one in terms of the other.

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

> 316:   static inline MallocHeader* malloc_header(void *memblock) {
> 317:     assert(memblock != NULL, "NULL pointer");
> 318:     return &(((MallocHeader*)memblock)[-1]);

Why not just ```return (MallocHeader*)memblock - 1; ``` ?

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

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