On Mon, 20 Mar 2023 14:21:55 GMT, Jaikiran Pai <j...@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 incrementally with one additional > commit since the last revision: > > 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 src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 548: > 546: Path > retainModulesPath, > 547: boolean > ignoreSigning, > 548: boolean bindService, formatting nit: align the parameters with the first one. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 822: > 820: PrintWriter log) throws IOException { > 821: if (order != null) { > 822: this.targetPlatform = Platform.runtime(); The target platform should always be fetched from `java.base` (as it was implemented in `DefaultImageBuilder`) or use the current runtime if `java.base` is from the module path of the current runtime. If `--endian <order>` is specified, jlink can check if the target platform's endianness matches the specified value and emit an error if mismatch unless `Platform::getNativeByteOrder` is null. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 856: > 854: // unsupported target platform > 855: throw new IOException( > 856: > taskHelper.getMessage("err.unsupported.target.platform", IIUC, the current jlink implementation does not fail if the target platform is unknown as long as `--endian` option is specified. So I think `--endian` may be useful to keep (rather than taking it out). This error key should be `err.unknown.target.endianness` and the message can suggest to use `--endian` option to specify. src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Platform.java line 192: > 190: case "s390x" -> Architecture.s390x; > 191: case "sparc" -> Architecture.SPARC; > 192: case "sparcv9" -> Architecture.SPARCv9; This list should be trimmed to the ones supported in the mainline as in `target.properties`. src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 151: > 149: err.signing=signed modular JAR {0} is currently not supported,\ > 150: \ use --ignore-signing-information to suppress error > 151: err.cannot.determine.target.platform=cannot determine target platform > for image generation Suggestion: err.cannot.determine.target.platform=cannot determine target platform from {0} `{0}` can be the path to `java.base.jmod` (i.e. `ModuleReference::location`) src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 152: > 150: \ use --ignore-signing-information to suppress error > 151: err.cannot.determine.target.platform=cannot determine target platform > for image generation > 152: err.unsupported.target.platform=image generation for target platform {0} > is not supported Suggestion: err.unsupported.target.endianness=Unknown native byte order for target platform {0}` `{0}` is `targetPlatformVal` ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143813371 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143823833 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143872844 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143847888 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143875777 PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143877925
