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

[Gerard Ziemski]

On Fri, 13 Jan 2023 15:38:51 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> Johan Sjölen has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Style
>>  - Style
>
> Just FYI I'm taking a look at this...

> Hi @gerard-ziemski ,
> 
> > I was a bit concerned over the non-trivial change this fix turned out to be, so I investigated a little.
> 
> thank you for your thoughts. Skimming patches down is always good.
> 
> > 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))) {
> > ```
> 
> I'd personally be fine with it but still would be UB and other compilers may complain again.
> 
> > 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.
> 
> So, make `MallocHeader::assert_block_integrity()` a static function, and pass the void* payload pointer in? That could work.
> 
> > I am OK with the proposed fix, though I personally would prefer either one of the ones I presented.
> 
> A small patch would be easier to backport and clearer with respect to what is actually the fix.
> 
> But Johans patch also has some cleanups I like:
> 
> * moving the header resolving functions out of memTracker.hpp into mallocHeader.hpp and making them static members of MallocHeader
> * the solution to the cons/nonconst duplication via templates is neat
> * -1 instead of sizeof to resolve the header
> * and other small things.
> 
> So possibly it would be fine to have a minimal patch, and then some cleanups afterwards?
> 
> I leave this up to @jdksjolen. He has put a lot of work into it already, and if he wants to be done with it and ship it that totally fine for me. I already approved the current form.

Yes, I'm OK with that as well, I just wanted to point out that we could have done this in a simpler way...

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

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