RFR: 8230797: ARM32-softfp: assertion in InterpreterRuntime::resolve_ldc

[Christoph Göttschkes]

Hi David.

On 7/6/21 2:27 AM, David Holmes wrote:

> 
> I do find this somewhat inconsistent and confusing. Looking at x86 and Aarch64 for example, TemplateTable::ldc2_w doesn't use condy_helper for long/double, yet condy_helper has case statements for _ldc2_w to handle long and double; which occur after the call to InterpreterRuntime::resolve_ldc.
> 
> For ARM32 we have two cases. For __ABI_HARD__ condy_helper is not used for double; but otherwise it is. And again condy_helper has explicit case statements for _ldc2_w, but after the call to InterpreterRuntime::resolve_ldc.
> 
> And resolve_ldc itself seems to be catering for all kinds of constants, except the ASSERT block. It seems to me that logic is only intended for use with ldc's relating to objects not primitives. Now maybe cp_to_object_index is expected to handle that and just return the _no_index_sentinel, or perhaps this just happens to work by accident (as long as the map is initialized)? I'm really not sure. And those ConstantPool methods should really have their own asserts to check that the reference_map() has in fact been initialized.
> 

I looked into this now in depth. Thanks for your input, it really helped me getting through it. I think I was able to find the root cause now.

condy_helper and resolve_ldc should only be used for CONSTANT_dynamic. The ldc, ldc_w, and ldc2_w bytecodes are all valid with CONSTANT_dynamic and therefore it makes sense that the condy_helper has a case for those bytecodes. Also, I think it makes sense that the Assert block always thinks that something has been cached, since for a CONSTANT_dynamic, something always needs to be cached, even if the constant is a double. So I think the confusion comes, because you think ldc2_w should only be used for double and long. That's true, but because of CONSTANT_dynamic, the special handling of the interpreter isn't used and the condy_helper is called (hopefully that makes sense, I am by no means an expert on condy, but the hotspot enum for the constant pool entries has JVM_CONSTANT_Double, JVM_CONSTANT_Long and JVM_CONSTANT_Dynamic, and those constants are used for the checks in the template interpreter).

All the handling around ldc, ldc_w, and ldc2_w in resolve_ldc comes from the initial condy implementation [1] [2].

For the ARM32 case, it looks like the initial condy implementation introduced the wrong behavior [3]. Before condy, the template interpreter loaded double and long identically for softp [4]. With the condy implementation, a new check was introduced [5] and it was overlooked that from now on, CONSTANT_double was no longer loaded using the interpreter templates alone, but with the help of the condy_helper and the InterpreterRuntime::resolve_ldc.

Maybe there is also something wrong/strange in resolve_ldc about the assumption of caching, but I think that should be looked after by someone more familiar with condy.

> Your proposed workaround, doesn't really address any of the underlying confusion and inconsistency, but does at least attempt to get things working again on ARM32. That said, I think you need to be using ConstantPool::resolved_references_or_null() as resolved_references() still relies on _cache being initialized.

I am not so sure on how to go forward now. I would like to stick with the current fix and will open a new bug which I will fix in the near future. I don't have a softfp setup right now and need to organize some hardware to make proper tests. I will update the PR with your suggestion, even though I think that the cache itself should always be initialized and never be null, if we are already executing a method of a class. It might be that there are no resolved references in the cache (like in the case this issue addresses). But on the other hand, an additional NULL check doesn't hurt.

Is that OK for you, or do you think we should make a new bug and close this one as won't fix?

[1] https://bugs.openjdk.java.net/browse/JDK-8186046
[2] https://hg.openjdk.java.net/jdk/hs/rev/c4d9d1b08e2e#l18.6
[3] https://github.com/openjdk/jdk/commit/2ee56fd1cf4acd634e19cc77b7c9a6858e1c315a
[4] https://github.com/openjdk/jdk/blob/7101b28dc3903be27cb46a00781f4d74f81f1114/src/hotspot/cpu/arm/templateTable_arm.cpp#L529
[5] https://github.com/openjdk/jdk/blob/2ee56fd1cf4acd634e19cc77b7c9a6858e1c315a/src/hotspot/cpu/arm/templateTable_arm.cpp#L545

Thanks,
Christoph