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

[Thomas Stuefe]

On Tue, 17 Jan 2023 09:31:49 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.
>
> When you agree with "The input arg should be `const void*`...", do you mean this?
> 
> 
>   inline static const MallocHeader* resolve_checked(const void* memblock);
>   inline static       MallocHeader* resolve_checked(const void* memblock);
> 
> I don't think this is valid C++.
> 
> What I suggested is to have a single method and let the caller decide regarding `const` of the returned value.

> When you agree with "The input arg should be `const void*`...", do you mean this?
> 
> ```c++
>   inline static const MallocHeader* resolve_checked(const void* memblock);
>   inline static       MallocHeader* resolve_checked(const void* memblock);
> ```
> 
> I don't think this is valid C++.
> 
> What I suggested is to have a single method and let the caller decide regarding `const` of the returned value.

No, I did not mean that. Both functions are needed. What you suggest would lead to a violation of constness. Why drop protection? To gain what, save one line?

Having two functions that differ in constness of input arg (or of this pointer) and output argument is an established practice. The normal recommendation for non-trivial functions then is to implement one in terms of the other by casting constness away. @jdksjolen solved this in a neater, albeit more verbose way with a templatized implementation.

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

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