Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Fri, 20 Jan 2023 14:23:13 GMT, Jaikiran Pai wrote: > Right now the ModuleTarget class file attribute is not enough but a short > term fix might be for jlink to have a simple mapping of known ModuleTarget > values to endianness. Overall, it's probably low priority as the most likely > users of this will be environments building for embedded platforms. I've now updated this PR to revert the previous commits and instead follow this suggestion to infer the endianness of the target platform while generating the image. The updated PR now continues to allow users to provide `--endian` option while generating an image through `jlink`. If any value is provided for that option, the image generation will continue to honour that value. However, if no value is provided for that option, then during image generation, we will determine the endianness of the target platform, by using the (OpenJDK specific) `ModuleTarget` attribute of target `java.base` module's `module-info` class. This determination is best-effort and if the value cannot be determined, then we default to the current platform's endianness (like we have been doing so far). This commit hardcodes the mapping between a `ModuleTarget`'s architecture and endianness and this mapping is borrowed from what we already do in the OpenJDK build file https://github.com/openjdk/jdk/blob/master/make/autoconf/platform.m4#L26 No new tests have been added in this change, because I couldn't think of a way to test the cross platform `jlink` execution. Existing tests in `test/jdk/tools/jlink` continue to pass with this change. I've triggered our CI runs to run the tiered tests to make sure nothing regresses. The commit in this PR doesn't deprecate the --endian option and I think we can do that as a separate issue. - PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture [v2]
> 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 incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - Infer endianness for the generated image from the target platform - Revert "8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture" This reverts commit 4020a37849d04d0638941b36c8953884b933461e. - Revert "take into account SecurityManager checks" This reverts commit 1694f3a02cf470ac3aaef5d8cdeb0a40c0c66b12. - Revert "Alan's input - remove "final" and match the current code style" This reverts commit 7a754a1bcd20f02da33a9f5d3d170ead0675d33c. - Revert "fix jcheck issue - convert tab to space" This reverts commit 2422399094862718ed8e0e9d3de77d9396283fa4. - merge latest master branch - fix jcheck issue - convert tab to space - Alan's input - remove "final" and match the current code style - take into account SecurityManager checks - 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture - Changes: - all: https://git.openjdk.org/jdk/pull/11943/files - new: https://git.openjdk.org/jdk/pull/11943/files/24223990..d0f0e0ee Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11943=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11943=00-01 Stats: 384736 lines in 5099 files changed: 212681 ins; 120746 del; 51309 mod Patch: https://git.openjdk.org/jdk/pull/11943.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11943/head:pull/11943 PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Tue, 17 Jan 2023 17:41:33 GMT, Jim Laskey 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. > > src/java.base/share/classes/jdk/internal/jimage/ImageHeader.java line 226: > >> 224: ByteOrder altByteOrder = bb.order() == ByteOrder.BIG_ENDIAN >> 225: ? ByteOrder.LITTLE_ENDIAN : ByteOrder.BIG_ENDIAN; >> 226: bb.flip(); > > Don't you just want to rewind here? Hello Jim, you are right `rewind()` would be a better API to use here. - PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Wed, 11 Jan 2023 13:19:32 GMT, Jaikiran Pai 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. Thank you everyone for the review and the suggestions. > Right now the ModuleTarget class file attribute is not enough but a short > term fix might be for jlink to have a simple mapping of known ModuleTarget > values to endianness. Overall, it's probably low priority as the most likely > users of this will be environments building for embedded platforms. I'll experiment with the short term fix. For now, I'll put this PR in draft and will reopen when it's ready for review. - PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Wed, 11 Jan 2023 13:19:32 GMT, Jaikiran Pai 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. > When jlink is cross linking, as in creating a run-time image for a different > target platform, then I think we should strive to have it create the jimage > file in the native endian for the target platform. Instead I think we > need to consider determine the endian from the target java.base module. Having jlink to determine the endianness for the target platform is a good idea. This will improve its usability and `--endian` option could be dropped. Currently jlink looks at the `ModuleTarget` attribute to determine the target platform, which can be extended to include endianness. > Right now the ModuleTarget class file attribute is not enough but a short > term fix might be for jlink to have a simple mapping of known ModuleTarget > values to endianness. This is a viable short-term solution. - PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Wed, 11 Jan 2023 13:19:32 GMT, Jaikiran Pai 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. I'm in agreement with Alan that this change seems suspicious. That is, you should not attempt to run with non-native byte order. That said, I can see where the java reader could read non-native as an analysis tool. So, C code should continue to be an error if not the native byte order. The java code, might be useful for tools but not sure. - PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Wed, 11 Jan 2023 13:19:32 GMT, Jaikiran Pai 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.
Changes requested by jlaskey (Reviewer).
src/java.base/share/classes/jdk/internal/jimage/ImageHeader.java line 222:
> 220: if (bb.getInt() == MAGIC) {
> 221: return bb.order();
> 222: }
I would have just added `public static final int REVERSE_MAGIC = 0xDADAFECA;`
and add an else clause. to compare to REVERSE_MAGIC. Much simpler.
src/java.base/share/classes/jdk/internal/jimage/ImageHeader.java line 226:
> 224: ByteOrder altByteOrder = bb.order() == ByteOrder.BIG_ENDIAN
> 225: ? ByteOrder.LITTLE_ENDIAN : ByteOrder.BIG_ENDIAN;
> 226: bb.flip();
Don't you just want to rewind here?
src/java.base/share/native/libjimage/imageFile.cpp line 284:
> 282: if (reader == NULL || !reader->open()) {
> 283: // Failed to open.
> 284: if (reader != NULL && reader->_invalid_magic) {
Similar here. Flip the byte order if matches IMAGE_REVERSE_MAGIC.
-
PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Wed, 11 Jan 2023 14:30:46 GMT, Jaikiran Pai 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. > > While running tier testing, one existing test showed that I hadn't taken into > account SecurityManager checks in the new code that I had added. I have now > fixed that part and pushed an update to the PR. > > There's one additional unrelated test failing (`TestMaxCachedBufferSize`) in > a odd way and I'll investigate it separately. @jaikiran I've been mulling over this a bit further. When jlink is cross linking, as in creating a run-time image for a different target platform, then I think we should strive to have it create the jimage file in the native endian for the target platform. Right now this means the user needs to help out by providing the --endian option. For example, if the user is on linux-x64 but generating a run-time image for solaris-sparcv9 or aix-ppc, then it requires specifying jlink --endian=big to ensure that the jimage file is in the endianness needed at runtime. I don't disagree that having the 2-arg ImageFileReader::open retry with the opposite of the endian parameter will work but I don't think this is the right thing to do. Instead I think we need to consider determine the endian from the target java.base module. Right now the ModuleTarget class file attribute is not enough but a short term fix might be for jlink to have a simple mapping of known ModuleTarget values to endianness. Overal l, it's probably low priority as the most likely users of this will be environments building for embedded platforms. - PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Wed, 11 Jan 2023 13:19:32 GMT, Jaikiran Pai 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. I'm in two minds on whether this should be "fixed" in libjimage. I'm wondering if libjimage should continue to use native order and we instead explore having jlink determine the endianness at link time for the target platform - this might be looking at some property in the packaged module for java.base (java.base.jmod). @JimLaskey Do you have any opinion on this? For the image reader used by jrtfs then I think it has to work with both as the file system could be created with java.home set in the env to a run-time image for a complete different platform. - PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Wed, 11 Jan 2023 13:19:32 GMT, Jaikiran Pai 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. While running tier testing, one existing test showed that I hadn't taken into account SecurityManager checks in the new code that I had added. I have now fixed that part and pushed an update to the PR. There's one additional unrelated test failing (`TestMaxCachedBufferSize`) in a odd way and I'll investigate it separately. - PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Wed, 11 Jan 2023 13:50:11 GMT, Alan Bateman 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.
>
> src/java.base/share/classes/jdk/internal/jimage/ImageHeader.java line 199:
>
>> 197: * @throws IOException If any error occurred reading the contents
>> of the image file.
>> 198: */
>> 199: static ByteOrder tryDetectByteOrder(final Path imageFile) throws
>> IOException {
>
> Style-wise I think I would try to keep it consistent with the existing code
> if you can, meaning the noise/over-use of finals can mostly be removed if you
> can.
Done. I've updated the PR to remove all the `final` usage from this part of the
code and made it consistent with the current code.
-
PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Wed, 11 Jan 2023 13:19:32 GMT, Jaikiran Pai 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.
I hope @JimLaskey or @sundararajana can review this.
src/java.base/share/classes/jdk/internal/jimage/ImageHeader.java line 199:
> 197: * @throws IOException If any error occurred reading the contents of
> the image file.
> 198: */
> 199: static ByteOrder tryDetectByteOrder(final Path imageFile) throws
> IOException {
Style-wise I think I would try to keep it consistent with the existing code if
you can, meaning the noise/over-use of finals can mostly be removed if you can.
-
PR: https://git.openjdk.org/jdk/pull/11943
