8308780: Fix the Java Integer types on Windows #14125

TheShermanTanker · 2023-05-24T13:56:05Z

On Windows, the basic Java Integer types are defined as long and __int64 respectively. In particular, the former is rather problematic since it breaks compilation as the Visual C++ becomes stricter and more compliant with every release, which means the way Windows code treats long as a typedef for int is no longer correct, especially with -permissive- enabled. Instead of changing every piece of broken code to match the jint = long typedef, which is far too time consuming, we can instead change jint to an int (which is still the same 32 bit number type as long), as there are far fewer problems caused by this definition. It's better to get this over and done with sooner than later when a future version of Visual C++ finally starts to break on existing code

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 1 Reviewer, 1 Author)

Issue

JDK-8308780: Fix the Java Integer types on Windows (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 80b6f787
Daniel Jeliński (@djelinski - Reviewer) ⚠️ Review applies to 84f8e08c
Alexey Ivanov (@aivanov-jdk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14125

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-24T13:57:54Z

👋 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 · 2023-05-24T14:00:33Z

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

client
core-libs
hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-05-24T14:04:02Z

Webrevs

dholmes-ora

I think the JNI type definition change is okay.

However many of the other changes appear to me to not involve Java variables and so don't need to be Java types i.e they should be int rather than jint - though as these are native Windows types there may not actually be any reason to change them from long. This is for the client-libs folk to decide.

dholmes-ora · 2023-05-25T01:28:57Z

src/java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.cpp

@@ -322,7 +322,7 @@ Java_sun_java2d_windows_GDIRenderer_doDrawArc
        return;
    }

-    long sx, sy, ex, ey;
+    jint sx, sy, ex, ey;


These are not Java variables. They get passed to the win32 GDI Arc function below which expects int.

src/java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.cpp

TheShermanTanker · 2023-05-25T07:22:50Z

I think the JNI type definition change is okay.

However many of the other changes appear to me to not involve Java variables and so don't need to be Java types i.e they should be int rather than jint - though as these are native Windows types there may not actually be any reason to change them from long. This is for the client-libs folk to decide.

All the changes from long were done since there was conversion from or to a jint somewhere down the line, and the compilation would fail if not done otherwise. I also changed them to jint rather than int so there wouldn't be a need to keep the variables in sync with the jni.h declarations, but I guess I'll wait for more reviews to see what to do here

TheShermanTanker · 2023-05-25T07:26:07Z

Going to page for @aivanov-jdk for client-libs review

dholmes-ora · 2023-05-25T07:31:31Z

All the changes from long were done since there was conversion from or to a jint somewhere down the line,

Okay I see that now. It is a messy situation - at some point the incoming jint's need to be "converted" to a native type.

TheShermanTanker · 2023-05-25T08:49:31Z

I'll see what I can do, I'll check the parameter type for the methods that are called in relevant code, though if they take an int as an argument I'm not really sure what to change

TheShermanTanker · 2023-05-25T09:23:27Z

For reference as to what types the calls in affected code accepts as parameters, so any future reviews don't need to dig through the code

GDIRenderer.cpp:

- Java_sun_java2d_windows_GDIRenderer_doFillArc
  * AngleToCoord takes jints as arguments
  * ::Pie takes ints as arguments

- Java_sun_java2d_windows_GDIRenderer_doDrawArc
  * AngleToCoord takes jints as arguments
  * ::Arc takes ints as arguments

GDIWindowSurfaceData.cpp:

- GDIWinSD_GetRasInfo
  * SurfaceDataRasInfo's lutBase field is a jint*

awt_MenuBar.cpp (Encompasses the changes in awt_Menu.h and awt_MenuBar.h as well):

- AwtMenuBar::GetItem passes the only relevant jint (formerly long) into env->CallObjectMethod()

ShellFolder2.cpp

* colorBits is passed to both SetIntArrayRegion (takes a jint pointer) and GetDIBits (takes a void pointer)

AccessInfo.cpp

- getAccessibleInfo
  * start and end (both formerly long) are both passed to GetAccessibleTextLineBounds and GetAccessibleTextRange which take jints

Only outlier is jaccesswalker, which I think I may have edited wrongly

TheShermanTanker · 2023-05-31T07:05:44Z

Bumping

:(

…site

TheShermanTanker · 2023-06-01T11:55:52Z

While looking through the code for this I've come to realize that a staggering amount of code in the accessibility binaries specify longs where unsigned longs would be much more appropriate (see the one example in this PR for instance), wonder if this should also be fixed in the long term too

prrace · 2023-06-01T17:43:02Z

I'm not sure I understand the logic here.
I would not want to move to using Java typedefs in places where the windows APIs specify the types they are expecting.
If something comes in from a JNI down-call we should convert it to the type expected by Windows using the name expected by Windows.

Also "compilation" isn't nearly good enough. How is this being tested ?

TheShermanTanker · 2023-06-02T11:52:24Z

I'm not sure I understand the logic here. I would not want to move to using Java typedefs in places where the windows APIs specify the types they are expecting. If something comes in from a JNI down-call we should convert it to the type expected by Windows using the name expected by Windows.

I can change the jints in this PR to regular ints if required. As listed above, the native Windows API routines that the java.desktop code calls are actually expecting ints, so our existing declarations of passing longs to them are also wrong regardless, even without the Java typedefs

Actually, now that I revisit this issue (shown in the list above), the only actual calls in this change that don't take Java typedefs are the calls to ::Arc and ::Pie, so this is less of a problem than initially expected

Also "compilation" isn't nearly good enough. How is this being tested?

-permissive-

TheShermanTanker · 2023-06-06T15:33:59Z

Bumping

TheShermanTanker · 2023-06-08T07:42:58Z

Anyone?

aivanov-jdk · 2023-06-08T11:20:05Z

I'll take a look… hopefully next week.

TheShermanTanker · 2023-06-15T06:01:52Z

Bumping

src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp

TheShermanTanker · 2023-06-20T07:11:24Z

Bumping

mlbridge · 2023-06-21T06:55:52Z

Mailing list message from Patrick Chen on client-libs-dev:

No rejected

Le mer. 21 juin 2023, 08:47, Julian Waters <jwaters at openjdk.org> a ?crit :

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/client-libs-dev/attachments/20230621/9855c0aa/attachment.htm>

TheShermanTanker · 2023-06-21T08:19:18Z

...

src/jdk.accessibility/windows/native/jaccesswalker/jaccesswalker.cpp

TheShermanTanker · 2023-06-22T15:09:06Z

Will do, thanks Daniel
@prrace @dholmes-ora Are both of you happy with the changes?

aivanov-jdk

Looks fine to me, except for a few comments.

The size of the types for jint and jlong remains the same after amending the typedef jni_md.h. Yet I'm still cautious about it. You should have an approval from hotspot.

Please also update the copyright year in the modified files.

src/java.desktop/windows/native/libawt/java2d/windows/GDIRenderer.cpp

src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp

src/java.desktop/windows/native/libawt/windows/awt_MenuBar.cpp

dholmes-ora

Hotspot changes still approved.

Other changes seem okay to me.

Thanks

TheShermanTanker · 2023-06-23T02:09:41Z

Alright, I'll integrate once Alexsey approve, anyone else has further objections?

djelinski · 2023-06-23T06:10:05Z

src/java.desktop/windows/native/libawt/windows/awt_Menu.h

@@ -73,7 +73,7 @@ class AwtMenu : public AwtMenuItem {

    /*for multifont menu */
    BOOL IsTopMenu();
-    virtual AwtMenuItem* GetItem(jobject target, long index);
+    virtual AwtMenuItem* GetItem(jobject target, int index);


Hi @aivanov-jdk are you OK leaving this inconsistent with the definition?

jdk/src/java.desktop/windows/native/libawt/windows/awt_Menu.cpp

Line 261 in 16b5a91

AwtMenuItem* AwtMenu::GetItem(jobject target, jint index)

The declaration and implementation have to match.

To minimise the number of changes, we can go for using jint in AwtMenu::GetItem.

What do you thing, @djelinski and @TheShermanTanker?

Hmm, I lean towards jint as I feel it conveys the fact that it is a Java parameter clearer, intuitively to me it makes sense that a Java integer type would still work in a C++ for loop in native code

You're right… it gives a hint it'll be an upcall into Java. Let's go for jint then.

I don't think there's a need to change the type of the for-loop variable to jint.

Oh no, I didn't mean to change the loop variable, rather that leaving the jint as is should be fine in the for loop

TheShermanTanker · 2023-06-23T06:12:51Z

Wait a minute, I was right, it was a jint the whole time! Oh well, I'll wait for what @aivanov-jdk has to say, but I don't like the idea of leaving both inconsistent

TheShermanTanker · 2023-06-23T17:04:23Z

Alright, waiting for you to do the honours :)

src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp

Co-authored-by: Alexey Ivanov <alexey.ivanov@oracle.com>

TheShermanTanker · 2023-06-24T01:24:32Z

Alright, done

TheShermanTanker · 2023-06-25T15:53:48Z

@aivanov-jdk Is the final change ok with you?

aivanov-jdk · 2023-06-25T16:23:47Z

@aivanov-jdk Is the final change ok with you?

Looks good now. Thanks!

I've run client tests, all is green.

TheShermanTanker · 2023-06-25T23:38:50Z

Haha, thanks Alexsey!

TheShermanTanker · 2023-06-25T23:41:14Z

/integrate

openjdk · 2023-06-25T23:41:57Z

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

0db63ec: 8310571: Use inline @return tag on java.util.Objects
dad7bd9: 8310234: Refactor Locale tests to use JUnit
69f3114: 8310676: add note about unnamed module to Elements.getAllModuleElements
bfcca5e: 8310494: Do not include constantPool.hpp from instanceKlass.hpp
0314292: 8310267: Javadoc for Class#isPrimitive() is incorrect regarding Class objects for primitives
a9c0a0f: 8310628: GcInfoBuilder.c missing JNI Exception checks
556831d: 8310460: Remove jdeps -profile option
d91d0d3: 8310537: Fix -Wconversion warnings in gcUtil.hpp
7628da2: 8279254: PKCS9Attribute SigningTime always encoded in UTFTime
5ca4cdd: 8309740: Expand timeout windows for tests in JDK-8179502
... and 441 more: https://git.openjdk.org/jdk/compare/2d4d850813235a7533cd3bbf776adf69f90f02e6...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-25T23:42:03Z

@TheShermanTanker Pushed as commit c92b049.

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

Fix the Java Integer types on Windows

b925f5e

openjdk bot added the rfr Pull request is ready for review label May 24, 2023

openjdk bot added hotspot-compiler hotspot-compiler-dev@openjdk.org client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels May 24, 2023

dholmes-ora reviewed May 25, 2023

View reviewed changes

TheShermanTanker added 3 commits June 1, 2023 15:06

Revert changes to jaccesswalker and add proper cast to offending call…

628be6b

…site

Nevermind

29b9368

Fix the code that is actually warning

5fa2d3e

prrace reviewed Jun 15, 2023

View reviewed changes

src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp Show resolved Hide resolved

djelinski reviewed Jun 21, 2023

View reviewed changes

src/jdk.accessibility/windows/native/jaccesswalker/jaccesswalker.cpp Outdated Show resolved Hide resolved

aivanov-jdk reviewed Jun 22, 2023

View reviewed changes

Fixups

80b6f78

dholmes-ora approved these changes Jun 23, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 23, 2023

TheShermanTanker added 2 commits June 23, 2023 10:31

Copyright

5c5b437

Revert wrong Copyright

16b5a91

djelinski reviewed Jun 23, 2023

View reviewed changes

TheShermanTanker added 3 commits June 24, 2023 01:02

Swap to jint in awt_Menu.h

01d642d

Swap to jint in awt_MenuBar.cpp

8817dfc

Swap to jint in awt_MenuBar.h

d5f74cc

aivanov-jdk suggested changes Jun 23, 2023

View reviewed changes

src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp Outdated Show resolved Hide resolved

TheShermanTanker and others added 2 commits June 24, 2023 09:21

src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp

a011c5d

Co-authored-by: Alexey Ivanov <alexey.ivanov@oracle.com>

Leave nullptr for another day

3fe2a89

aivanov-jdk approved these changes Jun 25, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 25, 2023

openjdk bot closed this Jun 25, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 25, 2023

TheShermanTanker deleted the integers branch June 25, 2023 23:43

8308780: Fix the Java Integer types on Windows #14125

8308780: Fix the Java Integer types on Windows #14125

Conversation

TheShermanTanker commented May 24, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 24, 2023

openjdk bot commented May 24, 2023

mlbridge bot commented May 24, 2023 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora May 25, 2023

Choose a reason for hiding this comment

TheShermanTanker commented May 25, 2023

TheShermanTanker commented May 25, 2023

dholmes-ora commented May 25, 2023

TheShermanTanker commented May 25, 2023

TheShermanTanker commented May 25, 2023 • edited Loading

TheShermanTanker commented May 31, 2023

TheShermanTanker commented Jun 1, 2023 • edited Loading

prrace commented Jun 1, 2023

TheShermanTanker commented Jun 2, 2023

TheShermanTanker commented Jun 6, 2023

TheShermanTanker commented Jun 8, 2023

aivanov-jdk commented Jun 8, 2023

TheShermanTanker commented Jun 15, 2023

TheShermanTanker commented Jun 20, 2023

mlbridge bot commented Jun 21, 2023

TheShermanTanker commented Jun 21, 2023

TheShermanTanker commented Jun 22, 2023

aivanov-jdk left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

TheShermanTanker commented Jun 23, 2023

djelinski Jun 23, 2023

Choose a reason for hiding this comment

aivanov-jdk Jun 23, 2023

Choose a reason for hiding this comment

aivanov-jdk Jun 23, 2023

Choose a reason for hiding this comment

TheShermanTanker Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

aivanov-jdk Jun 23, 2023

Choose a reason for hiding this comment

TheShermanTanker Jun 23, 2023

Choose a reason for hiding this comment

TheShermanTanker commented Jun 23, 2023 • edited Loading

TheShermanTanker commented Jun 23, 2023

TheShermanTanker commented Jun 24, 2023

TheShermanTanker commented Jun 25, 2023

aivanov-jdk commented Jun 25, 2023

TheShermanTanker commented Jun 25, 2023

TheShermanTanker commented Jun 25, 2023

openjdk bot commented Jun 25, 2023

openjdk bot commented Jun 25, 2023

TheShermanTanker commented May 24, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 24, 2023 •

edited

Loading

TheShermanTanker commented May 25, 2023 •

edited

Loading

TheShermanTanker commented Jun 1, 2023 •

edited

Loading

TheShermanTanker Jun 23, 2023 •

edited

Loading

TheShermanTanker commented Jun 23, 2023 •

edited

Loading