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-8254189: Improve comments for StackOverFlow and fix in_xxx() functions #795

Closed
wants to merge 4 commits into from

Conversation

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Oct 22, 2020

Hi,

may I please have reviews for this small cleanup / fix?

While reviewing JDK-8253717 it was found that comments would help with understanding the StackOverFlow class. Especially the fact that the various base are actually pointing outside their respective zone, since the stack grows downward and a zone (and the stack itself) range is [end, base). If you don't look at this code daily it can be surprising.

This also fixes some small off-by-one errors in various "in_stack_xxx_zone()" methods which test whether a given address is inside a zone and gave wrong results for address=base since base points outside its zone. This had the effect that an address could be in multiple zones.

Finally it adds a small gtest which tests the StackOverFlow methods.

/issue: JDK-8254189


Progress

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

Testing

Linux x32 Linux x64 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) (1/9 failed) ✔️ (9/9 passed)

Failed test task

Issue

  • JDK-8254189: Improve comments for StackOverFlow and fix in_xxx() functions

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 22, 2020

👋 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 Oct 22, 2020

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

  • hotspot-runtime

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.

Loading

@tstuefe tstuefe marked this pull request as ready for review Oct 26, 2020
@openjdk openjdk bot added the rfr label Oct 26, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 26, 2020

Webrevs

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Thomas,

Seems okay.

One nit below.

Thanks,
David

Loading

src/hotspot/share/runtime/stackOverflow.cpp Outdated Show resolved Hide resolved
Loading
@openjdk
Copy link

@openjdk openjdk bot commented Oct 27, 2020

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

8254189: Improve comments for StackOverFlow and fix in_xxx() functions

Reviewed-by: dholmes, gziemski

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

  • caec8d2: 8233560: [TESTBUG] ToolTipManager/Test6256140.java is failing on macos
  • a5b42ec: 8233570: [TESTBUG] HTMLEditorKit test bug5043626.java is failing on macos
  • 7e305ad: 8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe manner
  • d82a6dc: 8255438: [Vector API] More instructs in x86.ad should use legacy mode for code-gen
  • 1a5e6c9: 8253101: Clean up CallStaticJavaNode EA flags
  • a7595b2: 8250669: Running JMH micros is broken after JDK-8248135
  • edd1988: 8255530: Additional cleanup after JDK-8235710 (elliptic curve removal)
  • 790d6e2: 8255533: Incorrect javadoc in DateTimeFormatterBuilder.appendPattern() for 'uu'/'yy'
  • 3f20612: 8255555: Bad copyright headers in SocketChannelCompare.java SocketChannelConnectionSetup.java UnixSocketChannelReadWrite.java
  • 42fc158: 8253939: [TESTBUG] Increase coverage of the cgroups detection code
  • ... and 115 more: https://git.openjdk.java.net/jdk/compare/da97ab5c8a4f6d54c322bed133d5dc551bdd2de1...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 Oct 27, 2020
@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Oct 28, 2020

Ping... May I have a second review please? Its quite trivial.
Patch was tested in our internal systems and no problems surfaced.

Loading

@gerard-ziemski
Copy link

@gerard-ziemski gerard-ziemski commented Oct 28, 2020

Very nice.

Personally I'd like to see additional:

ASSERT_TRUE(so.in_stack_red_zone(so.stack_end()));
ASSERT_TRUE(so.in_stack_yellow_zone(so.stack_red_zone_base()));
ASSERT_TRUE(so.in_stack_reserved_zone(so.stack_yellow_zone_base()));

in the test, to drive home the point of the inclusion of the ranges, but that's a tiny nick pick.

Loading

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Oct 28, 2020

Very nice.

Thank you, Gerard.

Personally I'd like to see additional:

ASSERT_TRUE(so.in_stack_red_zone(so.stack_end()));
ASSERT_TRUE(so.in_stack_yellow_zone(so.stack_red_zone_base()));
ASSERT_TRUE(so.in_stack_reserved_zone(so.stack_yellow_zone_base()));

in the test, to drive home the point of the inclusion of the ranges, but that's a tiny nick pick.

I can do this. Will have to reshape those a bit since there is no "in_yellow_zone" (I considered adding one for tests sake but then did not).

...Thomas

Loading

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Oct 28, 2020

... and StackOverFlow::stack_end() is private, so I cannot use it in ASSERT either...

Loading

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Oct 29, 2020

/integrate

Loading

@openjdk openjdk bot closed this Oct 29, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 29, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 29, 2020

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

  • caec8d2: 8233560: [TESTBUG] ToolTipManager/Test6256140.java is failing on macos
  • a5b42ec: 8233570: [TESTBUG] HTMLEditorKit test bug5043626.java is failing on macos
  • 7e305ad: 8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe manner
  • d82a6dc: 8255438: [Vector API] More instructs in x86.ad should use legacy mode for code-gen
  • 1a5e6c9: 8253101: Clean up CallStaticJavaNode EA flags
  • a7595b2: 8250669: Running JMH micros is broken after JDK-8248135
  • edd1988: 8255530: Additional cleanup after JDK-8235710 (elliptic curve removal)
  • 790d6e2: 8255533: Incorrect javadoc in DateTimeFormatterBuilder.appendPattern() for 'uu'/'yy'
  • 3f20612: 8255555: Bad copyright headers in SocketChannelCompare.java SocketChannelConnectionSetup.java UnixSocketChannelReadWrite.java
  • 42fc158: 8253939: [TESTBUG] Increase coverage of the cgroups detection code
  • ... and 115 more: https://git.openjdk.java.net/jdk/compare/da97ab5c8a4f6d54c322bed133d5dc551bdd2de1...master

Your commit was automatically rebased without conflicts.

Pushed as commit 4031cb4.

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

Loading

@coleenp
Copy link
Contributor

@coleenp coleenp commented Oct 29, 2020

Sorry for not getting to this review on time. Thank you for doing this and writing the gtest. This is really nice.

Loading

@tstuefe tstuefe deleted the JDK-8254189 branch Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants