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

[David Holmes]

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?

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