RFR (XS) 8055146: Split Verifier incorrectly throws VerifyError for getstatic of an array field

[David Holmes]

On 6/01/2015 6:24 AM, harold seigel wrote:
> Hi David,
>
> Thanks for looking at this change.
>
> For getfield, JVM 8 Spec says:
>
>     A /getfield/ instruction with operand |CP| is type safe iff |CP|
>     refers to a constant pool entry denoting a field whose declared type
>     is |FieldType|, declared in a class |FieldClass|, and one can
>     validly replace a type matching |FieldClass| with type |FieldType|
>     on the incoming operand stack yielding the outgoing type state.
>     |FieldClass| must not be an array type. |protected| fields are
>     subject to additional checks (§4.10.1.8
>     <http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.10.1.8>).
>
>
> The description of getstatic in JVMS-8
> <http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.10.1.9.getstatic>
> is similar, but does not restrict FieldClass from being an array.

Yep I got that. Don't understand why this distinction exists though.

> In the ArrayField example, the FieldClass is ArrayField.  This is not an
> array type and so is a valid FieldClass for a getfield instruction.

Ah! This is not the field type but the type of the "class" defining the 
field - got it. And as arrays don't have fields this is not allowed (and 
can't be expressed in Java source code). Except we're allowing it for 
static fields (which don't exist) ??? And we disallow it for instance 
fields even though there is the psuedo-field for the array length. 
Totally bizarre.

So in terms of your fix making the splitverifier work the same as the 
old verifier - fine.

But as far as understanding why the getfield and getstatic are defined 
differently in the first place, I just don't get it. :)

Thanks,
David

>        public void test();
>          descriptor: ()V
>          flags: ACC_PUBLIC
>          Code:
>            stack=2, locals=3, args_size=1
>               0: getstatic     #2                  // Field
>     sarray:[Ljava/lang/Object;
>               3: astore_1
>               4: aload_0
>               5: getfield      #3                  // Field
>     array:[Ljava/lang/Object;
>                      ...
>
>     Constant pool:
>         #1 = Methodref          #7.#19         //
>     java/lang/Object."<init>":()V
>         #2 = Fieldref           #6.#20         //
>     ArrayField.sarray:[Ljava/lang/Object;
>         #3 = Fieldref           #6.#21         //
>     ArrayField.array:[Ljava/lang/Object;
>         #4 = Class              #22            // java/lang/RuntimeException
>         #5 = Methodref          #4.#19         //
>     java/lang/RuntimeException."<init>":()V
>     *#6 = Class              #23 // ArrayField*
>                              ...
>
>
> In the below example, the FieldClass is an array.  According to JVMS-8,
> this is valid for getstatic (and putstatic) but not valid for getfield
> (and putfield):
>
>        public static void main(java.lang.String[]);
>          descriptor: ([Ljava/lang/String;)V
>          flags: ACC_PUBLIC, ACC_STATIC
>          Code:
>            stack=2, locals=2, args_size=1
>               0: getstatic     #2                  // Field
>     "[C".out:Ljava/io/PrintStream;
>               3: putstatic     #2                  // Field
>     "[C".out:Ljava/io/PrintStream;
>                        ...
>
>     Constant pool:
>         #1 = String             #30            // Hello
>         #2 = Fieldref           #13.#24        //
>     "[C".out:Ljava/io/PrintStream;
>         #3 = Methodref          #25.#28        //
>     java/io/PrintStream.println:(Ljava/lang/String;)V
>         #4 = Methodref          #17.#10        //
>     java/lang/Object."<init>":()V
>         #5 = Methodref          #23.#16        //
>     java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
>         #6 = Utf8               ()V
>         #7 = Utf8               Getstatic_Test
>         #8 = Utf8               java/lang/Integer
>         #9 = Utf8               <init>
>        #10 = NameAndType        #9:#6          // "<init>":()V
>        #11 = Utf8               out
>        #12 = Utf8               java/io/PrintStream
>     *  #13 = Class              #22 // "[C"*
>                  ...
>
>
> The bug fix allows the FieldClass type to be an array for getstatic and
> putstatic, bu tthrows VerifyError for getfield and putfield.
>
> Thanks, Harold
>
>
> On 1/4/2015 9:13 PM, David Holmes wrote:
>> Hi Harold,
>>
>> On 1/01/2015 2:38 AM, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this small fix for bug 8055146.  The fix allows the
>>> operands of getstatic and putstatic bytecodes to be arrays, instead of
>>> throwing VerifyError exceptions.
>>>
>>> Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8055146/
>>>
>>> JBS bug: https://bugs.openjdk.java.net/browse/JDK-8055146
>>
>> I'm missing something here. It says for getField that it must not be
>> an array type; but doesn't say that for getStatic. This puzzles me on
>> two fronts:
>>
>> 1. Why should there be any difference between accessing a static
>> versus a non-static field in this regard?
>>
>> 2. If I code this:
>>
>> public class ArrayField {
>>   static Object[] sarray;
>>   Object[] array;
>>
>>   public void test() {
>>     Object o = sarray;
>>     Object o2 = array;
>>     if (o == o2) throw new RuntimeException();
>>   }
>> }
>>
>> the bytecode generated is:
>>
>>   public void test();
>>     Code:
>>        0: getstatic     #2                  // Field
>> sarray:[Ljava/lang/Object;
>>        3: astore_1
>>        4: aload_0
>>        5: getfield      #3                  // Field
>> array:[Ljava/lang/Object;
>>        ...
>>
>> so this seems to be using getField for an array field when it is not
>> allowed ??
>>
>> Thanks,
>> David
>>
>>> The fix was tested with JCK lang and VM tests, hotspot JTReg, and NSK
>>> split_verifier tests.
>>>
>>> Thanks, Harold
>