Fix potential bug in jaotc
Ludovic Henry
luhenry at microsoft.com
Sat Jan 25 01:47:30 UTC 2020
I've created https://github.com/oracle/graal/pull/2096. Thanks for the feedback :)
--
Ludovic
-----Original Message-----
From: Dean Long <dean.long at oracle.com>
Sent: Friday, January 24, 2020 5:07 PM
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
Yes, that's a great idea. If you'd like to get involved yourself, you
can create a Pull Request here:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Foracle%2Fgraal%2Ftree%2Fmaster%2Fcompiler&data=02%7C01%7Cluhenry%40microsoft.com%7C50869631ba974939d17708d7a13322c1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637155113313150283&sdata=K9EJLJChV%2BOz35kjoTZeVoh%2F7WHg80UeWEuw26HgMzE%3D&reserved=0
If you need any help with that, you can email me directly.
thanks,
dl
On 1/23/20 10:06 PM, Ludovic Henry wrote:
> 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%7C50869631ba974939d17708d7a13322c1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637155113313150283&sdata=jRRK1eMS0kRYufdygtPQIbru%2F2HN1nhJujDEDS3%2FuSE%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%7C50869631ba974939d17708d7a13322c1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637155113313150283&sdata=keaNvbokdui6jUhpnG0veJR4r4rGn8b%2Bb8irOSLUEy0%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 hotspot-compiler-dev
mailing list