RFR: 8320530: has_resolved_ref_index flag not restored after resetting entry [v4]

[Matias Saavedra Silva]

On Mon, 27 Nov 2023 09:03:01 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

>>> Change seems fine but what was the effect of not restoring the flag? Does this cause failures or just unnecessary re-resolution, or?
>>> 
>>> Thanks
>> 
>> The code works somehow, but in an unsafe manner. We are reading from `resolved_references_index()` even when the `has_resolved_ref_shift` bit has been (improperly) cleared. Adding the following assert makes it impossible to start jtreg:
>> 
>> 
>> diff --git a/src/hotspot/share/oops/cpCache.cpp b/src/hotspot/share/oops/cpCache.cpp
>> index daa094baa7e..760c5268c88 100644
>> --- a/src/hotspot/share/oops/cpCache.cpp
>> +++ b/src/hotspot/share/oops/cpCache.cpp
>> @@ -310,6 +310,9 @@ ResolvedMethodEntry* ConstantPoolCache::set_method_handle(int method_index, cons
>>    // Store appendix, if any.
>>    if (has_appendix) {
>> +    assert(method_entry->has_resolved_ref_index(), "huh");
>>      const int appendix_index = method_entry->resolved_references_index();
>>      objArrayOop resolved_references = constant_pool()->resolved_references();
>> 
>> 
>> I think we should take this as a chance to tighten up the code in resolvedMethodEntry.hpp:
>> 
>> - `resolved_references_index()` should assert that `has_resolved_ref_index()`.
>> - The following three functions should assert for mutual exclusivity. I.e., you can't call set_klass() and then call  set_resolved_references_index(). Probably the easiest is to add two more bits: `_has_klass_shift` and `_has_table_index_shift`. At entry of these three setters, we should assert that all three klass/table_index/resolved_references_index bits are cleared.
>> 
>> 
>>   void set_klass(InstanceKlass* klass) {
>>     _entry_specific._interface_klass = klass;
>>   }
>>   void set_resolved_references_index(u2 ref_index) {
>>     set_flags(1 << has_resolved_ref_shift);
>>     _entry_specific._resolved_references_index = ref_index;
>>   }
>>   void set_table_index(u2 table_index) {
>>     _entry_specific._table_index = table_index;
>>   }
>> 
>> 
>> Also, `has_resolved_ref_index` should be renamed to `has_resolved_reference_index`; otherwise it's difficult to search for all code related to `resolved_refenece_index`.
>
>> . . . Probably the easiest is to add two more bits: _has_klass_shift and _has_table_index_shift. . . .
> 
> Maybe so, but only in debug builds?

Thank you for the reviews @adinn, @coleenp, and @dholmes-ora!

The remaining GHA failure is not related to my change.

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

PR Comment: https://git.openjdk.org/jdk/pull/16769#issuecomment-1834575171