RFR: 8304006: jlink should create the jimage file in the native endian for the target platform [v19]

[Mandy Chung]

On Wed, 12 Jul 2023 10:58:49 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review for this change which proposes to fix the issue reported in https://bugs.openjdk.org/browse/JDK-8206890?
>> 
>> The `jlink` command allows a `--endian` option to specify the byte order in the generated image. Before this change, when such a image was being launched, the code would assume the byte order in the image to be the native order of the host where the image is being launched. That would result in failure to launch java, as noted in the linked issue.
>> 
>> The commit in this PR, changes relevant places to not assume native order and instead determine the byte order by reading the magic bytes in the image file's header content.
>> 
>> A new jtreg test has been added which reproduces the issue and verifies the fix.
>
> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 45 commits:
> 
>  - move copyright before imports in the new test
>  - add a new test for jlink --endian usages
>  - merge latest from master branch
>  - use newly introduced Architecture.byteOrder() API
>  - merge latest from master branch
>  - update jdk.tools.jlink.internal.Platform class to be aware of non-current platform's endianness
>  - remove no longer needed constructor
>  - merge latest from master branch
>  - foo
>  - merge latest from master branch
>  - ... and 35 more: https://git.openjdk.org/jdk/compare/753bd563...962d542d

Looks okay with a suggestion to determine the endianness outside `ImageHelper`.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Jlink.java line 168:

> 166:             this.output = output;
> 167:             this.modules = Objects.requireNonNull(modules);
> 168:             this.endian = endian;

`JlinkConfiguration` does not need to know the endianness.   Only the image builder needs it.   Probably good to take the `endian` parameter out from `JlinkConfiguration` and instead pass the value of `--endian` to `JlinkTask::createImageProvider` like other jlink options.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 412:

> 410: 
> 411:         // First create the image provider
> 412:         ImageHelper imageProvider = createImageProvider(config,

formatting nit: line 413-415 should be adjusted to align the parameters.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 568:

> 566:         Map<String, Path> mods = cf.modules().stream()
> 567:             .collect(Collectors.toMap(ResolvedModule::name, JlinkTask::toPathLocation));
> 568:         return new ImageHelper(cf, mods, config.getByteOrder(), retainModulesPath, ignoreSigning,

Also we can move the logic of determining the endianess of the target platform out of `ImageHelper`.   Evaluate the target platform and endianness here and pass to `ImageHelper`.

Suggestion:

        // endian is the value specified via --endian and a new parameter to createImageProvider 
        Platform targetPlatform = targetPlatform(cf, modsPaths);
        ByteOrder targetOrder = endian != null ? endian : targetPlatform.arch().byteOrder();
        return new ImageHelper(cf, mods, targetOrder, retainModulesPath, ignoreSigning,

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 856:

> 854:         // returns true if the current platform's "jmods" directory is the parent of the
> 855:         // passed javaBasePath
> 856:         private static boolean isJavaBaseFromCurrentPlatform(Path javaBasePath) throws IOException {

This method checks if we are jlinking the JMOD files from the default module path.   This is not exactly checking if `java.base` matches the current platform since there are other possible setups too.    Suggest to rename the method name to `isJavaBaseFromDefaultModulePath`.   So the comments should also be updated to reflect that.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 857:

> 855:         // passed javaBasePath
> 856:         private static boolean isJavaBaseFromCurrentPlatform(Path javaBasePath) throws IOException {
> 857:             Path currentPlatformJmods = getDefaultModulePath();

Suggestion:

            Path defaultModulePath = getDefaultModulePath();

-------------

PR Review: https://git.openjdk.org/jdk/pull/11943#pullrequestreview-1527094883
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1261636015
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1261642743
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1261654437
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1261639546
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1261640202