RFR: 8365661: oops/resolvedMethodEntry.hpp could use default copy constructor [v3]

[Ioi Lam]

On Mon, 25 Aug 2025 09:02:49 GMT, Francesco Andreuzzi <duke at openjdk.org> wrote:

>> We may replace the non-default copy constructor and assignment with the default ones. It seems that all we have to do is a member-wise shallow copy. This would also make the class `TriviallyCopiable`.
>> 
>> This change fixes a build failure I'm getting with clang20:
>> 
>> /jdk/src/hotspot/share/oops/resolvedMethodEntry.cpp:43:12: error: first argument in call to 'memset' is a pointer to non-trivially copyable type 'ResolvedMethodEntry' [-Werror,-Wnontrivial-memcall]
>>    43 |     memset(this, 0, sizeof(*this));
>>       |            ^
>> /jdk/src/hotspot/share/oops/resolvedMethodEntry.cpp:43:12: note: explicitly cast the pointer to silence this warning
>>    43 |     memset(this, 0, sizeof(*this));
>>       |            ^
>>       |            (void*)
>> /jdk/src/hotspot/share/oops/resolvedMethodEntry.cpp:48:12: error: first argument in call to 'memset' is a pointer to non-trivially copyable type 'ResolvedMethodEntry' [-Werror,-Wnontrivial-memcall]
>>    48 |     memset(this, 0, sizeof(*this));
>>       |            ^
>> /jdk/src/hotspot/share/oops/resolvedMethodEntry.cpp:48:12: note: explicitly cast the pointer to silence this warning
>>    48 |     memset(this, 0, sizeof(*this));
>>       |            ^
>>       |            (void*)
>> 2 errors generated.
>> 
>> 
>> Testing:
>> - [x] tier1 fast-debug
>> - [x] tier2 fast-debug
>
> Francesco Andreuzzi has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'master' into resolved-default-cctor
>  - Merge branch 'master' into resolved-default-cctor
>  - use copy ctor

The problem with the default copy constructor is it might copy random values from the padding bytes:


  InstanceKlass* _field_holder; // Field holder klass
  int _field_offset; // Field offset in bytes
  u2 _field_index; // Index into field information in holder InstanceKlass
  u2 _cpool_index; // Constant pool index
  u1 _tos_state; // TOS state
  u1 _flags; // Flags: [0000|00|is_final|is_volatile]
  u1 _get_code, _put_code; // Get and Put bytecodes of the field
  // 1 padding byte on 32-bit
  // 5 padding bytes on 64-bit


This will cause failures in test/hotspot/jtreg/runtime/cds/DeterministicDump.java on certain platforms.

If you want to use the copy constructor, you need to add the padding bytes fields by hand and explicitly set them to zero in ResolvedFieldEntry::remove_unshareable_info().

I am not sure if structure padding is compiler-specific or not, so it might be difficult to write portable code.

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

PR Comment: https://git.openjdk.org/jdk/pull/26818#issuecomment-3221159221