Fix potential bug in jaotc
Ludovic Henry
luhenry at microsoft.com
Fri Jan 24 06:06:57 UTC 2020
Hi Dean,
That indeed makes a lot more sense! Would it then be possible to add a comment in `src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/MetadataBuilder.java` explaining just that, so it's not confusing for anyone else? I am thinking of something like the following:
diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/MetadataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/MetadataBuilder.java
index 34ce4d8c8b..3cb4c4b65d 100644
--- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/MetadataBuilder.java
+++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/MetadataBuilder.java
@@ -98,7 +98,9 @@ final class MetadataBuilder {
byte[] scopeDesc = metaData.scopesDescBytes();
byte[] relocationInfo = metaData.relocBytes();
byte[] oopMapInfo = metaData.oopMaps();
+ // this may be null as the field does not exist before JDK 13
byte[] implicitExceptionBytes = HotSpotGraalServices.getImplicitExceptionBytes(metaData);
+ byte[] exceptionBytes = metaData.exceptionBytes();
// create a global symbol at this position for this method
NativeOrderOutputStream metadataStream = new NativeOrderOutputStream();
@@ -160,7 +162,7 @@ final class MetadataBuilder {
metadataStream.put(relocationInfo).align(8);
exceptionOffset.set(metadataStream.position());
- metadataStream.put(metaData.exceptionBytes()).align(8);
+ metadataStream.put(exceptionBytes).align(8);
if (implicitExceptionBytes != null) {
implictTableOffset.set(metadataStream.position());
Thank you,
--
Ludovic
-----Original Message-----
From: Dean Long <dean.long at oracle.com>
Sent: Thursday, January 23, 2020 2:52 AM
To: Ludovic Henry <luhenry at microsoft.com>; hotspot-compiler-dev at openjdk.java.net; graal-dev <graal-dev at openjdk.java.net>
Subject: Re: Fix potential bug in jaotc
Hi Ludovic. Good catch noticing this issue! It may not be obvious, but
the code is written this way because the master version of
MetadataBuilder.java that JDK syncs with is upstream in the github Graal
repo, and it supports multiple versions of JDK, some of which don't
support that field. For example:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Foracle%2Fgraal%2Fblob%2Fd8d41988d0438e28df94ef08d5b8573db8f42a81%2Fcompiler%2Fsrc%2Forg.graalvm.compiler.hotspot.jdk12%2Fsrc%2Forg%2Fgraalvm%2Fcompiler%2Fhotspot%2FHotSpotGraalServices.java%23L38&data=02%7C01%7Cluhenry%40microsoft.com%7C3f3fd93631df446c5b2108d79ff23d3b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637153735067898897&sdata=8nBmvFbwZYGzDElkx2jJhLW980hamEFwhh2DbzFgv%2Bw%3D&reserved=0
https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fhg.openjdk.java.net%2Fjdk%2Fjdk12%2Ffile%2F06222165c35f%2Fsrc%2Fhotspot%2Fshare%2Faot%2FaotCompiledMethod.hpp%23l53&data=02%7C01%7Cluhenry%40microsoft.com%7C3f3fd93631df446c5b2108d79ff23d3b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637153735067898897&sdata=P%2BzgyEw1VTfz8b790ac8KOjYdYNR5n%2FAv5WFylN%2BsPo%3D&reserved=0
Notice that there is a different HotSpotGraalServices for almost every
JDK version, and that JDK versions older than 13 don't have that
aot_metadata _nul_chk_table_begin field.
dl
On 1/7/20 2:16 PM, Ludovic Henry wrote:
> Hello,
>
> As I was learning and reading over the codebase of src/jdk.aot and src/hotspot/share/aot, I discovered a discrepancy which might lead to future bugs. The issue is around how some of the metadata is generated and loaded between jaotc and the runtime.
>
> In the runtime, `aot_metadata` (src/hotspot/share/aot/aotCompiledMethod.hpp) contains metadata about each method that is compiled in the AOT image - for example, information like the implicit and explicit exception table, or the oop map. Because the size of this data is specific to each method, `aot_metadata` contains fields that store offsets to the `this` pointer, and the data is stored right after the `aot_metadata` structure in the AOT image.
>
> The structure of `aot_metadata` is the following:
>
> class aot_metadata {
> int _size;
> int _code_size;
> int _entry;
> int _verified_entry;
> int _exception_handler_offset;
> int _deopt_handler_offset;
> int _stubs_offset;
> int _frame_size;
> int _orig_pc_offset;
> int _unsafe_access;
>
> // contain the offset to the `this` pointer
> int _pc_desc_begin;
> int _scopes_begin;
> int _reloc_begin;
> int _exception_table_begin;
> int _nul_chk_table_begin;
> int _oopmap_begin;
> }
>
> When creating the AOT image and generating the information in `jdk.tools.jaotc.MetadataBuilder` (src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/MetadataBuilder.java), the `aot_metadata._nul_chk_table_begin` field is conditionally generated. In the case where there is no implicit exceptions in the method, no data/field is generated in the AOT image. However, there is no check for the existence of this field when loading the `aot_metadata` from the AOT image.
>
> This issue is mitigated by the fact that the `HotSpotMetaData.implicitExceptionBytes` is always set, even if only with an empty `byte[]`. Nonetheless, this field is populated in the runtime in `getMetadata` (in src/hotspot/share/jvmci/jvmciCompilerToVM.cpp), making the initialization and usage of this field far apart, complexifying the overall solution.
>
> The following patch proposes to always generate the `aot_metadata._nul_chk_table_begin` field. It also has the advantage to unify how this field and the others are generated.
>
> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/MetadataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/MetadataBuilder.java
> index 34ce4d8c8b..fa89d34796 100644
> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/MetadataBuilder.java
> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/MetadataBuilder.java
> @@ -98,7 +98,8 @@ final class MetadataBuilder {
> byte[] scopeDesc = metaData.scopesDescBytes();
> byte[] relocationInfo = metaData.relocBytes();
> byte[] oopMapInfo = metaData.oopMaps();
> - byte[] implicitExceptionBytes = HotSpotGraalServices.getImplicitExceptionBytes(metaData);
> + byte[] exceptionBytes = metaData.exceptionBytes();
> + byte[] implicitExceptionBytes = metaData.implicitExceptionBytes();
>
> // create a global symbol at this position for this method
> NativeOrderOutputStream metadataStream = new NativeOrderOutputStream();
> @@ -143,10 +144,7 @@ final class MetadataBuilder {
> NativeOrderOutputStream.PatchableInt scopeOffset = metadataStream.patchableInt();
> NativeOrderOutputStream.PatchableInt relocationOffset = metadataStream.patchableInt();
> NativeOrderOutputStream.PatchableInt exceptionOffset = metadataStream.patchableInt();
> - NativeOrderOutputStream.PatchableInt implictTableOffset = null;
> - if (implicitExceptionBytes != null) {
> - implictTableOffset = metadataStream.patchableInt();
> - }
> + NativeOrderOutputStream.PatchableInt implictTableOffset = metadataStream.patchableInt();
> NativeOrderOutputStream.PatchableInt oopMapOffset = metadataStream.patchableInt();
> metadataStream.align(8);
>
> @@ -160,12 +158,10 @@ final class MetadataBuilder {
> metadataStream.put(relocationInfo).align(8);
>
> exceptionOffset.set(metadataStream.position());
> - metadataStream.put(metaData.exceptionBytes()).align(8);
> + metadataStream.put(exceptionBytes).align(8);
>
> - if (implicitExceptionBytes != null) {
> - implictTableOffset.set(metadataStream.position());
> - metadataStream.put(implicitExceptionBytes).align(8);
> - }
> + implictTableOffset.set(metadataStream.position());
> + metadataStream.put(implicitExceptionBytes).align(8);
>
> // oopmaps should be last
> oopMapOffset.set(metadataStream.position());
> diff --git a/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMetaData.java b/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMetaData.java
> index fc54c41312..3acb6191b9 100644
> --- a/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMetaData.java
> +++ b/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotMetaData.java
> @@ -41,6 +41,7 @@ public class HotSpotMetaData {
> scopesDescBytes = new byte[0];
> relocBytes = new byte[0];
> exceptionBytes = new byte[0];
> + implicitExceptionBytes = new byte[0];
> oopMaps = new byte[0];
> metadata = new String[0];
> // ...some of them will be overwritten by the VM:
>
> Thank you,
>
> --
> Ludovic
>
>
>
>
>
>
More information about the graal-dev
mailing list