RFR: 8286876: NMT.test_unaliged_block_address_vm_assert fails if using clang toolchain [v6]
Gerard Ziemski
gziemski at openjdk.org
Thu Jan 19 20:39:55 UTC 2023
On Tue, 13 Dec 2022 12:23:45 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 two additional commits since the last revision:
>
> - Style
> - Style
I was a bit concerned over the non-trivial change this fix turned out to be, so I investigated a little.
The smallest fix I can come up with is to change:
if (!is_aligned(this, sizeof(uint64_t))) {
to
volatile uintptr_t ptr = (uintptr_t)this;
if (!is_aligned(ptr, sizeof(uint64_t))) {
which works, since using volatile turns any compiler optimizations off, so we end up with the needed pointer (? that's how I understand it, but not 100% sure)
Another way I can see this fixed is to pass the `memblock` into `MallocHeader::assert_block_integrity` keeping it as `void*`, which we can then pass right onto `check_block_integrity`, which we need to adapt slightly to receive the pointer.
I am OK with the proposed fix, though I personally would prefer either one of the ones I presented.
-------------
Marked as reviewed by gziemski (Committer).
PR: https://git.openjdk.org/jdk/pull/11465
More information about the hotspot-runtime-dev
mailing list