Possible subtle memory model error in ClassValue
peter.levart at gmail.com
Mon Aug 17 14:24:44 UTC 2020
On 8/16/20 7:35 PM, Andrew Haley wrote:
> On 15/08/2020 10:13, Peter Levart wrote:
>> 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?).
More information about the core-libs-dev