RFR (S) 8251336: Shenandoah: assert "only get here when SATB active" after JDK-8244997

[Roman Kennke]

On Wed, 2020-08-12 at 08:01 +1000, David Holmes wrote:
> On 12/08/2020 12:42 am, Roman Kennke wrote:
> > On Tue, 2020-08-11 at 23:35 +1000, David Holmes wrote:
> > > Hi Coleen,
> > > 
> > > On 11/08/2020 9:40 pm, Coleen Phillimore wrote:
> > > > Summary: Release OopStorage oops outside a safepoint.
> > > > 
> > > > Tested with tier1-6.
> > > > 
> > > > open webrev at
> > > > http://cr.openjdk.java.net/~coleenp/2020/8251336.01/webrev
> > > > bug link https://bugs.openjdk.java.net/browse/JDK-8251336
> > > > 
> > > > Thank you to Zhengyu for alerting me about this issue.
> > > 
> > > Yeah ... I was focusing on the locking aspect and completely
> > > overlooked
> > > the fact that section of code is within the scope of the TBIVM.
> > > Not
> > > that
> > > it would have immediately rung any alarm bells.
> > > 
> > > Do we have usage rules about manipulating OopStorage at a
> > > safepoint?
> > > If
> > > some GC's can't cope with it then there should be some detection
> > > of
> > > that
> > > in the OopStorage APIs.
> > 
> > In general, non-GC threads should not mess with GC roots (e.g.
> > OopStorage) during safepoints. I don't think that this plays well
> > with
> > any GC. Not sure if it can be automatically detected, it sure would
> > be
> > nice if that would fire an assert or so.
> 
> I will file a RFE for that.

In-fact, this current bug is kinda what we want, except that the assert
that we've seen is buried in GC specific code while it should be
generic, and was checking another specific invariant (which is violated
more rarely: only when observing the race), while we should explicitely
check that threads do not mess with GC roots. This can probably be
achieved in the heap access layer somehow.

Roman