RFR: 8336103: Sharper checks for <init> and <clinit> initializers [v2]

[Aleksey Shipilev]

On Fri, 12 Jul 2024 03:59:06 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Indenting
>
> src/hotspot/share/classfile/javaClasses.cpp line 3018:
> 
>> 3016:   int flags = (jushort)( m->access_flags().as_short() & JVM_RECOGNIZED_METHOD_MODIFIERS );
>> 3017:   if (m->is_object_initializer()) {
>> 3018:     flags |= java_lang_invoke_MemberName::MN_IS_CONSTRUCTOR;
> 
> I'm going to assume that `clinit` would already get filtered out at some point otherwise this would be a change in behaviour.

No, it is not filtered, we still have `clinit`-s on this path. In the initial version https://github.com/openjdk/jdk/pull/20120/commits/6769cfe609849aa9ed0985dcbecb2b0aa24bca03 I caught the assert in many tests, mostly in stack traces generation. 

Yes, this changes the behavior: `clinit` would now be recorded as "method", instead of "constructor". Tracing back the uses of `get_flags`: it is used for initializing `java.lang.ClassFrameInfo.flags`. There seem to be no readers for this field in VM. Java side for `j.l.CFI` does not seem to check any method/constructor flags. So I would say this change in behavior is not really visible, and there is no need to try and keep the old (odd) behavior.

> src/hotspot/share/runtime/reflection.cpp line 772:
> 
>> 770:   assert(!method()->is_object_initializer() &&
>> 771:          (for_constant_pool_access || !method()->is_static_initializer()),
>> 772:          "should call new_constructor instead");
> 
> Nit: existing -The assert message isn't really correct

Yeah, it is a bit odd. I thought to leave the messages alone, but we can massage them as well. Should be done in new commit.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20120#discussion_r1675564908
PR Review Comment: https://git.openjdk.org/jdk/pull/20120#discussion_r1675565003