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

8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler #15096

Closed
wants to merge 113 commits into from

Conversation

TheShermanTanker
Copy link
Contributor

@TheShermanTanker TheShermanTanker commented Aug 1, 2023

We should set the -permissive- flag for the Microsoft Visual C compiler, as was requested by the now backed out JDK-8241499. Doing so makes the Visual C compiler much less accepting of ill formed code, which will improve code quality on Windows in the future.


Progress

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

Issue

  • JDK-8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler (Enhancement - P4) ⚠️ Issue is already resolved. Consider making this a "backport pull request" by setting the PR title to Backport <hash> with the hash of the original commit. See Backports.

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15096

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 1, 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 changed the title 8307160 8307160: [REDO] Enable the permissive- flag on the Microsoft Visual C compiler Aug 1, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 1, 2023
@openjdk
Copy link

openjdk bot commented Aug 1, 2023

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

  • build

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 build build-dev@openjdk.org label Aug 1, 2023
@mlbridge
Copy link

mlbridge bot commented Aug 1, 2023

Webrevs

@TheShermanTanker TheShermanTanker marked this pull request as draft August 1, 2023 02:06
@openjdk openjdk bot removed the rfr Pull request is ready for review label Aug 1, 2023
@TheShermanTanker
Copy link
Contributor Author

That broken mismatched exception specifier in allocation.cpp has been annoying the absolute **** out of me for a long time now, feels good to see it go

@TheShermanTanker
Copy link
Contributor Author

/label hotspot

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Aug 1, 2023
@openjdk
Copy link

openjdk bot commented Aug 1, 2023

@TheShermanTanker
The hotspot label was successfully added.

@@ -111,7 +111,7 @@ class SimpleBufferWithFallback {
_p = _fallback_buffer;
_capacity = (int)(sizeof(_fallback_buffer) / sizeof(T));
}
_p[0] = '\0';
_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.

I find this use of '\0' quite suspicious in itself, but it is consistent with the use of 'X' in imprint_sentinel(). I'm unclear what types are possible for T in this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are currently only 2 possible types of HMODULE and char/uint8_t for T at the moment. Weirdly enough only line 126 errors out without the cast while line 114, despite having the same problem, doesn't, but I added the cast to both lines for consistency. If someone else knows why I could probably deal with this code in a better way besides just casting it to T (which I also am reluctant to do)

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 just checked and the value of the sentinel is ultimately the prvalue 88. I don't know if we'd want to replace all the weird char usages here with explicit values of 0 (and 88 for the sentinel). Maybe future reviews can help with that

Copy link
Member

Choose a reason for hiding this comment

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

I wrote this code ages ago. I'm not sure what's weird or suspicious about it, though. The comment at the file's beginning explains this code's motivation.

The buffer was never thought to be used for something different than HANDLEs or characters, where the assignment of integer literals work. I often use char constants for sentinels as debugging aid. As for '\0', that indicates to the casual code reader that this is a termination of a string, better than had I used a plain 0.

Copy link
Member

Choose a reason for hiding this comment

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

Because there is nothing to state what T may be, I found assigning character literals to be odd. If T is char and the buffer is meant to be a C string then it makes more sense. But for non-char T it just raised questions for me.

Copy link
Member

Choose a reason for hiding this comment

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

I see that. We can remove the sentinel stuff, which leaves us with the zero termination. Arguably, this could be done by sub classes that derive from the char instantiation.

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

Please remind us what the issue is with throw() as this is a change to shared code and it looks very inconsistent to remove it only for this one definition of operator new.

Copy link
Contributor Author

@TheShermanTanker TheShermanTanker Aug 1, 2023

Choose a reason for hiding this comment

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

Hi David, this throw was removed for this method's declaration in the corresponding hpp file in an earlier commit 8305590: Remove nothrow exception specifications from operator new, but the Author forgot to remove the throw() from the definition as well (the error can be viewed in the GHA for the earlier versions of this PR). I believe gcc only errors on this in C++17 mode (for some weird reason) and not our current C++14 mode, which is why it gets a pass on other platforms

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for that.

Choose a reason for hiding this comment

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

C++17 makes exception specifications part of the type system:
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0012r1.html
Hence a mismatch between a declaration and it's definition is an error.

@TheShermanTanker
Copy link
Contributor Author

/label client security

@openjdk openjdk bot added client client-libs-dev@openjdk.org security security-dev@openjdk.org labels Aug 4, 2023
@openjdk
Copy link

openjdk bot commented Aug 4, 2023

@TheShermanTanker
The client label was successfully added.

The security label was successfully added.

@TheShermanTanker
Copy link
Contributor Author

I've switched the marked cases to use direct setting and bypass pData, should I do the unmarked ones too? @prrace

@TheShermanTanker
Copy link
Contributor Author

Bumping

@magicus
Copy link
Member

magicus commented Apr 1, 2024

Yes, fix all pData in areas touched by your changes.

@prrace
Copy link
Contributor

prrace commented Apr 2, 2024

I've been monitoring it but saw no reason to pay particular attention until everything I raised is addressed.
Including the cases identical to the ones I explicitly identified
(which I suppose is what is meant by your question "should I do the unmarked ones too ?").
At some point you do tire of typing "ditto" :-)

It has been long enough that I will need a little time to remember and re-review properly once the requested changes are in place.


AwtCanvas *c = (AwtCanvas*)pData;
c->m_eraseBackground = doErase;
c->m_eraseBackgroundOnResize = doEraseOnResize;

ret:

Copy link
Member

Choose a reason for hiding this comment

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

I recommend deleting this line, since it does not make sense to have two consecutive blank lines here.


component->GetInsets(gis->insets);

ret:

Copy link
Member

Choose a reason for hiding this comment

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

I recommend deleting this line, since it does not make sense to have two consecutive blank lines here.

return;
} else {
f = (AwtFrame *) JNI_GET_PDATA(peer);
if (f == nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

@prrace What is the current take on NULL vs nullptr in client code? I know Hotspot made an effort to completely purge all NULL references.

The new code here uses a mix of NULL and nullprt. Should it:
a) only use NULL
b) only use nullptr
c) remain as it is
?

@@ -209,7 +211,6 @@ static const double POINTS_TO_HIMETRIC = (2540.0 / 72.0);
*/
static const double POINTS_TO_LOMETRIC = (254.0 / 72.0);

jfieldID AwtPrintDialog::pageID;

Copy link
Member

Choose a reason for hiding this comment

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

I recommend deleting one more line, since otherwise you will now have three consecutive blank lines.


window->RepositionSecurityWarning(env);

ret:
Copy link
Member

Choose a reason for hiding this comment

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

I recommend deleting this line, since it does not make sense to have two consecutive blank lines here.

@@ -3137,7 +3156,7 @@ void AwtWindow::_ModalDisable(void *param)
AwtWindow::SetAndActivateModalBlocker(windowHWnd, blockerHWnd);
}

ret:

Copy link
Member

Choose a reason for hiding this comment

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

I recommend deleting this line, since it does not make sense to have two consecutive blank lines here.

@@ -3165,25 +3190,38 @@ void AwtWindow::_ModalEnable(void *param)
AwtWindow::SetModalBlocker(windowHWnd, NULL);
}

ret:

Copy link
Member

Choose a reason for hiding this comment

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

I recommend deleting this line, since it does not make sense to have two consecutive blank lines here.

AwtWindow *window = (AwtWindow *)pData;

window->SetTranslucency(iOpacity, window->isOpaque());

ret:

Copy link
Member

Choose a reason for hiding this comment

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

I recommend deleting this line, since it does not make sense to have two consecutive blank lines here.


window->SetTranslucency(window->getOpacity(), isOpaque);

ret:

Copy link
Member

Choose a reason for hiding this comment

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

I recommend deleting this line, since it does not make sense to have two consecutive blank lines here.


window->UpdateWindow(env, data, (int)uws->width, (int)uws->height,
uws->hBitmap);

ret:

Copy link
Member

Choose a reason for hiding this comment

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

I recommend deleting this line, since it does not make sense to have two consecutive blank lines here.


window->setFullScreenExclusiveModeState(state != 0);

ret:

Copy link
Member

Choose a reason for hiding this comment

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

I recommend deleting this line, since it does not make sense to have two consecutive blank lines here.

@magicus
Copy link
Member

magicus commented Apr 2, 2024

@TheShermanTanker I tried to help you get this done. I added fixes to a copy of your branch on my personal fork, but then it turned out I could not push them to your branch. :-(

It ended up with me creating a new PR, #18584. As a bonus, I think it might be easier to review with a fresh start. This PR has grown quite heavy with lots of comments and commits.

I hope you don't feel like I'm stealing this away from you. You have done a great job, and shown a lot of patience of carrying this all the way here. But I also got the impression that you would appreciate my assistance in getting the last pieces in place so we can integrate this.

@TheShermanTanker
Copy link
Contributor Author

@TheShermanTanker I tried to help you get this done. I added fixes to a copy of your branch on my personal fork, but then it turned out I could not push them to your branch. :-(

It ended up with me creating a new PR, #18584. As a bonus, I think it might be easier to review with a fresh start. This PR has grown quite heavy with lots of comments and commits.

I hope you don't feel like I'm stealing this away from you. You have done a great job, and shown a lot of patience of carrying this all the way here. But I also got the impression that you would appreciate my assistance in getting the last pieces in place so we can integrate this.

Not at all, I don't feel like you're stealing this from me. In fact, I should be the one apologising for giving you extra work! Thanks for taking this up, I once again apologise for making you do this instead, I've been very busy since Thursday (working on OpenJDK while in lectures at times), and during my breaks I've been too drained to continue, so i really appreciate your help :)

I will keep this open until the other Pull Request has been integrated, in case this might still be needed

@magicus
Copy link
Member

magicus commented Apr 3, 2024

I will keep this open until the other Pull Request has been integrated, in case this might still be needed

I don't think it is necessary. You can always re-open a PR if it should be needed, and you can look at source code and comments (and even make new comments) on a closed PR. So I think it will just make it easier for everybody if there is only a single open PR for this issue.

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed rfr Pull request is ready for review labels Apr 5, 2024
@bridgekeeper
Copy link

bridgekeeper bot commented May 1, 2024

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@magicus
Copy link
Member

magicus commented May 2, 2024

@TheShermanTanker You can close this PR. The bug was fixed with #18584 instead.

@bridgekeeper
Copy link

bridgekeeper bot commented May 30, 2024

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@TheShermanTanker TheShermanTanker deleted the patch-10 branch May 31, 2024 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org client client-libs-dev@openjdk.org merge-conflict Pull request has merge conflict with target branch
9 participants