[13] RFR (S): 8225106: C2: Parse::clinit_deopt asserts when holder klass is in error state

[Doerr, Martin]

Hi Vladimir,

> > The new assertions look correct to me. Would "initializion_not_started" be
> a better name?
> Probably, but ciInstanceKlass::is_not_initialized() mirrors existing
> InstanceKlass::is_not_initialized(). So either both should be renamed or
> left as is.

I think this name is a little bit misleading, because the function implies that the initialization has not even been started.
I'd understand "not initialized" rather as "initialization has not completed".
I'll leave it up to you and other reviewers if you want to change it.


> > I think the "assert(VM_Version::supports_fast_class_init_checks(), ..." are
> pointless in platform files. Would you like to keep them?
> 
> Yes, I'd like to leave them as is.
> 
> In x86_64.ad it doesn't look completely redundant:
>     if (C->clinit_barrier_on_entry()) {
>       assert(VM_Version::supports_fast_class_init_checks(), "sanity");
> 
> Moreover, c1_LIRAssembler_x86.cpp is shared between 32-bit & 64-bit
> ports and 32-bit port lacks fast clinit checks.

Makes sense for files which are shared between 32 and 64 bit.


> > Thanks for improving the test. Seems like you currently expect a wrong
> exception:
> > Execution failed: `main' threw exception: java.lang.AssertionError:
> INIT_FAILURE: unexpected exception thrown: expected
> java.lang.NoClassDefFoundError, caught java.lang.AssertionError
> 
> More like the error message is a bit misleading: the failure is caused
> by a  bug which is addressed by JDK-8225141 [1]. Would providing
> exception message improve the situation?

Thanks for explaining. I'd appreciate a message improvement, but I don't insist on it.


Otherwise, change looks good to me.

Best regards,
Martin