Build the JSpecify JDK #114

wmdietl · 2024-12-17T00:52:22Z

This helps ensure all .java files successfully compile.

The basic java compilation is successful for me locally.
The final make jdk fails with:

=== Output from failing command(s) repeated here ===
* For target buildtools_create_symbols_javac__the.COMPILE_CREATE_SYMBOLS_batch:
Error occurred during initialization of boot layer
java.lang.StackOverflowError

I do not get that error when I compile eisop/jdk at https://github.com/eisop/jdk/tree/jdk-24 ... Something in a merge in this repo must have gone wrong...

#99

…cify/jdk into jspecify-annotations-for-ci-2

…annotations-for-ci-2

This needs to be checked against j2cl: b/201433789.

See eisop/jdk@b5f9d91

Otherwise, we get errors like this one: src/java.base/share/classes/org/jspecify/annotations/Nullable.java:39: error: unmappable character (0x80) for encoding ascii * List<@nullable String> getList() { �� }

java/time/zone/ZoneOffsetTransitionRule.java is compiled for the bootstrap tools, so must not contain JSpecify annotations.

wmdietl · 2024-12-17T02:54:34Z

@cpovirk The CI task now compiles the java files, but then also fails with

Creating support/modules_libs/jdk.security.auth/libjaas.so from 1 file(s)
Compiling up to 4 files for COMPILE_CREATE_SYMBOLS
Error occurred during initialization of boot layer
java.lang.StackOverflowError
gmake[3]: *** [Gendata.gmk:64: /home/runner/work/jdk/jdk/build/linux-x86_64-server-release/buildtools/create_symbols_javac/_the.COMPILE_CREATE_SYMBOLS_batch] Error 1
gmake[2]: *** [make/Main.gmk:139: jdk.compiler-gendata] Error 2
gmake[2]: *** Waiting for unfinished jobs....

Some earlier merge must have mixed something up... maybe you see what's going wrong?

Some of the OpenJDK GHA seem to pass. I found a simple way to disable them all, to avoid wasting cycles. Let me know if you want me to do that.

I tried to break down my changes into meaningful chunks, please have a look through my comments. In particular, you need to check whether this change is a problem.

cpovirk

Nice!

I will look at the rest in more detail, but one quick response:

src/java.base/share/classes/java/util/Objects.java

cpovirk · 2024-12-17T15:20:57Z

src/java.base/share/classes/java/util/Properties.java

@@ -1515,7 +1515,7 @@ public synchronized Object computeIfAbsent(Object key,

    @Override
    public synchronized @Nullable Object compute(Object key,
-            BiFunction<? super Object, ?, ?> remappingFunction) {
+            BiFunction<? super Object, ? super Object, ? extends @Nullable Object> remappingFunction) {


Is the change from ? to ? extends @Nullable Object (for the BiFunction output) necessary for technical reasons? I would expect that we'd want to stick with ? to match what the JDK has: With the annotation on the wildcard bound, which doesn't exist in the JDK class file, we end up with 1: #8(): METHOD_FORMAL_PARAMETER, param_index=1, location=[TYPE_ARGUMENT(2), WILDCARD], i.e., an annotation for a bound that doesn't exist. (We had trouble with bogus annotation locations previously in Remove method-level Atomic annotations. #56.) Now maybe the JDK has had different types at different times (though I checked JDK 11 and JDK 17, and both had the bare ?).

The change from ? to ? super Object (for the second input of the BiFunction) seems strange in another way: That parameter is normally nullable (including in Map and in Hashtable). I could understand ? super @Nullable Object if we needed that instead of ? (as discussed in the previous bullet), but the non-null type is stranger.

If it turns out that we do need the types written this way here and need them written differently inside Google, we can accomplish it with a patch easily enough. I just want to understand where we stand (and be ready to warn NullAway or others if we learn of problems).

The original signature produces a Java error:

src/java.base/share/classes/java/util/Properties.java:1517: error: name clash: compute(Object,BiFunction<? super Object,?,?>) in Properties and compute(Object,BiFunction<? super Object,? super Object,? extends Object>) in Hashtable have the same erasure, yet neither overrides the other public synchronized @Nullable Object compute(Object key, ^

I need to change it to at least BiFunction<? super Object,? super Object,?> to make Java happy. I removed the @Nullable, I added that too eagerly.
Is the new signature okay?

cpovirk

We can talk more about Properties. Everything else looks great. Thanks again.

cpovirk · 2024-12-17T16:19:34Z

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

We had dropped this intentionally. But I'm actually all for restoring it: We changed the way we use these stubs inside Google so that we drop all files without JSpecify annotations, including this one. So it doesn't matter what we have in this file :) (Now, if we someday add JSpecify annotations to the class, then we'd have an issue. But we might be better off solving that by manually omitting the file inside Google, which we have multiple ways to do.)

Is there a way to restore that file's history? Maybe by reverting that earlier commit?

I tried git revert 0d2d6b6e5515313d48478d39115e689cd13a3767 and that fails, at least in this branch.

I suspect that we're out of luck for blame output. I can still see history with...

git log src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

...or even more with...

git log --full-history -- src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

Maybe that's good enough?

Co-authored-by: Chris Povirk <[email protected]>

cpovirk · 2024-12-17T19:03:13Z

src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

I suspect that we're out of luck for blame output. I can still see history with...

git log src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

...or even more with...

git log --full-history -- src/java.desktop/share/classes/java/awt/color/ICC_Profile.java

Maybe that's good enough?

src/java.base/share/classes/java/util/Properties.java

This reverts commit c62e6d0.

…ullable. This matches [what we have for `Map.compute`](https://github.com/jspecify/jdk/blob/7cc2fbd5f404bf5403bd1375b47024a855838fef/src/java.base/share/classes/java/util/Map.java#L1247), noting that `Properties` is a `Hashtable<Object, Object>` and so a `Map<Object, Object>. See previous discussion in #114 (comment)

…ullable. This matches [what we have for `Map.compute`](https://github.com/jspecify/jdk/blob/7cc2fbd5f404bf5403bd1375b47024a855838fef/src/java.base/share/classes/java/util/Map.java#L1247), noting that `Properties` is a `Hashtable<Object, Object>` and so a `Map<Object, Object>`. See previous discussion in #114 (comment)

…ullable. (#117) This matches [what we have for `Map.compute`](https://github.com/jspecify/jdk/blob/7cc2fbd5f404bf5403bd1375b47024a855838fef/src/java.base/share/classes/java/util/Map.java#L1247), noting that `Properties` is a `Hashtable<Object, Object>` and so a `Map<Object, Object>`. See previous discussion in #114 (comment)

cpovirk and others added 18 commits October 7, 2024 10:57

Try adding the JSpecify annotations to see if it helps with CI.

ba27da4

#99

Trim list of platforms.

13ae048

Try --disable-jvm-feature-jvmti.

c62e6d0

Merge branch 'jspecify-annotations-for-ci' of https://github.com/jspe…

c87fa58

…cify/jdk into jspecify-annotations-for-ci-2

Delete unused files

1b4743c

Merge branch 'main' of https://github.com/jspecify/jdk into jspecify-…

30005e1

…annotations-for-ci-2

Delete Checker Framework annotation declarations

b9c9aea

Remove uses of Checker Framework annotations

3b2d67e

Add missing imports

817de16

Static method needs a body.

3510a10

This needs to be checked against j2cl: b/201433789.

Make signature compatible with Hashtable

053209b

Make build files consistent with eisop/jdk versions

db687aa

Hacky handling of type annotations.

4fed369

See eisop/jdk@b5f9d91

Add file that was lost in some merge

1c19f4a

Use ASCII letters only

00585ac

Otherwise, we get errors like this one: src/java.base/share/classes/org/jspecify/annotations/Nullable.java:39: error: unmappable character (0x80) for encoding ascii * List<@nullable String> getList() { �� }

Strip JSpecify annotations from file in bootstrap tools.

09309ef

java/time/zone/ZoneOffsetTransitionRule.java is compiled for the bootstrap tools, so must not contain JSpecify annotations.

Minimal GitHub Actions CI setup

34da1c8

Install dependencies

efbb1e3

wmdietl assigned cpovirk Dec 17, 2024

cpovirk reviewed Dec 17, 2024

View reviewed changes

src/java.base/share/classes/java/util/Objects.java Outdated Show resolved Hide resolved

cpovirk reviewed Dec 17, 2024

View reviewed changes

cpovirk approved these changes Dec 17, 2024

View reviewed changes

wmdietl and others added 3 commits December 17, 2024 13:38

Fix stack overflow

75a3cf4

Co-authored-by: Chris Povirk <[email protected]>

Avoid annotation on Object wildcard bound

56bb9db

Only run JSpecify CI job

ea1d2b7

cpovirk approved these changes Dec 17, 2024

View reviewed changes

src/java.base/share/classes/java/util/Properties.java Show resolved Hide resolved

wmdietl added 2 commits December 17, 2024 17:14

Revert "Try --disable-jvm-feature-jvmti."

858e3ea

This reverts commit c62e6d0.

Fix build warnings by reducing diffs to openjdk/jdk

9f0fe99

wmdietl merged commit c9befbc into main Dec 17, 2024
19 checks passed

wmdietl deleted the jspecify-annotations-for-ci-2 branch December 17, 2024 23:50

cpovirk mentioned this pull request Dec 19, 2024

Mark the "previous value or null" type of Properties.compute as nullable. #117

Merged

cpovirk mentioned this pull request Dec 20, 2024

Set up meaningful CI #99

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build the JSpecify JDK #114

Build the JSpecify JDK #114

wmdietl commented Dec 17, 2024

wmdietl commented Dec 17, 2024

cpovirk left a comment

cpovirk Dec 17, 2024

wmdietl Dec 17, 2024

cpovirk left a comment

cpovirk Dec 17, 2024

wmdietl Dec 17, 2024

wmdietl Dec 17, 2024

cpovirk Dec 17, 2024

cpovirk Dec 17, 2024

Build the JSpecify JDK #114

Build the JSpecify JDK #114

Conversation

wmdietl commented Dec 17, 2024

wmdietl commented Dec 17, 2024

cpovirk left a comment

Choose a reason for hiding this comment

cpovirk Dec 17, 2024

Choose a reason for hiding this comment

wmdietl Dec 17, 2024

Choose a reason for hiding this comment

cpovirk left a comment

Choose a reason for hiding this comment

cpovirk Dec 17, 2024

Choose a reason for hiding this comment

wmdietl Dec 17, 2024

Choose a reason for hiding this comment

wmdietl Dec 17, 2024

Choose a reason for hiding this comment

cpovirk Dec 17, 2024

Choose a reason for hiding this comment

cpovirk Dec 17, 2024

Choose a reason for hiding this comment