Possible subtle memory model error in ClassValue

[Peter Levart]

On 8/16/20 7:35 PM, Andrew Haley wrote:
> On 15/08/2020 10:13, Peter Levart wrote:
>> https://github.com/openjdk/jdk/pull/9
>>
>>
>> Sorry for abusing GitHub pull request mechanism but I don't have
>> bandwidth currently to clone the mercurial repository ;-)
> That's a lot of work to avoid a simple fence.
>
Two fences, mind you (the read fence is no-op only on Intel). So take 
half of that work for each fence ;-) Still a lot?

No, really it was not much work to make the patch. The real work is yet 
to come - checking that it is correct.


I leaned on the assumption that it is already correct when the 
cacheArray has to be swapped with bigger array. All accesses to the 
cacheArray field but the fast-path read-only probing is done under lock. 
So it is not that hard to reason about most of the code changes. The 
only changes that are done to code that accesses cacheArray field 
without holding a lock are two additional null checks in: loadFromCache 
and probeBackupLocations which basically just make the methods return 
null (not found) when parameter cache (read from cacheArray field) is 
found to be null. Now that the set of valid values for cache parameter 
contains null, there's no need for EMPTY_CACHE.


I think that ClassValue was designed to cope with only one pair of 
fences (the ones that are used for initializing/accessing the final 
Entry.value field) and that all other accesses are performed with data 
races. So adding fences to accesses of cacheArray field would mean 
giving up on the design (John?).


Regards, Peter