8356224: JFR: Default value of @registered is ignored

egahlin · egahlin · commit 74f047b84d94 · 2025-05-11T22:40:29.000Z
Reviewed-by: mgronlun
diff --git a/src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp b/src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp
@@ -369,7 +369,7 @@ class AnnotationIterator : public StackObj {
 };
 
 static const char value_name[] = "value";
-static bool has_annotation(const InstanceKlass* ik, const Symbol* annotation_type, bool& value) {
+static bool has_annotation(const InstanceKlass* ik, const Symbol* annotation_type, bool default_value, bool& value) {
   assert(annotation_type != nullptr, "invariant");
   AnnotationArray* class_annotations = ik->class_annotations();
   if (class_annotations == nullptr) {
@@ -385,6 +385,12 @@ static bool has_annotation(const InstanceKlass* ik, const Symbol* annotation_typ
         SymbolTable::probe(value_name, sizeof value_name - 1);
       assert(value_symbol != nullptr, "invariant");
       const AnnotationElementIterator element_iterator = annotation_iterator.elements();
+      if (!element_iterator.has_next()) {
+        // Default values are not stored in the annotation element, so if the
+        // element-value pair is empty, return the default value.
+        value = default_value;
+        return true;
+      }
       while (element_iterator.has_next()) {
         element_iterator.move_to_next();
         if (value_symbol == element_iterator.name()) {
@@ -402,15 +408,15 @@ static bool has_annotation(const InstanceKlass* ik, const Symbol* annotation_typ
 // Evaluate to the value of the first found Symbol* annotation type.
 // Searching moves upwards in the klass hierarchy in order to support
 // inherited annotations in addition to the ability to override.
-static bool annotation_value(const InstanceKlass* ik, const Symbol* annotation_type, bool& value) {
+static bool annotation_value(const InstanceKlass* ik, const Symbol* annotation_type, bool default_value, bool& value) {
   assert(ik != nullptr, "invariant");
   assert(annotation_type != nullptr, "invariant");
   assert(JdkJfrEvent::is_a(ik), "invariant");
-  if (has_annotation(ik, annotation_type, value)) {
+  if (has_annotation(ik, annotation_type, default_value, value)) {
     return true;
   }
   InstanceKlass* const super = InstanceKlass::cast(ik->super());
-  return super != nullptr && JdkJfrEvent::is_a(super) ? annotation_value(super, annotation_type, value) : false;
+  return super != nullptr && JdkJfrEvent::is_a(super) ? annotation_value(super, annotation_type, default_value, value) : false;
 }
 
 static const char jdk_jfr_module_name[] = "jdk.jfr";
@@ -469,7 +475,7 @@ static bool should_register_klass(const InstanceKlass* ik, bool& untypedEventHan
   }
   assert(registered_symbol != nullptr, "invariant");
   bool value = false; // to be set by annotation_value
-  untypedEventHandler = !(annotation_value(ik, registered_symbol, value) || java_base_can_read_jdk_jfr());
+  untypedEventHandler = !(annotation_value(ik, registered_symbol, true, value) || java_base_can_read_jdk_jfr());
   return value;
 }
 
diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/ClassInspector.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/ClassInspector.java
@@ -30,6 +30,7 @@
 import java.lang.classfile.AnnotationElement;
 import java.lang.classfile.AnnotationValue;
 import java.lang.classfile.Attribute;
+import java.lang.classfile.Attributes;
 import java.lang.classfile.ClassFile;
 import java.lang.classfile.ClassModel;
 import java.lang.classfile.FieldModel;
@@ -63,6 +64,7 @@ final class ClassInspector {
     private static final ClassDesc ANNOTATION_NAME = classDesc(Name.class);
     private static final ClassDesc ANNOTATION_ENABLED = classDesc(Enabled.class);
     private static final ClassDesc ANNOTATION_REMOVE_FIELDS = classDesc(RemoveFields.class);
+    private static final String[] EMPTY_STRING_ARRAY = {};
 
     private final ClassModel classModel;
     private final Class<?> superClass;
@@ -104,12 +106,12 @@ MethodDesc findStaticCommitMethod() {
     }
 
     String getEventName() {
-        String name = annotationValue(ANNOTATION_NAME, String.class);
+        String name = annotationValue(ANNOTATION_NAME, String.class, null);
         return name == null ? getClassName() : name;
     }
 
     boolean isRegistered() {
-        Boolean result = annotationValue(ANNOTATION_REGISTERED, Boolean.class);
+        Boolean result = annotationValue(ANNOTATION_REGISTERED, Boolean.class, true);
         if (result != null) {
             return result.booleanValue();
         }
@@ -123,7 +125,7 @@ boolean isRegistered() {
     }
 
     boolean isEnabled() {
-        Boolean result = annotationValue(ANNOTATION_ENABLED, Boolean.class);
+        Boolean result = annotationValue(ANNOTATION_ENABLED, Boolean.class, true);
         if (result != null) {
             return result.booleanValue();
         }
@@ -201,52 +203,59 @@ private ImplicitFields determineImplicitFields() {
             }
         }
         ImplicitFields ifs = new ImplicitFields(superClass);
-        String[] value = annotationValue(ANNOTATION_REMOVE_FIELDS, String[].class);
+        String[] value = annotationValue(ANNOTATION_REMOVE_FIELDS, String[].class, EMPTY_STRING_ARRAY);
         if (value != null) {
             ifs.removeFields(value);
         }
         return ifs;
     }
 
-    private List<AnnotationValue> getAnnotationValues(ClassDesc classDesc) {
-        List<AnnotationValue> list = new ArrayList<>();
-        for (Attribute<?> attribute: classModel.attributes()) {
-            if (attribute instanceof RuntimeVisibleAnnotationsAttribute rvaa) {
-                for (Annotation a : rvaa.annotations()) {
-                    if (a.classSymbol().equals(classDesc) && a.elements().size() == 1) {
-                        AnnotationElement ae = a.elements().getFirst();
-                        if (ae.name().equalsString("value")) {
-                            list.add(ae.value());
-                        }
-                    }
+    private Annotation getFirstAnnotation(ClassDesc classDesc) {
+        for (RuntimeVisibleAnnotationsAttribute attribute : classModel.findAttributes(Attributes.runtimeVisibleAnnotations())) {
+            for (Annotation a : attribute.annotations()) {
+                if (a.classSymbol().equals(classDesc)) {
+                    return a;
                 }
             }
         }
-        return list;
+        return null;
     }
 
     @SuppressWarnings("unchecked")
     // Only supports String, String[] and Boolean values
-    private <T> T annotationValue(ClassDesc classDesc, Class<T> type) {
-        for (AnnotationValue a : getAnnotationValues(classDesc)) {
-            if (a instanceof AnnotationValue.OfBoolean ofb && type.equals(Boolean.class)) {
-                Boolean b = ofb.booleanValue();
-                return (T) b;
-            }
-            if (a instanceof AnnotationValue.OfString ofs && type.equals(String.class)) {
-                String s = ofs.stringValue();
-                return (T) s;
-            }
-            if (a instanceof AnnotationValue.OfArray ofa && type.equals(String[].class)) {
-                List<AnnotationValue> list = ofa.values();
-                String[] array = new String[list.size()];
-                int index = 0;
-                for (AnnotationValue av : list) {
-                    var avs = (AnnotationValue.OfString) av;
-                    array[index++] = avs.stringValue();
-                }
-                return (T) array;
+    private <T> T annotationValue(ClassDesc classDesc, Class<T> type, T defaultValue) {
+        Annotation annotation = getFirstAnnotation(classDesc);
+        if (annotation == null) {
+            return null;
+        }
+        // Default values are not stored in the annotation element, so if the
+        // element-value pair is empty, return the default value.
+        if (annotation.elements().isEmpty()) {
+            return defaultValue;
+        }
+
+        AnnotationElement ae = annotation.elements().getFirst();
+        if (!ae.name().equalsString("value")) {
+            return null;
+        }
+        AnnotationValue a = ae.value();
+        if (a instanceof AnnotationValue.OfBoolean ofb && type.equals(Boolean.class)) {
+            Boolean b = ofb.booleanValue();
+            return (T) b;
+        }
+        if (a instanceof AnnotationValue.OfString ofs && type.equals(String.class)) {
+            String s = ofs.stringValue();
+            return (T) s;
+        }
+        if (a instanceof AnnotationValue.OfArray ofa && type.equals(String[].class)) {
+            List<AnnotationValue> list = ofa.values();
+            String[] array = new String[list.size()];
+            int index = 0;
+            for (AnnotationValue av : list) {
+                var avs = (AnnotationValue.OfString) av;
+                array[index++] = avs.stringValue();
             }
+            return (T) array;
         }
         return null;
     }
diff --git a/test/jdk/jdk/jfr/api/metadata/annotations/TestOverrideWithDefaultValue.java b/test/jdk/jdk/jfr/api/metadata/annotations/TestOverrideWithDefaultValue.java
@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package jdk.jfr.api.metadata.annotations;
+
+import java.io.IOException;
+import java.util.List;
+
+import jdk.jfr.Enabled;
+import jdk.jfr.Event;
+import jdk.jfr.Recording;
+import jdk.jfr.Registered;
+import jdk.jfr.consumer.RecordedEvent;
+import jdk.test.lib.jfr.Events;
+
+/**
+ * @test
+ * @summary Tests that annotations can be overridden with the default value.
+ * @requires vm.flagless
+ * @requires vm.hasJFR
+ * @library /test/lib
+ * @run main/othervm jdk.jfr.api.metadata.annotations.TestOverrideWithDefaultValue
+ */
+public class TestOverrideWithDefaultValue {
+
+    @Enabled(false)
+    static class Mammal extends Event {
+    }
+
+    @Enabled
+    static class Cat extends Mammal {
+    }
+
+    @Registered(false)
+    static class Animal extends Event {
+    }
+
+    @Registered
+    static class Dog extends Animal {
+    }
+
+    public static void main(String[] args) throws IOException {
+        testEnabled();
+        testRegistered();
+    }
+
+    private static void testEnabled() throws IOException {
+        try (Recording r = new Recording()) {
+            r.start();
+            Cat cat = new Cat();
+            cat.commit();
+            List<RecordedEvent> events = Events.fromRecording(r);
+            Events.hasEvents(events);
+        }
+    }
+
+    private static void testRegistered() throws IOException {
+        try (Recording r = new Recording()) {
+            r.start();
+            Dog dog = new Dog();
+            dog.commit();
+            List<RecordedEvent> events = Events.fromRecording(r);
+            Events.hasEvents(events);
+        }
+    }
+}
