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

[Krystal Mo]

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