RFR: 8217450: Add PackageEntry::locked_lookup_only

[David Holmes]

On 23/01/2019 5:35 am, Claes Redestad wrote:
> 
> 
> On 2019-01-22 18:10, Lois Foltan wrote:
>>>>
>>>> Ok - updated http://cr.openjdk.java.net/~redestad/8217450/open.01/ in
>>>> place and running some sanity tests before push.
>>>>
>>>> Thanks!
>>>>
>>>> /Claes
>>>
>>> Hi Claes,
>>>
>>> The code change in modules.cpp lines #211-214 does not look correct. 
>>> PackageEntry::locked_create_entry_or_null() will return NULL if the 
>>> package exists and has already been created.  The code at lines 
>>> #211-214 need a PackageEntry to be successfully returned for 
>>> java.base packages created during bootstrapping so we can decrement 
>>> the refcount for it appropriately.
>>
>> Just to clarify further.  Obtaining a PackageEntry itself is not a 
>> requirement for decrementing the refcount of the Symbol* within the 
>> table.  However, the assert at line #213 will trigger if 
>> PackageEntry::locked_create_entry_or_null() returns NULL for an 
>> existing package.  A PackageEntry can be created for any class loaded, 
>> such as java/lang/Class and java/lang/Object for example, during 
>> bootstrapping before java.base is defined to the JVM.  These classes 
>> are put on a fixup_module_list so that once the java.base module is 
>> defined the JVM can then fixup their java.lang.Module field to be 
>> correctly set to java.base.
> 
> Hi Lois,
> 
> thanks for looking at this!
> 
> As you say: when locked_create_entry_or_null() returns NULL this
> *should* be because the package already exists.  The assert is thus not
> asserting if pkg == NULL alone, locked_lookup_only() must also return
> NULL for the assert to trigger: i.e., I'm checking that a NULL really
> meant that the package already existed, and not something else, such as
> a failure to allocate the PackageEntry.

I'm in agreement with Claes here. In the original case the assert checks 
we created a pkg because we are in a path only executed when the package 
did not pre-exist. In the new code the assert is unconditional so the 
package may have pre-existed, which is what the second clause is now 
checking: either we successfully created a pkg, or we didn't because it 
already existed.

David
-----

> 
> I thought this was the purpose of the original assert: getting a NULL
> back from locked_create_entry_or_null() when we *know* there was no pre-
> existing package should be impossible (we're holding the lock, so that a
> NULL must be a failure to allocate or worse).
> 
> Am I missing something here?
> 
> /Claes