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

[Krystal Mo]

Thank you, David.

- Kris

On 01/24/2013 05:36 PM, David Holmes wrote:
> 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
>>