8365661: oops/resolvedMethodEntry.hpp could use default copy constructor

[fandreuz]

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:

• tier1 fast-debug
• tier2 fast-debug

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8365661: oops/resolvedMethodEntry.hpp could use default copy constructor (Enhancement - P4) ⚠️ Issue is not open.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26818/head:pull/26818
$ git checkout pull/26818

Update a local copy of the PR:
$ git checkout pull/26818
$ git pull https://git.openjdk.org/jdk.git pull/26818/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26818

View PR using the GUI difftool:
$ git pr show -t 26818

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26818.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back fandreuz! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@fandreuz The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (2b51d610)
• 01: Full - Incremental (ba555768)
• 00: Full (c79f43ee)

[fandreuz]

Test failure seems unrelated: https://github.com/fandreuz/jdk/actions/runs/17035211708/job/48288257455#step:10:1581

[fandreuz]

Hi @kimbarrett, could you have a look at this PR? I think the solution is reasonable, removes some possibly unnecessary code, and most importantly fixes a build error which occurs on recent clang versions.

[fandreuz]

Just realized this is a dup of JDK-8357579, for which there's an apparently inactive PR: #26098.

[iklam]

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.

[iklam]

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.

I am sorry, the problem is not with ResolvedFieldEntry::remove_unshareable_info(), but rather the ResolvedFieldEntries that are not cleaned by this function.

jdk/src/hotspot/share/oops/cpCache.cpp

Lines 433 to 438 in 0f7c0e9

	if (resolved && AOTConstantPoolResolver::is_resolution_deterministic(src_cp, cp_index)) {
	rfi->mark_and_relocate();
	archived = true;
	} else {
	rfi->remove_unshareable_info();
	}

    if (resolved && AOTConstantPoolResolver::is_resolution_deterministic(src_cp, cp_index)) {
      rfi->mark_and_relocate();  /// <--- HERE
      archived = true;
    } else {
      rfi->remove_unshareable_info();
    }

Note that rfi is allocated from within the metaspace, so originally it contains all zeros. However, sometimes we read a ResolvedFieldEntry from a GrowableArray and store it inside *rfi. If we use the default copy constructor, the copy inside the GrowableArray may have junk in its gaps, and the junk will leak into *rfi.

If I remember correctly, this problems is most prominent on Windows. It's probably because Windows likes to use aligned copies to move stack variables into the GrowableArray, taking whatever junk from the stack.

[iklam]

I created a PR to add comments about the need for copy_from(), etc
#26946

[openjdk]

@fandreuz this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout resolved-default-cctor
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push