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

8317132: Prepare HotSpot for permissive- #15955

Closed
wants to merge 18 commits into from

Conversation

TheShermanTanker
Copy link
Contributor

@TheShermanTanker TheShermanTanker commented Sep 28, 2023

Prepare HotSpot for the permissive- Visual C++ flag, this change contains all of the fixes required for HotSpot to compile under the stricter mode activated when the permissive- flag is passed

  • Reworks code in topLevelUnhandledExceptionFilter for os_windows.cpp to avoid goto jumping across uninitialized locals
  • Adds a CAST_FROM_FN_PTR cast to the return value from ::signal to void, as they cannot be implicitly converted
  • symbolengine.cpp's SimpleBufferWithFallback's templates cannot work with a raw char (Actual fix under discussion)
  • Removed a throw() specification from a mismatched definition in allocation.cpp

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

  • JDK-8317132: Prepare HotSpot for permissive- (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15955

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 28, 2023

👋 Welcome back jwaters! 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 openjdk bot added the rfr Pull request is ready for review label Sep 28, 2023
@openjdk
Copy link

openjdk bot commented Sep 28, 2023

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

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 28, 2023
@mlbridge
Copy link

mlbridge bot commented Sep 28, 2023

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Can you add information to the bug report about the actual errors caused by these coding constructs please.

Thanks

@@ -111,7 +111,7 @@ class SimpleBufferWithFallback {
_p = _fallback_buffer;
_capacity = (int)(sizeof(_fallback_buffer) / sizeof(T));
}
_p[0] = '\0';
_p[0] = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I seem to recall this being discussed in the past and it was suggested to do what is done previously and use:

_p[0] = (T) '\0';

Copy link
Member

Choose a reason for hiding this comment

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

Ah never mind. A link to:
#15096
would have been a useful reminder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, sorry for not linking that earlier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is Thomas around? Would like to get his opinion of what to do with this particular snippet

Copy link
Member

Choose a reason for hiding this comment

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

I know he was away until October but not exactly when in October.

Choose a reason for hiding this comment

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

Ick! I hadn't followed #15096 closely, so
hadn't noticed the discussion there. Not sure why this is using a literal 0
rather than (T) '\n' as in that PR?

So the literal "0" (or previously, NUL char constant) is either an integral
zero or a null pointer constant (because one of the types used for T is
HMODULE). imprint_sentinal uses (T)'X'. (Double ick! The value is either an
integral constant or a HMODULE with a weird value.)

An approach to being type safe/consistent would be to provide a traits utility
for specifying the terminator and the sentinal on a type-specific basis. Don't
know if that's worth doing in this PR. We've been living (dangerously?) with
the existing mechanism for a long time.

Something like

template<typename T>
struct SimpleBufferSpecialValues;

template<>
struct SimpleBufferSpecialValues<char> {
  char terminator() const { return '\0'; }
  char sentinal() const { return '\X'; }
};

template<>
struct SimpleBufferSpecialValues<HMODULE> {
  HMODULE terminator() const { return nullptr; }
  HMODULE sentinal() const {
    // Untested, might be completely wrong.
    alignas(HMODULE) static const char sentinal_data[1];
    return reinterpret_cast<HMODULE>(&sentinal_data);
  }
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd felt that the literal zero fit better, because it avoided an ambiguous cast and could fit in the context of a null pointer constant and char value of 0 both, casting a char constant to a pointer seemed to be something to avoid. The HMODULE or char in imprint_sentinel would always have a value of 88, which I'm not too fond of either (particularly in the case of HMODULE, which doesn't seem right since 88 is a perfectly normal value for a HMODULE to have, doesn't seem to fit the role of sentinel value too well)

Copy link
Member

@tstuefe tstuefe Nov 1, 2023

Choose a reason for hiding this comment

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

Wow, I cannot recall ever getting bashed this hard for code of mine.

I believe it was first a pure character buffer and got later adapted to hold HMODULE.

Feel free to remove the Sentinel mechanism completely. It's not needed. I would keep the zero termination of the array.

@@ -2909,20 +2909,23 @@ LONG WINAPI topLevelVectoredExceptionFilter(struct _EXCEPTION_POINTERS* exceptio

#if defined(USE_VECTORED_EXCEPTION_HANDLING)
LONG WINAPI topLevelUnhandledExceptionFilter(struct _EXCEPTION_POINTERS* exceptionInfo) {
if (InterceptOSException) goto exit;
DWORD exception_code = exceptionInfo->ExceptionRecord->ExceptionCode;
if (InterceptOSException) {
Copy link
Member

Choose a reason for hiding this comment

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

I may be missing something, but why not just use if (!InterceptOSException) here and get rid of the goto?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This decision was based on a review comment in #15096, which preferred keeping InterceptOSException without the negation. Perhaps I'll wait until Thomas sees this Pull Request

Copy link
Member

Choose a reason for hiding this comment

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

Okay. If an early bailout is desirable, I suggest instead putting everything in the else block into a helper method, and then have if (InterceptOSException) return; at the start. topLevelUnhandledExceptionFilter can then do:

helper(exceptionInfo); // or whatever name is chosen
return previousUnhandledExceptionFilter ? previousUnhandledExceptionFilter(exceptionInfo) : EXCEPTION_CONTINUE_SEARCH;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, creating an entire method exclusively for this seems a bit excessive, what if I just surrounded the else block with a scope and kept everything else the same? The issue is goto jumping across the locals of this method after all, which a scope coould solve

Copy link
Member

Choose a reason for hiding this comment

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

That seems fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not entirely sure what the issue is, since the code that goto jumps to never uses the uninitialized locals. I don't really have any other solutions to solve this issue unfortunately :/ Maybe @djelinski can add some perspective on this?

Copy link
Member

Choose a reason for hiding this comment

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

I think killing the goto makes most sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the goto, but would like to wait until Thomas is back

Copy link
Member

Choose a reason for hiding this comment

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

No response from @tstuefe but I think the empty block is pointless and we should just check if (!InterceptOSException) {.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, remove the empty block.

@TheShermanTanker
Copy link
Contributor Author

Can you add information to the bug report about the actual errors caused by these coding constructs please.

Thanks

Done

@@ -111,7 +111,7 @@ void* ArenaObj::operator new(size_t size, Arena *arena) throw() {
// AnyObj
//

void* AnyObj::operator new(size_t size, Arena *arena) throw() {
void* AnyObj::operator new(size_t size, Arena *arena) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this function guaranteed to crash in case of allocation failure? @afshin-zafari removed throw() in the header file in 0f51e63, but left the one here... should we restore the throw() in the header file instead?

Choose a reason for hiding this comment

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

[not a review, just a drive-by comment]
arena->Amalloc takes a second argument that defaults to EXIT_OOM, so this function does not return nullptr.
The earlier removal of the nothrow specification from the header was correct, but this one got missed, and
should be removed too. So the change here is correct.

Copy link
Member

@JornVernee JornVernee left a comment

Choose a reason for hiding this comment

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

os_windows.cpp changes look good

/reviewers 2

@openjdk
Copy link

openjdk bot commented Oct 5, 2023

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

8317132: Prepare HotSpot for permissive-

Reviewed-by: dholmes, jvernee, djelinski

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

  • 5207443: 8317965: TestLoadLibraryDeadlock.java fails with "Unable to load native library.: expected true, was false"
  • ee57e73: 8317612: ChoiceFormat and MessageFormat constructors call non-final public method
  • f262f06: 8319211: Regression in LoopOverNonConstantFP
  • bfaf570: 8311546: Certificate name constraints improperly validated with leading period
  • d354141: 8318694: [JVMCI] disable can_call_java in most contexts for libjvmci compiler threads
  • c86592d: 8319046: Execute tests in source/class-file order in JavadocTester
  • 3660a90: 8319139: Improve diagnosability of JavadocTester output
  • 7f47c51: 8316025: Use testUI() method of PassFailJFrame.Builder in FileChooserSymLinkTest.java
  • 36de19d: 8317048: VerifyError with unnamed pattern variable and more than one components
  • ab19348: 8318647: Serial: Refactor BlockOffsetTable
  • ... and 448 more: https://git.openjdk.org/jdk/compare/84390dd0639e29ddb792964cca9ebf79e29cfcad...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 Pull request is ready to be integrated label Oct 5, 2023
@openjdk
Copy link

openjdk bot commented Oct 5, 2023

@JornVernee
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Oct 5, 2023
@TheShermanTanker
Copy link
Contributor Author

/reviewers 2 reviewer

No one knows that the reviewers command has a second parameter ;)

@openjdk
Copy link

openjdk bot commented Oct 6, 2023

@TheShermanTanker
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

These all seem fine to me.

Thanks.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 6, 2023
@TheShermanTanker
Copy link
Contributor Author

@tstuefe What's your opinion on the removal of goto in the ARM exception filter?

@TheShermanTanker
Copy link
Contributor Author

I am going to add -permissive- temporarily to check the GHA logs for anything I have missed, I'm not going to integrate this yet. Would also still like to get @tstuefe opinion on the InterruptOSException changes...

@TheShermanTanker
Copy link
Contributor Author

TheShermanTanker commented Oct 30, 2023

Just as I suspected, the '\0' is now required to be changed to 0 to compile in the initialize() method (for some weird reason, when it was compiling fine before)

@TheShermanTanker
Copy link
Contributor Author

Sorry, re-requesting reviews from all again, as well as permission for including -permissive- for HotSpot here

@TheShermanTanker
Copy link
Contributor Author

/label build

@openjdk openjdk bot added the build build-dev@openjdk.org label Oct 30, 2023
@openjdk
Copy link

openjdk bot commented Oct 30, 2023

@TheShermanTanker
The build label was successfully added.

@TheShermanTanker
Copy link
Contributor Author

@dholmes-ora @JornVernee @djelinski There have been a couple of changes made after your reviews, please see if you are still ok with them

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

One nit but it isn't a showstopper.

@@ -2909,20 +2909,23 @@ LONG WINAPI topLevelVectoredExceptionFilter(struct _EXCEPTION_POINTERS* exceptio

#if defined(USE_VECTORED_EXCEPTION_HANDLING)
LONG WINAPI topLevelUnhandledExceptionFilter(struct _EXCEPTION_POINTERS* exceptionInfo) {
if (InterceptOSException) goto exit;
DWORD exception_code = exceptionInfo->ExceptionRecord->ExceptionCode;
if (InterceptOSException) {
Copy link
Member

Choose a reason for hiding this comment

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

No response from @tstuefe but I think the empty block is pointless and we should just check if (!InterceptOSException) {.

@TheShermanTanker
Copy link
Contributor Author

With this, the reign of non-conforming HotSpot code on Windows comes to an end :)

Will wait for the tests to pass before integrating

Comment on lines 2913 to 2915
goto result;
} else {
DWORD exceptionCode = exceptionInfo->ExceptionRecord->ExceptionCode;
Copy link
Member

Choose a reason for hiding this comment

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

No you brought the goto back! Why? goto's are bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, was experimenting different ways of doing it, and trying to change it minimally. I guess the duplicated return statement is the way to go, as Thomas suggested

Comment on lines 2912 to 2913
if (InterceptOSException) return previousUnhandledExceptionFilter ? previousUnhandledExceptionFilter(exceptionInfo) : EXCEPTION_CONTINUE_SEARCH;
DWORD exceptionCode = exceptionInfo->ExceptionRecord->ExceptionCode;
Copy link
Member

Choose a reason for hiding this comment

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

I though there was agreement to use the simple and obvious:

if (!InterceptOSException) {
...
}
return ...;

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Thank you.

@TheShermanTanker
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Nov 2, 2023

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

  • 5207443: 8317965: TestLoadLibraryDeadlock.java fails with "Unable to load native library.: expected true, was false"
  • ee57e73: 8317612: ChoiceFormat and MessageFormat constructors call non-final public method
  • f262f06: 8319211: Regression in LoopOverNonConstantFP
  • bfaf570: 8311546: Certificate name constraints improperly validated with leading period
  • d354141: 8318694: [JVMCI] disable can_call_java in most contexts for libjvmci compiler threads
  • c86592d: 8319046: Execute tests in source/class-file order in JavadocTester
  • 3660a90: 8319139: Improve diagnosability of JavadocTester output
  • 7f47c51: 8316025: Use testUI() method of PassFailJFrame.Builder in FileChooserSymLinkTest.java
  • 36de19d: 8317048: VerifyError with unnamed pattern variable and more than one components
  • ab19348: 8318647: Serial: Refactor BlockOffsetTable
  • ... and 448 more: https://git.openjdk.org/jdk/compare/84390dd0639e29ddb792964cca9ebf79e29cfcad...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 2, 2023
@openjdk openjdk bot closed this Nov 2, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 2, 2023
@openjdk
Copy link

openjdk bot commented Nov 2, 2023

@TheShermanTanker Pushed as commit 4a85f6a.

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

@TheShermanTanker TheShermanTanker deleted the hotspot branch November 2, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
6 participants