Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8263361: Incorrect arraycopy stub selected by C2 for SATB collectors #3008

Closed
wants to merge 2 commits into from

Conversation

neliasso
Copy link

@neliasso neliasso commented Mar 15, 2021

This bug has only been reproduced with -XX:+StressReflectiveCode and -XX:-ReduceInitialCardMarks. If StressReflectiveCode is necessary isn't obvious.

I am fixing three things:

  1. When running with ReduceInitialCardMarks enabled the stubroutines for int- or long-arraycopy will be used for arraycopies to tightly allocated arrays (like when cloning). With ReduceInitialCardMarks disabled the oop-version will be used with barriers.

The generate_arraycopy method in PhaseMacroExpand have a bool dest_uninitialized field - but that field denotes that we have proven that no zeroing is necessary (since all fields will be written anyway).

To fix this bug generate_unchecked_arraycopy with the param dest_uninitialized will need to be appended with a check for tighly coupled object-copies when ReduceInitialCardMarks is disabled.

  1. I also fix a bug in library_call kit where arraycopy for clones doesn't get marked as tightly coupled - The call to tightly_coupled_allocation can't be used when the IR isn't constructed yet.

  2. In stubroutines.cpp I fix so that the name of the arraycopy stubroutines gets appended with _uninit when appropriate. This shows up in ir-dumps and IGV.

Please review,
Nils Eliasson


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263361: Incorrect arraycopy stub selected by C2 for SATB collectors

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3008/head:pull/3008
$ git checkout pull/3008

To update a local copy of the PR:
$ git checkout pull/3008
$ git pull https://git.openjdk.java.net/jdk pull/3008/head

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 15, 2021

👋 Welcome back neliasso! 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
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@neliasso 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.

@openjdk openjdk bot added the hotspot label Mar 15, 2021
@neliasso
Copy link
Author

@neliasso neliasso commented Mar 15, 2021

/label add hotspot-compiler
/label remove hotspot

@openjdk openjdk bot added the hotspot-compiler label Mar 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@neliasso
The hotspot-compiler label was successfully added.

@openjdk openjdk bot removed the hotspot label Mar 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@neliasso
The hotspot label was successfully removed.

@neliasso neliasso marked this pull request as ready for review Mar 15, 2021
@openjdk openjdk bot added the rfr label Mar 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 15, 2021

Webrevs

@@ -661,10 +661,13 @@ Node* PhaseMacroExpand::generate_arraycopy(ArrayCopyNode *ac, AllocateArrayNode*
MergeMemNode* local_mem = MergeMemNode::make(mem);
transform_later(local_mem);

// A tightly coupled arraycopy of reference types might touch uninitialized memory
// which will make cardbarriers fail.
bool may_be_uninitialized = dest_uninitialized || (ac->is_alloc_tightly_coupled() && !ReduceInitialCardMarks && is_reference_type(basic_elem_type));
Copy link
Contributor

@fisk fisk Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if you can just adjust the way that dest_uninitialized is computed way above to unconditionally check if ac->is_alloc_tightly_coupled(), so that the value of dest_uninitialized makes sense in all cases.
Part of the reason I'm asking is that the embedded information about !ReduceInitialCardMarks really belongs to the GC, and is utilized by some but not all GCs, sometimes in not trivial ways, in BarrierSetC2::array_copy_requires_gc_barriers(), which I'm guessing is what you are referring to here. So in a way we rely on the implementation details of BarrierSetC2::array_copy_requires_gc_barriers() to compute whether we need to correctly annotate the arraycopy stub or not, regarding whether the destination is uninitialized or not. It would be nice if it was always annotated correctly, regardless of whether correct annotation is required by the GC implementation backend or not. By unconditionally performing a correct computation of dest_uninitialized, it seems like we would solve that. Or are there non-obvious reasons why that can not be done?
I guess what I'm proposing is to modify the condition where dest_uninitialized is computed, around where this hilarious comment is: "You break it, you buy it."
In fact, reading the comment just above that, it is kind of hinting at how something like is_alloc_tightly_coupled() had been used there before. It's saying that "Note: Because tightly_coupled_allocation performs checks on the out-edges of the dest, we need to avoid making derived pointers from it until we have checked its uses.", however there is no such check to be found there, which seems like the cause of the bug.

Copy link
Author

@neliasso neliasso Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bit of a conceptual disconnect in the code. In generate_arraycopy dest_uninitialized is set only when the allocation isn't already "complete" - that is we haven't already been able to prove that no zeroing is neccessary. Clones will for example create complete allocations from the beginning - but they aren't initialized.

The code that is guarded by dest_uninitialized will add zeroing for the array allocation that is outside the copy range. The part that is inside the copy range will still not be initialized. (But that is ok - since dest_uninitialized is set).

The problem here is that there are acopies that are proven to be complete from the beginning (like clones) that will copy uninitialized memory - and they don't need anything of the zeroing code that is guarded by dest_uninitialized.

Copy link
Author

@neliasso neliasso Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My plan is to rename dest_uninitialized to dest_needs_zeroing and add a separate variable for when the acopy is done to uninitilialized memory.

Copy link
Contributor

@fisk fisk Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good plan!

Node* alloc = tightly_coupled_allocation(alloc_obj, NULL);
ArrayCopyNode* ac = ArrayCopyNode::make(this, true, obj, intcon(0), alloc_obj, intcon(0), obj_length, alloc != NULL, false);
// Clones are always tightly coupled.
ArrayCopyNode* ac = ArrayCopyNode::make(this, true, obj, intcon(0), alloc_obj, intcon(0), obj_length, true, false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also fix a bug in library_call kit where arraycopy for clones doesn't get marked as tightly coupled - The call to tightly_coupled_allocation can't be used when the IR isn't constructed yet.

Can you elaborate, please, on the problem with tightly_coupled_allocation()?
If it can't be relied upon during parsing, then all other usages in LibraryCallKit have the same problem, don't they?

Copy link
Author

@neliasso neliasso Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - they might all be wrong - but it depends. In this particular case - tightly_coupled_allocation tries to look at the false path too see if it is a uncommon_trap - which is the only allowed control flow. But that part hasn't been expanded yet - so it is only a safepoint holding the parsing state.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Please, file a bug to inspect other usages of LibraryCallKit::tightly_coupled_allocation().

Copy link

@iwanowww iwanowww left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@neliasso This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8263361: Incorrect arraycopy stub selected by C2 for SATB collectors

Reviewed-by: eosterlund, vlivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 47 new commits pushed to the master branch:

  • f9f2eef: 8263434: Dangling references after MethodComparator::methods_EMCP
  • 23fc2a4: 8263688: Coordinate equals, hashCode and compareTo of JavacFileManager.PathAndContainer
  • d1baed6: 8263672: fatal error: no reachable node should have no use
  • 086a66a: 8261095: Add test for clhsdb "symbol" command
  • ec95a5c: 8263410: ListModel javadoc refers to non-existent interface
  • 7b9d256: 8261262: Kitchensink24HStress.java crashed with EXCEPTION_ACCESS_VIOLATION
  • d2144a5: 8263058: Optimize vector shift with zero shift count
  • dd6c911: 8263705: Two shenandoah tests fail due to can't find ClassFileInstaller
  • 4acb883: 8261666: [mlvm] Remove WhiteBoxHelper
  • 5069796: 8263164: assert(_base >= VectorA && _base <= VectorZ) failed: Not a Vector while calling StoreVectorNode::memory_size()
  • ... and 37 more: https://git.openjdk.java.net/jdk/compare/8afec70c283ee549795996031e3a53a3212bf35a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Mar 15, 2021
fisk
fisk approved these changes Mar 17, 2021
Copy link
Contributor

@fisk fisk left a comment

Looks good!

@neliasso
Copy link
Author

@neliasso neliasso commented Mar 17, 2021

/integrate

@openjdk openjdk bot closed this Mar 17, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 17, 2021

@neliasso Since your change was applied there have been 53 commits pushed to the master branch:

  • e152cc0: 8263677: Improve Character.isLowerCase/isUpperCase lookups
  • b63b5d4: 8263732: ProblemList serviceability/sa/ClhsdbSymbol.java on ZGC
  • 000012a: 8148937: (str) Adapt StringJoiner for Compact Strings
  • a707fcb: 8263723: [BACKOUT] MoveAndUpdateClosure::do_addr calls function with side-effects in an assert
  • 86e9cd9: 8263667: Avoid running GitHub actions on branches named pr/*
  • 41276eb: 8259863: doc: JShell snippet doesn't compile
  • f9f2eef: 8263434: Dangling references after MethodComparator::methods_EMCP
  • 23fc2a4: 8263688: Coordinate equals, hashCode and compareTo of JavacFileManager.PathAndContainer
  • d1baed6: 8263672: fatal error: no reachable node should have no use
  • 086a66a: 8261095: Add test for clhsdb "symbol" command
  • ... and 43 more: https://git.openjdk.java.net/jdk/compare/8afec70c283ee549795996031e3a53a3212bf35a...master

Your commit was automatically rebased without conflicts.

Pushed as commit 5d87a21.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler integrated
3 participants