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

[Albert Mingkun Yang]

On Tue, 17 Jan 2023 06:25:35 GMT, Thomas Stuefe <stuefe 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. (Ofc, one could throw away `const` and mutate the mem anyway inside the method, but that's just bad code.)
>> 
>>> 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.
>> 
>> 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.
>> 
>> 
>> 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
>> 
>> 
>>     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`.)
>
>> 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.

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

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