Request for review (XS): JDK-8006758: LinkResolver assertion (caused by @Contended changes)

[David Holmes]

On 24/01/2013 7:10 PM, Krystal Mo wrote:
> David,
>
> Thanks for the review. Comments inline:
>
> On 01/24/2013 04:53 PM, David Holmes wrote:
>> Hi Kris,
>>
>> On 24/01/2013 6:06 PM, Krystal Mo wrote:
>>> Hi all,
>>>
>>> Could anyone review this patch, please?
>>
>> Based on your description this patch seems to address the immediate
>> problem.
>>
> Yes. But that "immediate problem" is causing a lot of JSR 292 test
> failures, and we got a "nogo" for hs25-b16 PIT, so it's urgent to at
> least get the immediate problem fixed.

Understood - you can consider this Reviewed.

Thanks,
David
------

> There's plans to add some jtreg tests to exercise this bug specifically,
> in a separate RFE.
>
>> My concern is whether this whole "privileged" notion that @Contended
>> introduced is actually valid? Is this how the constraint that
>> @Contended can only be applied to classes on the bootclasspath was
>> implemented?
>>
>
> Yes, the predicate in this bug is how @Contended only works for code on
> BCP.
> The notion of "privileged code" mentioned by John Rose equals to "code
> on bootclasspath", or in other words "core library code".
> Traditionally there's been a lot of privilege checks in the JDK in the
> form of "x.getClass().getClassLoader() == null", and it's just the same
> here.
>
>> I'd also like to know what tests needed to be run to catch this. Maybe
>> we need to add some jsr292 tests to JPRT?
>>
> With regards to adding JSR 292 tests to JPRT, I'm open to discussions.
> We'd need more input from John and Christian.
>
> Thanks,
> Kris
>
>> Thanks,
>> David
>>
>>> It's causing a lot of failures in nightlies, so I'd like to get it
>>> reviewed quickly and push it ASAP.
>>>
>>> Webrev: http://cr.openjdk.java.net/~kmo/8006758/webrev.00/
>>> Bug: https://jbs.oracle.com/bugs/browse/JDK-8006758
>>> (There's no public bug for this yet. The description is copied below.)
>>>
>>> Bug to cover recent nightly failures with:
>>>
>>> # To suppress the following error report, specify this argument
>>> # after -XX: or in .hotspotrc: SuppressErrorAt=/linkResolver.cpp:99
>>> #
>>> # A fatal error has been detected by the Java Runtime Environment:
>>> #
>>> # Internal Error
>>> (XXX/hotspot/src/share/vm/interpreter/linkResolver.cpp:99), pid=7993,
>>> tid=2731850608
>>> # assert(resolved_method->intrinsic_id() == vmIntrinsics::_invokeBasic
>>> || resolved_method->is_compiled_lambda_form()) failed: linkMethod must
>>> return one of these
>>> #
>>> # JRE version: Java(TM) SE Runtime Environment (8.0) (build
>>> 1.8.0-internal-fastdebug-201301181518.amurillo.hs25-b16-jdk8-b74--b00)
>>> # Java VM: Java HotSpot(TM) Client VM (25.0-b16-fastdebug compiled mode,
>>> sharing linux-x86 )
>>> # Core dump written. Default location: XXX/core or core.7993
>>> #
>>> # An error report file with more information is saved as:
>>> # XXX/hs_err_pid7993.log
>>> #
>>> # If you would like to submit a bug report, please visit:
>>> # http://bugreport.sun.com/bugreport/crash.jsp
>>> #
>>>
>>> This assertion is caused by a predicate on code privilege in
>>> ClassFileParser::AnnotationCollector::annotation_index():
>>>
>>> bool privileged = false;
>>> if (loader_data->is_the_null_class_loader_data()) {
>>> // Privileged code can use all annotations. Other code silently drops
>>> some.
>>> privileged = true;
>>> }
>>>
>>> The problematic code was introduced in the @Contended change,
>>> JDK-8003985: Support @Contended Annotation - JEP 142
>>> It didn't take into account that, with recent NPG changes for 292, every
>>> anonymous class would have a dedicated ClassLoaderData which is not the
>>> null_CLD, so the condition for privileged code was not complete.
>>>
>>> The way how this code triggers the assertion far away goes like this:
>>>
>>> (1) The new MethodHandle implementation for 292 via LambdaForm uses
>>> anonymous classes to represent "compiled lambda forms".
>>> (2) The sole method in compiled lambda forms is marked with three
>>> special marker annotations: @Hidden, @Compiled and @ForceInline.
>>> (3) There's special handling in the VM to recognized these annotations
>>> while parsing the class file; but these annotations are handled
>>> specially only when the class is privileged, otherwise the special
>>> handling for them is ignored silently.
>>> (4) That information is later used to tell if a Method is a compiled
>>> lambda form.
>>>
>>> This bug introduced an incomplete predicate for code privilege in (3),
>>> silently missed the special handling, and then triggered an assertion in
>>> (4).
>>>
>>> The fix is to treat all anonymous classes as privileged code. This is
>>> assumed to be safe in an internal discussion thread.
>>>
>>> --- a/src/share/vm/classfile/classFileParser.cpp
>>> +++ b/src/share/vm/classfile/classFileParser.cpp
>>> @@ -1802,11 +1802,9 @@
>>> ClassFileParser::AnnotationCollector::annotation_index(ClassLoaderData*
>>> loader_data,
>>> Symbol* name) {
>>> vmSymbols::SID sid = vmSymbols::find_sid(name);
>>> - bool privileged = false;
>>> - if (loader_data->is_the_null_class_loader_data()) {
>>> - // Privileged code can use all annotations. Other code silently drops
>>> some.
>>> - privileged = true;
>>> - }
>>> + // Privileged code can use all annotations. Other code silently drops
>>> some.
>>> + bool privileged = loader_data->is_the_null_class_loader_data() ||
>>> + loader_data->is_anonymous();
>>> switch (sid) {
>>> case
>>> vmSymbols::VM_SYMBOL_ENUM_NAME(java_lang_invoke_ForceInline_signature):
>>> if (_location != _in_method) break; // only allow for methods
>>>
>>> Thanks,
>>> Kris
>