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

8261918: two runtime/cds/appcds/VerifierTest failed with "Unable to use shared archive" #2903

Closed

Conversation

@calvinccheung
Copy link
Member

@calvinccheung calvinccheung commented Mar 9, 2021

In the VerifierTest_*.java, we only dump the static archive once. When the -Xverify setting has changed for the dynamic archive, the static archive cannot be used due to different -Xverify settings in the following check in filemap.cpp:

  if (_has_platform_or_app_classes && 
      ((!_verify_local && BytecodeVerificationLocal) || 
       (!_verify_remote && BytecodeVerificationRemote))) { 
    FileMapInfo::fail_continue("The shared archive file was created with less restrictive " 
                  "verification setting than the current setting."); 
    return false; 
  } 

The changes in TestCommon.java is to look for the -Xverify options in the input opts.suffix. If there exists -Xverify options and "DYNAMIC_DUMP", it will dump the static archive again using the new -Xverify option(s).

This change also separates the above condition into 2; the _has_platform_or_app_classes should only be tied to BytecodeVerificationRemote.

Passed mach5 tiers 1 - 4 tests.

Thanks,
Calvin


Progress

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

Issue

  • JDK-8261918: two runtime/cds/appcds/VerifierTest failed with "Unable to use shared archive"

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2903/head:pull/2903
$ git checkout pull/2903

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Mar 9, 2021

/label add hotspot-runtime

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 9, 2021

👋 Welcome back ccheung! 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 9, 2021

@calvinccheung
The hotspot-runtime label was successfully added.

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

@mlbridge mlbridge bot commented Mar 9, 2021

Webrevs

iklam
iklam approved these changes Mar 10, 2021
Copy link
Member

@iklam iklam left a comment

LGTM.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

@calvinccheung 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:

8261918: two runtime/cds/appcds/VerifierTest failed with "Unable to use shared archive"

Reviewed-by: iklam, minqi

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 60 new commits pushed to the master branch:

  • d8a9c3c: 8263002: Remove CDS MiscCode region
  • 67ea3bd: 8263102: Expand documention of Method.isBridge
  • d0c1aec: 8263140: Japanese chars garble in console window in HSDB
  • 70342e8: 8262520: Add SA Command Line Debugger support to connect to debug server
  • e5ce97b: 8263206: assert(*error_msg != '\0') failed: Must have error_message while parsing -XX:CompileCommand=unknown
  • 3212f80: 8261937: LambdaForClassInBaseArchive: SimpleApp$$Lambda$1 missing
  • 2218e72: 8262486: Merge trivial JDWP agent changes from the loom repo to the jdk repo
  • 86fac95: 8263142: Delete unused entry points in libawt/libawt_xawt/libawt_headless
  • b7f0b3f: 8252173: Use handles instead of jobjects in modules.cpp
  • a6e34b3: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()
  • ... and 50 more: https://git.openjdk.java.net/jdk/compare/02fbcb52b8ec8d6d5d865fcf2c340cfc02cbbbba...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 10, 2021
yminqi
yminqi approved these changes Mar 10, 2021
Copy link
Contributor

@yminqi yminqi left a comment

Generally LGTM, only a few comments to clear.

@@ -44,6 +44,8 @@
"ERROR: class VerifierTestC was loaded unexpectedly";
static final String MAP_FAIL =
Copy link
Contributor

@yminqi yminqi Mar 10, 2021

MAP_FAIL no longer used. Can be removed.

Copy link
Member Author

@calvinccheung calvinccheung Mar 10, 2021

You're right. I'll remove it.

((!_verify_local && BytecodeVerificationLocal) ||
(!_verify_remote && BytecodeVerificationRemote))) {
if (_has_platform_or_app_classes
&& !_verify_remote // we didn't verify the archived platform/app classes
Copy link
Contributor

@yminqi yminqi Mar 10, 2021

Should we use "don't verify" to keep grammar consistency with above/down words? I feel confused here about the action.

Copy link
Member Author

@calvinccheung calvinccheung Mar 10, 2021

The _verify_remote is from the archive header and the archive was created before we can check its header. So using past tense here is correct.

"verification setting than the current setting.");
return false;
"verification setting than the current setting.");
// Pretend that we didn't have any archived platform/app classes, so they won't be loaded
Copy link
Contributor

@yminqi yminqi Mar 10, 2021

Same here as above.

Copy link
Member Author

@calvinccheung calvinccheung Mar 10, 2021

Similar explanation as above. The past tense was referring to archive creation.

2. move the captureVerifyOpts() call to inside the DYNAMIC_DUMP block.
@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Mar 10, 2021

@iklam, @yminqi Thanks for your review.
I've updated the webrev.

@yminqi
Copy link
Contributor

@yminqi yminqi commented Mar 10, 2021

OK, thanks for the explanation. LGTM.

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Mar 10, 2021

/integrate

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

@openjdk openjdk bot commented Mar 10, 2021

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

  • 7e52a6e: 8263380: Unintended use of Objects.nonNull in VarHandles
  • 4b5be40: 8238812: assert(false) failed: bad AD file
  • b2a2ddf: 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed"
  • c8c0234: 8262471: Fix coding style in src/java.base/share/classes/java/lang/CharacterDataPrivateUse.java
  • 4d21a45: 8262913: KlassFactory::create_from_stream should never return NULL
  • fab5676: 8247869: Change NONCOPYABLE to delete the operations
  • c0542ed: 6251901: BasicTextUI: installDefaults method are contrary to the documentation
  • fdd3941: 8263233: Update java.net and java.nio to use instanceof pattern variable
  • 3fe8a46: 8263170: ComboBoxModel documentation refers to a nonexistent type
  • d8a9c3c: 8263002: Remove CDS MiscCode region
  • ... and 59 more: https://git.openjdk.java.net/jdk/compare/02fbcb52b8ec8d6d5d865fcf2c340cfc02cbbbba...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9399e1b.

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

@calvinccheung calvinccheung deleted the 8261918-VerifierTest-failed branch Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants