RFR: 8311971: SA's ConstantPool.java uses incorrect computation to read long value in the constant pool

[Ashutosh Mehra]

On Fri, 14 Jul 2023 03:08:50 GMT, David Holmes <dholmes at openjdk.org> wrote:

> First, what is it about constructing the long from two ints that is incorrect?

The incorrect part is fetching the ints from index and index+1. This doesn't work for 64-bit platforms because the single entry in runtime constant pool is large enough to store the long value, so the entry at index+1 is not used and is invalid.

In ClassFileParser::parse_constant_pool_entries():

      case JVM_CONSTANT_Long: {
        // some code
        const u8 bytes = cfs->get_u8_fast();
        cp->long_at_put(index, bytes);
        index++;   // Skip entry following eigth-byte constant, see JVM book p. 98
        
The existing code that uses VM.buildLongFromIntsPD() would have worked if each constant pool entry is of 4 bytes. So 32-bit systems it wouldn't be a problem.
>From my understanding getJLongAt() should work for both 32 and 64 bit systems.

> the fact we have VM.buildLongFromIntsPD suggests this is the intended way to do things. Why do we also have Address.getJLongAt()? Do we not actually need VM.buildLongFromIntsPD?

It seems both would produce the same result i.e. getJLongAt(0x1000) == VM.buildLongFromIntsPD(getIntAt(0x1004), getIntAt(0x1000));

> Is its other use in the code in ./jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/StackValueCollection.java also incorrect?

Not sure about that. Looking at the code for StackValueCollection.longAt() it would depend on whether any value is stored in slot+1. If the slots in the stack behave the same as the runtime constant pool, then it might be incorrect.
Also there are no users of StackValueCollection.longAt() method, so its a dead code as of now.

> how can it be that we seemingly have no test for this???

As per my knowledge we have very basic tests that just verify that the classfile generated by SA is parseable by javap. There is not test that checks for equality of each individual component of the classfile.
I have a test [0] that can be a good proxy for ensuring the generated classfiles are in a use-able shape because this test has helped me in uncovering quite a few issues including this one and another one in handling of invokedynamic bytecodes which is being fixed here https://github.com/openjdk/jdk/pull/14852 (although it was reported through another channel).

[0] https://bugs.openjdk.org/browse/JDK-8311101

-------------

PR Comment: https://git.openjdk.org/jdk/pull/14855#issuecomment-1636011869