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

[Thomas Stuefe]

On Mon, 16 Jan 2023 23:15:19 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

> The input arg should be `const void*`, which accepts both `void*` and `const void*`, because that's a promise to the caller that the current method will not mutate the mem pointed by the input arg. 

Yes,

(Ofc, one could throw away `const` and mutate the mem anyway inside the method, but that's just bad code.)

Yes.

> 
> > but that should yield a const pointer too, not a non-const one.
> 
> I believe your reasoning is that in&out pointers are essentially the same (off by 1), so they should have the same const-ness.
> 
> However, the in&out args are two semantically different entities, `void*` vs `MallocHeader*`. For instance, a caller may construct a `MallocHeader*` out of a `const void*` -- the mem is immutable via `void*` but mutable via `MallocHeader*` due to the extra info offered by the more concrete type. IOW, whether the mem indicated by `MallocHeader*` is mutable or not is up to the caller.

Not sure if we talk at cross purposes, but:

- const variant is for safe *querying* while preventing accidental write access. You don't need write access for querying.
- non-const variant is for modifying the header. The header is mostly immutable, the only thing we change are the canaries. Without that, we could dispose of the non-const variant altogether.

The whole thing follows the same logic as a class where you differentiate between read-only and write-access for the same accessor, e.g.


class X {
   Y y;
   const Y& get_y() const;
   Y& get_y();
}


see e.g. Effective C++, Item 3. Regrettably, C++ forces us to duplicate the method.

> 
> Looking at the `const`-variant caller (I can just find this single one), it's more or less the same as a caller of the non-`const` variant.
> 
> ```c++
> static void check_expected_malloc_header(const void* payload, MEMFLAGS type, size_t size) {
>   const MallocHeader* hdr = MallocHeader::resolve_checked(payload);
>   EXPECT_EQ(hdr->size(), size);
>   EXPECT_EQ(hdr->flags(), type);
> }
> ```
> 
> vs
> 
> ```c++
>     void* const memblock = ...
> 
>     MallocHeader* header2 = MallocHeader::resolve_checked(memblock);
>     assert(header2->size() == size, "Wrong size");
>     assert(header2->flags() == flags, "Wrong flags");
> ```
> 
> (Seems to me, `header2` should be `const` as well -- here we promise not to mutate the obj via `header2`.)

Agreed.

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

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