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

JDK-8267138: Stray suffix when starting gtests via GTestWrapper.java #4025

Conversation

@tstuefe
Copy link
Member

@tstuefe tstuefe commented May 14, 2021

For JDK-8185734: "[Windows] Structured Exception Catcher missing around gtest execution", I specified that --gtest_catch_exceptions=0 should be specified when starting the gtestlauncher from within the gtest jtreg wrappers. The intent was to leave signal handling to the VM - needed is this for windows 32-bit where fault handling happens via stack-based SEH, and gtestrunner's own SEH handler would intercept exceptions bubbling up from faults in test coding. For more details, please see the description of JDK-8185734.

However turned out I added a typo. It did not prevent the option from working, but should be fixed nevertheless.

This is really really trivial. Thanks for reviewing.


Progress

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

Issue

  • JDK-8267138: Stray suffix when starting gtests via GTestWrapper.java

Reviewers

Reviewing

Using git

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

Update a local copy of the PR:
$ git checkout pull/4025
$ git pull https://git.openjdk.java.net/jdk pull/4025/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4025

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4025.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 14, 2021

👋 Welcome back stuefe! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2021

@tstuefe To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels
  • 2d
  • awt
  • beans
  • build
  • compiler
  • core-libs
  • hotspot
  • hotspot-compiler
  • hotspot-gc
  • hotspot-jfr
  • hotspot-runtime
  • i18n
  • javadoc
  • jdk
  • jmx
  • kulla
  • net
  • nio
  • security
  • serviceability
  • shenandoah
  • sound
  • swing

Loading

@tstuefe tstuefe marked this pull request as ready for review May 18, 2021
@openjdk openjdk bot added the rfr label May 18, 2021
@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Jun 10, 2021

/label hotspot-runtime

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 10, 2021

@tstuefe
The hotspot-runtime label was successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 10, 2021

Webrevs

Loading

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Jun 25, 2021

Gentle ping. May I please have a review? This is really trivial. Thank you.

Loading

Copy link
Contributor

@shipilev shipilev left a comment

Yes, looks pretty trivial to me.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 25, 2021

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

8267138: Stray suffix when starting gtests via GTestWrapper.java

Reviewed-by: shade

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

  • 1d16797: 8268469: Update java.time to use switch expressions
  • ffa34ed: 8265919: RunThese30M fails "assert((!(((((JfrTraceIdBits::load(value)) & ((1 << 4) << 8)) != 0))))) failed: invariant"
  • fdcae66: 8269092: Add OldObjectSampleEvent.allocationSize field
  • fd43d9c: 8269225: JFR.stop misses the written info when the filename is only specified by JFR.start
  • 3a8f3d6: 8269280: (bf) Replace StringBuffer in *Buffer.toString()
  • c37988d: 8268276: Base64 Decoding optimization for x86 using AVX-512
  • 08ee7ae: 8268855: Cleanup name handling in the Thread class and subclasses
  • c79034e: 8269303: Remove unnecessary forward declaration of PSPromotionManager in cpCache.hpp
  • 42968db: 8269293: ObjectMonitor thread id fields should be 64 bits.
  • 2fd7943: 8256425: Obsolete Biased Locking in JDK 18
  • ... and 604 more: https://git.openjdk.java.net/jdk/compare/408608130621b340151276dceeaf52cf6d037d53...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.

Loading

@openjdk openjdk bot added the ready label Jun 25, 2021
@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Jun 25, 2021

Yes, looks pretty trivial to me.

Thanks Aleksey!

Loading

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Jun 25, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 25, 2021

Going to push as commit b565459.
Since your change was applied there have been 614 commits pushed to the master branch:

  • 1d16797: 8268469: Update java.time to use switch expressions
  • ffa34ed: 8265919: RunThese30M fails "assert((!(((((JfrTraceIdBits::load(value)) & ((1 << 4) << 8)) != 0))))) failed: invariant"
  • fdcae66: 8269092: Add OldObjectSampleEvent.allocationSize field
  • fd43d9c: 8269225: JFR.stop misses the written info when the filename is only specified by JFR.start
  • 3a8f3d6: 8269280: (bf) Replace StringBuffer in *Buffer.toString()
  • c37988d: 8268276: Base64 Decoding optimization for x86 using AVX-512
  • 08ee7ae: 8268855: Cleanup name handling in the Thread class and subclasses
  • c79034e: 8269303: Remove unnecessary forward declaration of PSPromotionManager in cpCache.hpp
  • 42968db: 8269293: ObjectMonitor thread id fields should be 64 bits.
  • 2fd7943: 8256425: Obsolete Biased Locking in JDK 18
  • ... and 604 more: https://git.openjdk.java.net/jdk/compare/408608130621b340151276dceeaf52cf6d037d53...master

Your commit was automatically rebased without conflicts.

Loading

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

@openjdk openjdk bot commented Jun 25, 2021

@tstuefe Pushed as commit b565459.

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

Loading

@shipilev
Copy link
Contributor

@shipilev shipilev commented Jun 25, 2021

One thing, shouldn't that be in JDK 17?

Loading

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Jun 26, 2021

One thing, shouldn't that be in JDK 17?

You are right. Oh well, I'll backport it once JDK17 is out. Its not such an important fix.

Loading

@tstuefe tstuefe deleted the JDK-8267138-Stray-suffix-when-starting-gtests branch Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants