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

[Alan Bateman]

On Fri, 24 Mar 2023 02:18:43 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 28 commits:
> 
>  - merge latest from master branch
>  - Mandy's suggested patch improvements
>  - ARM is 32 bit as per platform.m4 in OpenJDK build
>  - when --endian is specified, verify it matches the implicitly determined target platform's endianness
>  - trim down the architecture support to the previous values plus a few new that match target.properties
>  - improve error messages as suggested by Mandy
>  - formatting fix
>  - Alan's suggestions - don't parse arch out of osname-arch for determining endianness and reduce the number of supported/known target platforms for cross linking
>  - Mandy's suggestion - pass along target platform to the DefaultImageBuilder to prevent reparsing the value from the ModuleTarget
>  - Mandy's suggestion to use Platform class for additional arch support. Plus, Jim's suggestion to use a runtime resource for endianness mapping
>  - ... and 18 more: https://git.openjdk.org/jdk/compare/ac6af6a6...6f4ab9c6

I think you've got this to a good place. The other remaining issue is the target.properties and whether it limited to endianness as proposed or whether it can be used for other properties of the target.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Platform.java line 49:

> 47:             throw new ExceptionInInitializerError(e);
> 48:         }
> 49:         KNOWN_ENDIANNESS = p;

The naming/usage suggests the value is solely for endianness but I think we want target properties to be able to support other attributes of the target too. This means "endianness" in the property name or the property value is something like a comma separate list of attributes, I don't have a strong opinion on this. But maybe your proposal is just to get the target.properties in place and we change it later?

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

PR Review: https://git.openjdk.org/jdk/pull/11943#pullrequestreview-1357028246
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1147793209