Fix potential bug in jaotc

[Dean Long]

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://github.com/oracle/graal/blob/d8d41988d0438e28df94ef08d5b8573db8f42a81/compiler/src/org.graalvm.compiler.hotspot.jdk12/src/org/graalvm/compiler/hotspot/HotSpotGraalServices.java#L38
http://hg.openjdk.java.net/jdk/jdk12/file/06222165c35f/src/hotspot/share/aot/aotCompiledMethod.hpp#l53

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
>
>
>
>
>
>