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

[David Holmes]

Hi Christoph,

Sorry for the delay getting back to this.

On 1/07/2021 5:56 pm, Christoph Göttschkes wrote:
> Hi David
> 
> On 7/1/21 3:44 AM, David Holmes wrote:
>> Hi Christoph,
>>
>> On 24/06/2021 10:06 pm, Christoph Göttschkes wrote:
>>> Hi,
>>>
>>> please review the following change, which was way too long on my 
>>> chest. It fixes an assertion in the template interpreter for 
>>> ARM32-softfp.
>>>
>>> For ARM32-softfp, the template interpreter calls into the runtime to 
>>> load a double constant using the ldc bytecode. After the interpreter 
>>> loaded the constants, the assert block does some sanity checks on the 
>>> cached constants. But if the double constant is the first constant to 
>>> be loaded, the cache is not yet initialized and the check results in 
>>> a SIGSEGV.
>>>
>>> I guarded the usage of `ConstantPool::cp_to_object_index` by another 
>>> check, which tests if there are any resolved references and if that's 
>>> the case, the cache has already been initialized and the sanity 
>>> checks can be performed.
>>
>> Why isn't this something that should be fixed in the ARM code rather 
>> than the shared code? I admit I'm struggling to understand exactly how 
>> the problem arises - ldc is not to be used for long/double values.
> 
> Sorry for confusing you. Hopefully the following explanation clears 
> things up.
> 
> The runtime executes the ldc2_w bytecode to load the double constant. 
> For ARM32-softfp this uses the TemplateTable::condy_helper method, which 
> calls InterpreterRuntime::resolve_ldc to load the constant. resolve_ldc 
> is used for all kinds of bytecodes. I probably wrote "calls into the 
> runtime to load a double constant using the ldc bytecode" because of the 
> name resolve_ldc.
> 
> I guess ARM32-softfp is the only platform which uses resolve_ldc for 
> double constants. From the way resolve_ldc is written, it also looks 
> like it should be able to handle ldc2_w bytecodes. There are multiple 
> assertions which check for correct bytecodes etc. and ldc2_w is 
> explicitly among the supported bytecodes.
> 
> We could still think about not using the condy_helper and resolve_ldc 
> for loading double constants on ARM32-softfp, but after that change we 
> would still have a resolve_ldc implementation, which works for ldc2_w, 
> but fails in debug VMs because of the ASSERT check. So I think we have 
> to address both things even if we change the ARM interpreter behavior 
> for ldc2_w.

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.

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.

Thanks,
David
-----

>>
>> Thanks,
>> David
>> -----
>>
> -- Christoph
>