RFR: 8357579: Compilation error: first argument in call to 'memset' is a pointer to non-trivially copyable type [v7]

[Ioi Lam]

On Thu, 6 Nov 2025 22:44:40 GMT, Jan Kratochvil <jkratochvil at openjdk.org> wrote:

>> With clang-20 using --with-toolchain-type=clang resolveFieldEntry.cpp and resolveMethodEntry.cpp break the build with similar warnings like:
>> 
>> src/hotspot/share/oops/resolvedFieldEntry.cpp:49:10: error: first argument in call to 'memset' is a pointer to non-trivially copyable type 'ResolvedFieldEntry' [-Werror,-Wnontrivial-memcall]
>>    49 |   memset(this, 0, sizeof(*this));
>>       |          ^
>> src/hotspot/share/oops/resolvedFieldEntry.cpp:49:10: note: explicitly cast the pointer to silence this warning
>>    49 |   memset(this, 0, sizeof(*this));
>>       |          ^
>>       |          (void*)
>> 
>> The patch follows the suggested fix.
>
> Jan Kratochvil has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix 32-bit compilation error

I think adding some manual paddings might be OK. I am hoping the following will work on all reasonable C++ compilers, so we don't need to hard code any size values.


STATIC_ASSERT(sizeof(ResolvedMethodEntryWithExtra) > sizeof(ResolvedMethodEntry));


In the worst case, we may have to add extra paddings for weird compilers

#ifdef _SOME_COMPILER
    u8 _more_paddings;
#endif

There should be no performance impact as the C++ compiler should be smart enough to combine the init/copy operations of the padding with the trailing "real" fields. E.g., the following can be compiled to a single 64-bit move:


    _get_code(0),
    _put_code(0)
#ifdef _LP64
    , _padding(0)
#endif


There's a trade off with my other proposal, https://github.com/openjdk/jdk/pull/28172, which is arguably more portable, but it's more verbose as you need to copy each field by hand, so it's less maintainable.

I will be fine with either approach, although if we can find a clean *and* portable solution that would be best, but we shouldn't lose our mind doing it :-)

src/hotspot/share/oops/resolvedMethodEntry.cpp line 43:

> 41: STATIC_ASSERT(sizeof(ResolvedMethodEntry) == 16);
> 42: # endif
> 43: #endif

I think this can be cleaned up as:


#ifdef _LP64
STATIC_ASSERT(sizeof(ResolvedMethodEntry) == DEBUG_ONLY(32) NOT_DEBUG(24));
#else
STATIC_ASSERT(sizeof(ResolvedMethodEntry) == DEBUG_ONLY(20) NOT_DEBUG(16));
#endif


But I think this will be better without the need to hard code numbers:


// There should be no more padding at the end of ResolvedMethodEntry
class ResolvedMethodEntryWithExtra : public ResolvedMethodEntry {
  u1 _extra_field;
};
STATIC_ASSERT(sizeof(ResolvedMethodEntryWithExtra) > sizeof(ResolvedMethodEntry));


I tested by changing `u4 _padding2` to `u4 _padding2` in `ResolvedMethodEntry` and the static assert fails.

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

PR Review: https://git.openjdk.org/jdk/pull/26098#pullrequestreview-3431620301
PR Review Comment: https://git.openjdk.org/jdk/pull/26098#discussion_r2501670635