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

[Coleen Phillimore]

This change looks good to me also.    Hopefully you've already pushed it.

On 1/24/2013 3:53 AM, 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.
>
> 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?
>
> 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?

Yes and some SA tests too, since it's very easy to break the SA (rest of 
comment deleted).

Coleen

>
> 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