Add additional version of DynComp for Java 24 - #685
Conversation
remove IntrinsicCandidate annotations clean up many UNDONEs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @java/daikon/chicory/Instrument.java:
- Around line 95-131: The file Instrument contains several commented-out type
descriptor fields (e.g., CD_Class, CD_Throwable, CD_boolean, CD_byte, CD_char,
CD_double, CD_float, CD_long, CD_short) surrounding the active descriptors
CD_Object, CD_String, CD_int, CD_void, and CD_Object_array; remove the unused
commented-out declarations to clean up the class or, if any of those types are
actually required, restore them by uncommenting and using the corresponding
symbols (e.g., CD_Class, CD_Throwable, CD_boolean) where needed. After editing,
ensure Instrument still compiles and run existing tests to confirm no behavior
changes, and update any related Javadoc/comments to reflect the retained
descriptors.
In @java/daikon/dcomp/BuildJDK24.java:
- Around line 82-84: The field inst24 currently suppresses nullness warnings
with a TODO; replace the suppression by explicitly encoding the initialization
contract: declare private static
@org.checkerframework.checker.nullness.qual.MonotonicNonNull
daikon.dcomp.Instrument24 inst24 = null and keep the assignment in main(), or
alternatively initialize inst24 in a static initializer; also add a brief
Javadoc on class BuildJDK24 explaining that main() initializes inst24 before use
and/or add an explicit runtime check (Objects.requireNonNull(inst24)) where it’s
first used to make the contract explicit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
Makefilejava/daikon/chicory/Instrument.javajava/daikon/chicory/Runtime.javajava/daikon/dcomp/BuildJDK24.javajava/daikon/dcomp/ClassGen24.javajava/daikon/dcomp/DCInstrument.javajava/daikon/dcomp/DCInstrument24.java
🧰 Additional context used
🧬 Code graph analysis (1)
java/daikon/dcomp/BuildJDK24.java (3)
java/daikon/chicory/ClassInfo.java (1)
ClassInfo(20-154)java/daikon/chicory/Runtime.java (1)
SuppressWarnings(51-1199)java/daikon/chicory/MethodInfo.java (1)
SuppressWarnings(22-277)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
🔇 Additional comments (25)
Makefile (1)
754-804: LGTM! Output suppression improves build experience.The addition of
@prefixes to suppress command echoing in update targets is a good UX improvement. These changes don't affect the logic or error handling of the commands.java/daikon/chicory/Runtime.java (3)
44-44: LGTM! Required import for new helper methods.The
InternalFormannotation import is necessary for the new conversion methods added at lines 1175-1194.
539-555: Good fix: Error messages now properly go to stderr.Routing error messages to
System.errinstead ofSystem.outfollows standard conventions and makes it easier to separate error output from normal program output.
1175-1194: LGTM! Useful utility methods with proper annotations.These conversion helpers centralize standard name format transformations. The implementations are straightforward and correctly annotated with
@BinaryNameand@InternalForm.java/daikon/chicory/Instrument.java (9)
224-230: Good improvement: More precise package checks.Adding the trailing dot to the package prefix checks (lines 224, 227) ensures that only classes actually in those packages are matched, not classes with similar prefix names.
242-257: LGTM! Better error handling in debug output.The renamed method (
writeDebugClassFiles) is clearer, and the conditional stack trace printing (lines 252-254) prevents overwhelming output when not in debug mode.
280-283: Good defensive check for null className.Adding a null check prevents potential NPEs when processing lambda-related classes that may not have conventional class names.
318-345: Improved error handling with conditional diagnostics.The error handling at parse time (318-324) and instrumentation time (339-345) now provides clear error messages while only printing stack traces when debug mode is enabled. This is a good balance between diagnostics and noise reduction.
373-395: Static field handling is correct but note architectural limitation.The code correctly stores constant static field values in
classInfo.staticMap. The comment at lines 373-374 acknowledges that this should ideally be a method of ClassInfo, but can't be due to compatibility requirements with both Instrument.java and Instrument24.java. This is a reasonable pragmatic choice.
1178-1181: Nice refactoring using ArraysPlume utility.Replacing manual array processing with
ArraysPlume.mapArraymakes the code more concise and idiomatic.
1190-1191: Better method name reflects behavior.Renaming to
create_method_info_if_instrumentedmakes it clear that this method may return null if the method should not be instrumented.
1268-1277: Variable renames improve clarity.Renaming
exit_locstoexit_line_numbers(line 1268) andlast_line_numbertoprev_line_number(line 1276) makes the code more self-documenting.
1367-1380: Better method name for clarity.Renaming
isChicorytoisChicoryClassmakes the method's purpose clearer and is more consistent with naming conventions.java/daikon/dcomp/BuildJDK24.java (7)
116-208: LGTM! Main method has good structure and error handling.The main method properly handles two modes (specific class files vs. full JDK instrumentation), uses try-with-resources for file I/O, and includes appropriate progress reporting. The separation of concerns between the two modes is clear.
210-244: Good validation logic for Java home directory.The
check_java_homemethod provides thorough validation with clear error messages. Exiting the JVM on validation failures is appropriate for a build tool.
256-283: Verify resource management pattern for JarFile.The comment at lines 261-262 states that the
JarFileis intentionally not closed because input streams are saved for later reading. However, this pattern is risky:
- The
JarFileremains open for the lifetime of the application- If the JVM exits abnormally, the file handle may leak
- The input streams may become invalid if the underlying JarFile is garbage collected
Please verify that the input streams obtained from the JarFile remain valid when used in
instrument_classes(). Consider either:
- Closing the JarFile after reading all streams (and ensuring streams are cached/buffered)
- Using try-with-resources and reading all class bytes into memory before closing
- Documenting the resource management contract more explicitly
357-443: Good resource management in instrument_classes.The method properly uses try-with-resources for InputStreams at lines 394-396 and 408-410, ensuring streams from
class_stream_mapare closed after reading. This addresses any resource leak concerns from the gather methods.
450-507: LGTM! Interface class generation uses modern APIs.The creation of synthetic interface classes (DCompMarker, DCompInstrumented, etc.) uses the ClassFile API appropriately. The version calculation at line 497 (
BcelUtil.javaVersion + 44) correctly computes the class file major version.
524-609: Well-structured instrumentation and error reporting.The
instrumentClassFilemethod includes helpful progress reporting (lines 565-571), andprint_skipped_methodseffectively separates known issues from new problems, making it easier for users to understand what needs attention.
285-349: No action needed. InputStreams created ingather_runtime_from_modules_directory()are properly closed ininstrument_classes()using try-with-resources blocks (lines 394–395 and 408–409), consistent with the jar-based approach. This pattern is intentional and handles resource cleanup correctly.Likely an incorrect or invalid review comment.
java/daikon/dcomp/ClassGen24.java (5)
20-68: Excellent class documentation and design rationale.The javadoc clearly explains the purpose of ClassGen24 as an adapter to make the ClassFile API more similar to BCEL's ClassGen, facilitating code sharing between DCInstrument.java and DCInstrument24.java. The caching strategy for frequently-accessed fields is well-justified.
76-94: Constructor properly initializes all fields.The constructor correctly caches immutable data and creates a mutable copy of the interface list (line 93) to support the
addInterface()method. The comment at lines 91-92 explains this design choice.
101-111: LGTM! Proper interface addition with duplicate check.The method correctly converts names (line 102), checks for duplicates (lines 103-107), and adds the interface via the constant pool (lines 108-110).
120-128: Method lookup is correct but uses linear search.The
containsMethodimplementation correctly searches for a method by name and descriptor. The linear search (O(n)) should be fine given that most classes have a small number of methods. If performance becomes an issue with large classes, consider caching or using a Map.
195-202: Superclass handling for Object is documented.The comments at lines 177-178 and 187-189 acknowledge that returning "java.lang.Object" for Object's superclass is "probably incorrect" but maintains consistency with BCEL's behavior. This is a reasonable choice for API compatibility, though it could be a source of bugs if not carefully handled by callers.
|
|
||
| /** "java.lang.Object". */ | ||
| private static final ObjectType CD_Object = Type.OBJECT; | ||
|
|
||
| // /** Type for "java.lang.Class". */ | ||
| // private static final ObjectType CD_Class = Type.CLASS; | ||
|
|
||
| /** Type for "java.lang.String". */ | ||
| private static final ObjectType CD_String = Type.STRING; | ||
|
|
||
| // /** Type for "java.lang.Throwable". */ | ||
| // protected static ObjectType CD_Throwable = new ObjectType("java.lang.Throwable"); | ||
| // private static final ObjectType CD_Throwable = Type.THROWABLE; | ||
|
|
||
| // /** Type for "boolean". */ | ||
| // private static final @InternedDistinct BasicType CD_boolean = Type.BOOLEAN; | ||
| // /** Type for "byte". */ | ||
| // private static final @InternedDistinct BasicType CD_byte = Type.BYTE; | ||
| // /** Type for "char". */ | ||
| // private static final @InternedDistinct BasicType CD_char = Type.CHAR; | ||
| // /** Type for "double". */ | ||
| // private static final @InternedDistinct BasicType CD_double = Type.DOUBLE; | ||
| // /** Type for "float". */ | ||
| // private static final @InternedDistinct BasicType CD_float = Type.FLOAT; | ||
| /** Type for "int". */ | ||
| private static final @InternedDistinct BasicType CD_int = Type.INT; | ||
|
|
||
| // /** Type for "long". */ | ||
| // private static final @InternedDistinct BasicType CD_long = Type.LONG; | ||
| // /** Type for "short". */ | ||
| // private static final @InternedDistinct BasicType CD_short = Type.SHORT; | ||
| /** Type for "void". */ | ||
| private static final @InternedDistinct BasicType CD_void = Type.VOID; | ||
|
|
||
| /** "java.lang.Object[]". */ | ||
| protected static Type CD_Object_array = new ArrayType(CD_Object, 1); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing commented-out type descriptors.
The active type descriptors (CD_Object, CD_String, CD_int, CD_void, CD_Object_array) are well-defined and improve code clarity. However, the commented-out descriptors (lines 99-133) should either be removed if unused, or uncommented if needed.
🤖 Prompt for AI Agents
In @java/daikon/chicory/Instrument.java around lines 95 - 131, The file
Instrument contains several commented-out type descriptor fields (e.g.,
CD_Class, CD_Throwable, CD_boolean, CD_byte, CD_char, CD_double, CD_float,
CD_long, CD_short) surrounding the active descriptors CD_Object, CD_String,
CD_int, CD_void, and CD_Object_array; remove the unused commented-out
declarations to clean up the class or, if any of those types are actually
required, restore them by uncommenting and using the corresponding symbols
(e.g., CD_Class, CD_Throwable, CD_boolean) where needed. After editing, ensure
Instrument still compiles and run existing tests to confirm no behavior changes,
and update any related Javadoc/comments to reflect the retained descriptors.
| /** Allow BuildJDK24 to access outputDebugFiles. */ | ||
| @SuppressWarnings("nullness:initialization.static.field.uninitialized") // TODO | ||
| private static daikon.dcomp.Instrument24 inst24; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Address the TODO for inst24 initialization.
The @SuppressWarnings("nullness:initialization.static.field.uninitialized") with a TODO comment suggests incomplete initialization handling. Since inst24 is initialized in main() at line 134 before use, this is currently safe, but consider documenting the initialization contract or using a different pattern to avoid the suppression.
🤖 Prompt for AI Agents
In @java/daikon/dcomp/BuildJDK24.java around lines 82 - 84, The field inst24
currently suppresses nullness warnings with a TODO; replace the suppression by
explicitly encoding the initialization contract: declare private static
@org.checkerframework.checker.nullness.qual.MonotonicNonNull
daikon.dcomp.Instrument24 inst24 = null and keep the assignment in main(), or
alternatively initialize inst24 in a static initializer; also add a brief
Javadoc on class BuildJDK24 explaining that main() initializes inst24 before use
and/or add an explicit runtime check (Objects.requireNonNull(inst24)) where it’s
first used to make the contract explicit.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@java/daikon/chicory/Runtime.java`:
- Around line 1086-1088: The comment about shadowJar troubleshooting in
Runtime.java should include a tracked-issue reference so it can be revisited;
update the note in the Runtime class (the existing shadowJar comment block) to
add a TODO with the issue tracker identifier or URL (e.g., "TODO: track
shadowJar fix in ISSUE-XXXX: <link>") and optionally include the date and owner,
so future maintainers can find and close the TODO when the shadowJar problem is
resolved.
In `@java/lib/build.gradle`:
- Line 3: The build is using Shadow plugin version "com.gradleup.shadow" 9.3.1
which requires Gradle 9.0+, so add a gradle-wrapper.properties file to pin the
Gradle distribution to 9.0 or newer and commit the wrapper files; specifically,
create/update the Gradle Wrapper configuration (so the project uses Gradle 9.0+)
and regenerate the wrapper by running the Gradle task that produces the wrapper
(e.g., the "wrapper" task) so the repo contains a gradle-wrapper.properties with
a distributionUrl pointing to a Gradle 9.0+ distribution and the corresponding
wrapper scripts/jar.
| // Eventually the code should use the library rather than copying its code. | ||
| // (As of 2026-01-10, I'm having trouble with the shadowJar plugin.) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider linking this note to a tracked issue.
The shadowJar troubleshooting note is useful; adding a TODO/issue reference would make it easier to revisit and remove once resolved.
🤖 Prompt for AI Agents
In `@java/daikon/chicory/Runtime.java` around lines 1086 - 1088, The comment about
shadowJar troubleshooting in Runtime.java should include a tracked-issue
reference so it can be revisited; update the note in the Runtime class (the
existing shadowJar comment block) to add a TODO with the issue tracker
identifier or URL (e.g., "TODO: track shadowJar fix in ISSUE-XXXX: <link>") and
optionally include the date and owner, so future maintainers can find and close
the TODO when the shadowJar problem is resolved.
| plugins { | ||
| id("java") | ||
| id("com.gradleup.shadow").version("9.2.2") | ||
| id("com.gradleup.shadow").version("9.3.1") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find gradle-wrapper.properties to check Gradle version
fd -a gradle-wrapper.properties -x cat {}Repository: codespecs/daikon
Length of output: 42
🌐 Web query:
Shadow Gradle plugin 9.3.1 minimum Gradle version requirements
💡 Result:
Shadow Gradle Plugin 9.3.1 (plugin id com.gradleup.shadow) requires Gradle 9.0 or newer (minimum). This is because Shadow 9.3.0+ has a min Gradle version of 9.0, and 9.3.1 is a patch release in that line. [1], [2]
(For completeness: Shadow 9.3.x also requires Java 17+.) [1]
🌐 Web query:
Shadow Gradle plugin 9.3.1 breaking changes release notes
💡 Result:
Shadow Gradle plugin 9.3.1 (released 2026-01-06) is a patch release with no documented breaking changes. The published release notes list only one fix:
- Fixed: “Use ASM from jdependency embedded” to avoid potential classpath conflicts with other plugins that also use ASM. [1]
If you’re upgrading to 9.3.x from earlier Shadow versions, note that Shadow 9.3.0+ requires Gradle 9.0+ and Java 17+ (this requirement is a potentially breaking upgrade constraint, but it’s not new in 9.3.1 specifically). [2]
Sources: [1] Shadow changelog (GradleUp) • [2] GradleUp/shadow README compatibility matrix
🏁 Script executed:
#!/bin/bash
# Find and read gradle-wrapper.properties explicitly
if [ -f "gradle-wrapper.properties" ]; then
cat gradle-wrapper.properties
else
# Search for it recursively
find . -name "gradle-wrapper.properties" -type f -exec cat {} \;
fiRepository: codespecs/daikon
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Also check for any gradle.properties or settings.gradle that might specify version
fd -a "gradle.properties|settings.gradle" -x sh -c 'echo "=== {} ===" && head -20 {}'Repository: codespecs/daikon
Length of output: 42
🏁 Script executed:
#!/bin/bash
# List repo structure
ls -la
# Check for common gradle locations
find . -maxdepth 3 -name "*gradle*" -type f 2>/dev/null | head -20Repository: codespecs/daikon
Length of output: 1619
🏁 Script executed:
#!/bin/bash
# Check if gradle wrapper exists and list all gradle-related files
git ls-files | grep -i gradle | head -20Repository: codespecs/daikon
Length of output: 81
🏁 Script executed:
#!/bin/bash
# Read the full build.gradle file
cat -n java/lib/build.gradleRepository: codespecs/daikon
Length of output: 807
Add gradle-wrapper.properties to enforce Gradle 9.0+ (required by Shadow 9.3.1).
Shadow 9.3.1 requires Gradle 9.0 or newer. The repo lacks a gradle-wrapper.properties file to pin the Gradle version, which could cause build failures if developers or CI systems use older Gradle versions. Add a gradle-wrapper.properties file specifying Gradle 9.0 or later to ensure build reproducibility.
(Note: Shadow 9.3.1 is a patch release with no breaking changes—only an ASM classpath conflict fix.)
🤖 Prompt for AI Agents
In `@java/lib/build.gradle` at line 3, The build is using Shadow plugin version
"com.gradleup.shadow" 9.3.1 which requires Gradle 9.0+, so add a
gradle-wrapper.properties file to pin the Gradle distribution to 9.0 or newer
and commit the wrapper files; specifically, create/update the Gradle Wrapper
configuration (so the project uses Gradle 9.0+) and regenerate the wrapper by
running the Gradle task that produces the wrapper (e.g., the "wrapper" task) so
the repo contains a gradle-wrapper.properties with a distributionUrl pointing to
a Gradle 9.0+ distribution and the corresponding wrapper scripts/jar.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
java/Makefile (1)
51-60:⚠️ Potential issue | 🟠 MajorCreate BuildJDK25 instead of relying on BuildJDK24's runtime version detection for Java 25+.
Although BuildJDK24 uses runtime version detection (
BcelUtil.javaVersion) to generate the correct classfile version (69) when run under Java 25, the current design is a temporary workaround that should be formalized. The Makefile comment "Temporary, since Java 24 is not a LTS release" is now stale—Java 25 is an LTS release. Instead of using BuildJDK24 for both Java 24 and Java 25+, create a BuildJDK25 class (following the pattern of BuildJDK24) and update the Makefile to setBuildJDKTool = BuildJDK25whenJAVA_RELEASE_NUMBER >= 25. This makes the intent explicit and avoids confusion about which build tool is responsible for which Java version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@java/Makefile` around lines 51 - 60, Makefile currently reuses BuildJDK24 for Java 25+; create a distinct BuildJDK25 and update the Makefile logic so when JAVA_RELEASE_NUMBER >= 25 it sets BUILDJDKTool = BuildJDK25 (instead of BuildJDK24). Add a new BuildJDK25 class mirroring BuildJDK24's pattern (ensuring it generates classfile version 69 when appropriate, similar to BcelUtil.javaVersion handling), and adjust the Makefile block that checks JAVA_RELEASE_NUMBER to set JAVA25 := 1 and set BuildJDKTool = BuildJDK25; keep the existing JAVA25_HOME fallback to JAVA_HOME unchanged. Ensure references to BuildJDK24 remain for Java 24, and update any comments to remove the stale "Temporary" note.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@java/Makefile`:
- Around line 51-60: Makefile currently reuses BuildJDK24 for Java 25+; create a
distinct BuildJDK25 and update the Makefile logic so when JAVA_RELEASE_NUMBER >=
25 it sets BUILDJDKTool = BuildJDK25 (instead of BuildJDK24). Add a new
BuildJDK25 class mirroring BuildJDK24's pattern (ensuring it generates classfile
version 69 when appropriate, similar to BcelUtil.javaVersion handling), and
adjust the Makefile block that checks JAVA_RELEASE_NUMBER to set JAVA25 := 1 and
set BuildJDKTool = BuildJDK25; keep the existing JAVA25_HOME fallback to
JAVA_HOME unchanged. Ensure references to BuildJDK24 remain for Java 24, and
update any comments to remove the stale "Temporary" note.
---
Duplicate comments:
In `@java/Makefile`:
- Around line 2068-2081: No changes required: the Makefile's diff24 target and
its declaration (.PHONY: diff24 and target diff24) are correct as-is; leave the
four diff commands (prefixed with '-') and the explanatory comment about
excluded files (StackMapUtils24.java, ClassGen24.java, MethodGen24.java,
OperandStack24.java) untouched.
- Around line 24-25: Add a brief documentation comment above the Makefile
variable BuildJDKTool explaining that it selects which build target to use based
on JAVA_RELEASE_NUMBER (e.g., choose "BuildJDK" for older releases and
"BuildJDK24" for Java 24), and note the expected values of JAVA_RELEASE_NUMBER
and the two possible choices so maintainers understand the variable's purpose
and usage.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
java/daikon/chicory/Instrument.java (1)
211-224:⚠️ Potential issue | 🟠 MajorKeep default boot-class exclusions when
Chicory.boot_classesis configured.
Chicory.boot_classesis documented as “extra classes,” but theelse ifchain makes it replace the defaultloader == null, parent-loader, and reflect-package exclusions. If a user sets this regex, unmatched bootstrap/system classes can be instrumented and then fail because they cannot accessdaikon.chicory.Runtime.Proposed fix
- if (Chicory.boot_classes != null) { + if (Chicory.boot_classes != null) { Matcher matcher = Chicory.boot_classes.matcher(className); if (matcher.find()) { debug_transform.log("Ignoring boot class %s, matches boot_classes regex%n", className); return true; } - } else if (loader == null) { + } + if (loader == null) { debug_transform.log("Ignoring system class %s, class loader == null%n", className); return true; } else if (loader.getParent() == null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@java/daikon/chicory/Instrument.java` around lines 211 - 224, The current else-if chain causes Chicory.boot_classes to override default boot/system exclusions; change the control flow so the default checks always run and the user-specified regex is an additional independent exclusion. Concretely, make the checks for loader == null (system class), loader.getParent() == null, and className.startsWith("sun.reflect.") unconditional (independent ifs) and then, if Chicory.boot_classes != null, run Matcher matcher = Chicory.boot_classes.matcher(className) and skip only when matcher.find(); update the debug_transform.log calls to remain the same but remove the else chaining that ties these conditions together.java/daikon/chicory/Instrument24.java (2)
547-656:⚠️ Potential issue | 🔴 CriticalDo not emit a partially built class after method instrumentation fails.
Unlike the BCEL path, this class is rebuilt from scratch. If an exception occurs mid-loop, the catch block logs and continues, leaving all remaining methods uncopied from
classBuilder;transformcan then return a class missing methods. Re-throw so the outer transform catch returns the original bytecode instead.Proposed fix
} catch (Exception e) { System.err.printf("Unexpected exception encountered: %s", e); e.printStackTrace(); + throw new RuntimeException("Failed to instrument " + classInfo.class_name, e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@java/daikon/chicory/Instrument24.java` around lines 547 - 656, The catch block around the method loop currently swallows exceptions and lets transform return a partially built class; instead, after logging include a rethrow so the outer transform can fall back to the original bytecode. Update the try/catch in Instrument24 (the block iterating methods and calling create_method_info_if_instrumented, copyMethodToOutputUnchanged, instrumentMethod and classBuilder.withMethod) to rethrow the caught Exception (or wrap it in a RuntimeException) after printing/logging, so the failure aborts class rebuilding rather than emitting a partially built class.
208-224:⚠️ Potential issue | 🟠 MajorKeep default boot-class exclusions when
Chicory.boot_classesis configured.Same issue as in
Instrument.java: the user regex is treated as a replacement for default boot/system detection. WithChicory.boot_classesset, unmatched null-loader or platform-reflection classes can be instrumented even though they cannot access the Chicory runtime.Proposed fix
- if (Chicory.boot_classes != null) { + if (Chicory.boot_classes != null) { Matcher matcher = Chicory.boot_classes.matcher(className); if (matcher.find()) { debug_transform.log("Ignoring boot class %s, matches boot_classes regex%n", className); return true; } - } else if (loader == null) { + } + if (loader == null) { debug_transform.log("Ignoring system class %s, class loader == null%n", className); return true; } else if (loader.getParent() == null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@java/daikon/chicory/Instrument24.java` around lines 208 - 224, The current logic in Instrument24.java treats Chicory.boot_classes as a replacement for the default boot/system exclusions (the if/else chain starting with "if (Chicory.boot_classes != null) {...} else if (loader == null) {...}"), causing unmatched null-loader or platform-reflection classes to be instrumented; change the flow so the user regex in Chicory.boot_classes only adds extra exclusions instead of replacing defaults: first, if Chicory.boot_classes != null, test Matcher matcher = Chicory.boot_classes.matcher(className) and return true when matcher.find(); then DO NOT use else-if — continue running the original default checks (loader == null, loader.getParent() == null, className.startsWith("sun.reflect."), className.startsWith("jdk.internal.reflect."), etc.) as independent ifs so default boot/system exclusions always apply in addition to the user-provided regex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@java/daikon/chicory/Instrument24.java`:
- Around line 888-912: The catch block that surrounds the insertion logic for
CodeElement inst currently swallows exceptions, allowing the transform to
proceed without inserting newCode (nonce initialization) while later code such
as callEnterOrExit still expects minfo.nonceLocal and the entry label mapping
(minfo.entryLabel/minfo.labelMap), which can produce invalid bytecode; change
the catch to log the exception and then fail the transform by rethrowing (e.g.,
wrap and throw a RuntimeException or a specific TransformException) so the
transformation aborts instead of continuing with inconsistent state. Ensure the
rethrow includes the original exception so stack traces are preserved and
reference the insertion context (instructions listIterator, inst, newCode,
minfo.entryLabel, and minfo.labelMap) in the log message.
---
Outside diff comments:
In `@java/daikon/chicory/Instrument.java`:
- Around line 211-224: The current else-if chain causes Chicory.boot_classes to
override default boot/system exclusions; change the control flow so the default
checks always run and the user-specified regex is an additional independent
exclusion. Concretely, make the checks for loader == null (system class),
loader.getParent() == null, and className.startsWith("sun.reflect.")
unconditional (independent ifs) and then, if Chicory.boot_classes != null, run
Matcher matcher = Chicory.boot_classes.matcher(className) and skip only when
matcher.find(); update the debug_transform.log calls to remain the same but
remove the else chaining that ties these conditions together.
In `@java/daikon/chicory/Instrument24.java`:
- Around line 547-656: The catch block around the method loop currently swallows
exceptions and lets transform return a partially built class; instead, after
logging include a rethrow so the outer transform can fall back to the original
bytecode. Update the try/catch in Instrument24 (the block iterating methods and
calling create_method_info_if_instrumented, copyMethodToOutputUnchanged,
instrumentMethod and classBuilder.withMethod) to rethrow the caught Exception
(or wrap it in a RuntimeException) after printing/logging, so the failure aborts
class rebuilding rather than emitting a partially built class.
- Around line 208-224: The current logic in Instrument24.java treats
Chicory.boot_classes as a replacement for the default boot/system exclusions
(the if/else chain starting with "if (Chicory.boot_classes != null) {...} else
if (loader == null) {...}"), causing unmatched null-loader or
platform-reflection classes to be instrumented; change the flow so the user
regex in Chicory.boot_classes only adds extra exclusions instead of replacing
defaults: first, if Chicory.boot_classes != null, test Matcher matcher =
Chicory.boot_classes.matcher(className) and return true when matcher.find();
then DO NOT use else-if — continue running the original default checks (loader
== null, loader.getParent() == null, className.startsWith("sun.reflect."),
className.startsWith("jdk.internal.reflect."), etc.) as independent ifs so
default boot/system exclusions always apply in addition to the user-provided
regex.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22bb4871-8b1f-4e43-bf87-806566e84c49
📒 Files selected for processing (11)
Makefilejava/Makefilejava/daikon/chicory/Instrument.javajava/daikon/chicory/Instrument24.javajava/daikon/config/ParameterDoclet.java11java/daikon/dcomp/BuildJDK.javajava/daikon/dcomp/Premain.javajava/daikon/suppress/NIS.javajava/daikon/test/split/SplitterFactoryTestUpdater.javajava/lib/READMEjava/lib/build.gradle
| CodeElement inst = null; | ||
| try { | ||
| ListIterator<CodeElement> li = instructions.listIterator(); | ||
| while (li.hasNext()) { | ||
| inst = li.next(); | ||
| if ((inst instanceof LineNumber) || (inst instanceof Instruction)) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Label for new location of start of original code. | ||
| debugInstrument.log("entryLabel: %s%n", minfo.entryLabel); | ||
| assert inst != null : "@AssumeAssertion(nullness): inst will always be set in loop above"; | ||
| minfo.labelMap.put(inst, minfo.entryLabel); | ||
|
|
||
| // Insert code before this LineNumber or Instruction. | ||
| // Back up iterator to point to `inst`. | ||
| li.previous(); | ||
| for (CodeElement ce : newCode) { | ||
| li.add(ce); | ||
| } | ||
| } catch (Exception e) { | ||
| System.err.printf("Exception encountered: %s", e); | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
Do not swallow entry-instrumentation insertion failures.
If this catch fires, the method can continue to exit instrumentation without the inserted nonce initialization, while callEnterOrExit still loads minfo.nonceLocal. That can generate invalid bytecode or mismatched enter/exit events; fail the transform instead.
Proposed fix
} catch (Exception e) {
System.err.printf("Exception encountered: %s", e);
e.printStackTrace();
+ throw new RuntimeException("Failed to insert method-entry instrumentation for " + mgen, e);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@java/daikon/chicory/Instrument24.java` around lines 888 - 912, The catch
block that surrounds the insertion logic for CodeElement inst currently swallows
exceptions, allowing the transform to proceed without inserting newCode (nonce
initialization) while later code such as callEnterOrExit still expects
minfo.nonceLocal and the entry label mapping (minfo.entryLabel/minfo.labelMap),
which can produce invalid bytecode; change the catch to log the exception and
then fail the transform by rethrowing (e.g., wrap and throw a RuntimeException
or a specific TransformException) so the transformation aborts instead of
continuing with inconsistent state. Ensure the rethrow includes the original
exception so stack traces are preserved and reference the insertion context
(instructions listIterator, inst, newCode, minfo.entryLabel, and minfo.labelMap)
in the log message.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
java/daikon/dcomp/DCInstrument.java (2)
1341-1344: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winIncorrect method skipping on exception.
If an exception occurs during the
MethodGenconstructor or earlier in thetryblock (e.g., line 1230), the localmgenvariable is out of scope and the fieldthis.mgenwill still point to theMethodGenfrom the previous loop iteration. Callingskip_method(mgen)will then incorrectly record the previous (already successfully processed) method as skipped, while the actual failed method goes unrecorded.Since you already have
classnameandm.getName()available in this catch block, you can record the skipped method directly and avoid relying on themgenfield completely.🐛 Proposed fix
- // TODO: Is it guaranteed that mgen is non-null by the time control reaches here? - if (mgen != null) { - skip_method(mgen); - } + skipped_methods.add(classname + "." + m.getName());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/dcomp/DCInstrument.java` around lines 1341 - 1344, Update the exception-handling skip path in DCInstrument to record the failed method directly using the available classname and m.getName() values, rather than checking or passing the stale this.mgen field to skip_method. Preserve the existing behavior of recording the method as skipped when processing fails.
488-489: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winConcurrency hazard on static collections during class transformation.
Several static collections are mutated during class instrumentation without synchronization. Since the agent can transform classes concurrently (as indicated by the use of
ConcurrentHashMapforjavaClasses), this shared mutable state causes data races, which can lead to collection corruption, incorrect instrumentation, or infinite loops (e.g. inHashMap).
java/daikon/dcomp/DCInstrument.java#L488-L489: initializejunitTestClasseswith a thread-safe set (e.g.,ConcurrentHashMap.newKeySet()).java/daikon/dcomp/DCInstrument.java#L517-L517: initializestatic_field_idwith a thread-safe map (e.g.,Collections.synchronizedMap(new LinkedHashMap<>())to preserve insertion order).java/daikon/dcomp/DCInstrument.java#L524-L524: initializeaccessFlagswith aConcurrentHashMap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/dcomp/DCInstrument.java` around lines 488 - 489, Update the static collection initializations in DCInstrument: make junitTestClasses a concurrent set, make static_field_id a synchronized map backed by LinkedHashMap to preserve insertion order, and make accessFlags a ConcurrentHashMap. Apply the changes at java/daikon/dcomp/DCInstrument.java lines 488-489, 517-517, and 524-524.java/daikon/chicory/Instrument24.java (1)
1492-1509: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix
foundLinelogic to correctly track line numbers across instructions.In both
Instrument.javaandInstrument24.java, theboolean foundLine = false;declaration is inside the instruction loop. Consequently, it only reflects whether the current iteration evaluated a line number.
- In
Instrument24.java, aReturnInstructionis not aLineNumberelement, sofoundLineis alwaysfalseat the return, making!foundLineunconditionallytrueand breaking the check entirely.- In
Instrument.java, it only checks if the return instruction has aLineNumberGentarget, failing to account for line numbers attached to immediately preceding instructions.To implement the intended logic ("Only do incremental lines if we haven't seen a line number since the last return"), move the declaration outside the loop.
java/daikon/chicory/Instrument24.java#L1492-L1509: Moveboolean foundLine = false;to be declared before thefor (CodeElement inst : il)loop, and reset it tofalseinside theif (inst instanceof ReturnInstruction)block after it is evaluated.java/daikon/chicory/Instrument.java#L1276-L1305: Moveboolean foundLine = false;to be declared before thefor (InstructionHandle ih : il)loop, and reset it tofalseinside thecase Const.ARETURN:block after it is evaluated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/chicory/Instrument24.java` around lines 1492 - 1509, Move foundLine outside the instruction loop in Instrument24.java (lines 1492-1509) and reset it after processing each ReturnInstruction; retain the existing line-number evaluation before the return check. Apply the same change in Instrument.java (lines 1276-1305): declare foundLine before the InstructionHandle loop and reset it after handling Const.ARETURN, so it tracks line numbers since the previous return in both sites.java/Makefile (1)
763-763: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare non-file targets as
.PHONY.These targets do not produce files named after themselves and should be explicitly declared
.PHONYto avoid conflicts and silence static analysis warnings.
java/Makefile#L763-L763: explicitly declaretags-nojtbas.PHONY.java/Makefile#L1288-L1288: explicitly declareapi-test-eachas.PHONY.java/Makefile#L1302-L1302: explicitly declareapi-privateas.PHONY.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/Makefile` at line 763, Declare the non-file Make targets as phony: add tags-nojtb, api-test-each, and api-private to the .PHONY declarations in java/Makefile at lines 763-763, 1288-1288, and 1302-1302 respectively.Source: Linters/SAST tools
Makefile (1)
381-381: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExclude the Java 24 instrumentation classes from the version check.
test-staged-distscans every.classunder${DISTTESTDIRJAVA}, soInstrument24.classand other Java 24 outputs will failclassfile_check_version 52. Carve those files out or allow version 68 for them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 381, Update the test-staged-dist Java class scan around classfile_check_version so Java 24 instrumentation outputs, including Instrument24.class, are excluded from the version-52 check or validated with the Java 24 classfile version 68. Keep the version-52 validation for all other compiled classes.
♻️ Duplicate comments (1)
java/daikon/chicory/Instrument24.java (1)
907-910: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winDo not swallow internal instrumentation exceptions.
Across both files, internal helper methods catch and swallow
Exceptions during bytecode manipulation instead of propagating them. If an exception is swallowed mid-instrumentation, the helper returns normally, and the top-leveltransformmethod will return a corrupted, partially instrumented class to the JVM, leading to aClassFormatErroror runtime crash.
java/daikon/chicory/Instrument24.java#L907-L910: Remove this try-catch block or wrap and re-throw as aRuntimeException.java/daikon/chicory/Instrument24.java#L477-L480: Remove this try-catch block or wrap and re-throw as aRuntimeException.java/daikon/chicory/Instrument24.java#L651-L654: Remove this try-catch block or wrap and re-throw as aRuntimeException.java/daikon/chicory/Instrument.java#L443-L446: Remove this try-catch block or wrap and re-throw as aRuntimeException.java/daikon/chicory/Instrument.java#L726-L729: Remove this try-catch block or wrap and re-throw as aRuntimeException.Propagating these exceptions ensures that the top-level
transformmethod'scatch (Throwable t)block handles the failure and safely returnsnull(leaving the class cleanly uninstrumented).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/chicory/Instrument24.java` around lines 907 - 910, Stop swallowing instrumentation exceptions in the helper catch blocks at java/daikon/chicory/Instrument24.java lines 907-910, 477-480, and 651-654, and java/daikon/chicory/Instrument.java lines 443-446 and 726-729: remove each catch or rethrow the exception as a RuntimeException. Preserve propagation to the top-level transform methods so their existing catch (Throwable t) handling returns null for failed instrumentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@java/daikon/chicory/Instrument24.java`:
- Around line 1492-1509: Move foundLine outside the instruction loop in
Instrument24.java (lines 1492-1509) and reset it after processing each
ReturnInstruction; retain the existing line-number evaluation before the return
check. Apply the same change in Instrument.java (lines 1276-1305): declare
foundLine before the InstructionHandle loop and reset it after handling
Const.ARETURN, so it tracks line numbers since the previous return in both
sites.
In `@java/daikon/dcomp/DCInstrument.java`:
- Around line 1341-1344: Update the exception-handling skip path in DCInstrument
to record the failed method directly using the available classname and
m.getName() values, rather than checking or passing the stale this.mgen field to
skip_method. Preserve the existing behavior of recording the method as skipped
when processing fails.
- Around line 488-489: Update the static collection initializations in
DCInstrument: make junitTestClasses a concurrent set, make static_field_id a
synchronized map backed by LinkedHashMap to preserve insertion order, and make
accessFlags a ConcurrentHashMap. Apply the changes at
java/daikon/dcomp/DCInstrument.java lines 488-489, 517-517, and 524-524.
In `@java/Makefile`:
- Line 763: Declare the non-file Make targets as phony: add tags-nojtb,
api-test-each, and api-private to the .PHONY declarations in java/Makefile at
lines 763-763, 1288-1288, and 1302-1302 respectively.
In `@Makefile`:
- Line 381: Update the test-staged-dist Java class scan around
classfile_check_version so Java 24 instrumentation outputs, including
Instrument24.class, are excluded from the version-52 check or validated with the
Java 24 classfile version 68. Keep the version-52 validation for all other
compiled classes.
---
Duplicate comments:
In `@java/daikon/chicory/Instrument24.java`:
- Around line 907-910: Stop swallowing instrumentation exceptions in the helper
catch blocks at java/daikon/chicory/Instrument24.java lines 907-910, 477-480,
and 651-654, and java/daikon/chicory/Instrument.java lines 443-446 and 726-729:
remove each catch or rethrow the exception as a RuntimeException. Preserve
propagation to the top-level transform methods so their existing catch
(Throwable t) handling returns null for failed instrumentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e35a3b36-c20f-499f-9732-7e33cf93417a
📒 Files selected for processing (14)
Makefilejava/Makefilejava/daikon/DynComp.javajava/daikon/chicory/Instrument.javajava/daikon/chicory/Instrument24.javajava/daikon/chicory/MethodInfo.javajava/daikon/chicory/Runtime.javajava/daikon/config/ParameterDoclet.java11java/daikon/config/ParameterDoclet.java8java/daikon/dcomp/BuildJDK.javajava/daikon/dcomp/DCInstrument.javajava/daikon/dcomp/Premain.javajava/lib/READMEjava/lib/build.gradle
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
java/daikon/dcomp/DCInstrument24.java (2)
1307-1321: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not emit a partially instrumented class after a method failure.
When
quit_if_errorisfalse, this handler records the failure and continues. The later class-element copy loop excludes everyMethodModel. For a failed method, the instrumented overload can be absent or incomplete.Other instrumented methods still decide at class scope that the target is instrumented and add
DCompMarker. Calls can then fail withNoSuchMethodError. This also removes the only emitted version of special methods such as non-JDK<clinit>methods and JUnit methods.Fall back to the complete original class, or record failed method signatures and make call-site dispatch use the uninstrumented signature.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/dcomp/DCInstrument24.java` around lines 1307 - 1321, Update the method-failure handling around the catch block and later class-element copy loop so a failed MethodModel never produces a partially instrumented class. When quit_if_error is false, either abandon instrumentation and emit the complete original class, or record failed method signatures and ensure call-site dispatch uses their uninstrumented signatures; preserve special methods such as <clinit> and JUnit methods, and only add DCompMarker when the resulting class is safely instrumented.
1421-1433: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep non-blacklisted annotations.
In
RuntimeVisibleAnnotationsAttribute, settingoutputtofalsefor a single blacklisted annotation removes the whole copy path, andmethodBuilder.with(me)only adds the original attribute unchanged. Build a filteredRuntimeVisibleAnnotationsAttribute.of(filteredAnnotations)list instead, and omit the attribute only when no entries remain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/dcomp/DCInstrument24.java` around lines 1421 - 1433, Update the RuntimeVisibleAnnotationsAttribute handling around rvaa.annotations() to collect only annotations whose className is not in BLACKLISTED_ANNOTATIONS, logging each excluded annotation. Add a filtered RuntimeVisibleAnnotationsAttribute.of result to methodBuilder when the filtered list is nonempty, and omit the attribute when all annotations are blacklisted instead of copying the original attribute unchanged.java/daikon/chicory/Instrument24.java (5)
1666-1700: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle top-level wildcard arguments individually.
The
genericPart.equals("*")special case handles only a single wildcard. For the valid signatureLjava/util/Map<Ljava/lang/String;*>;, the loop stores the first argument and leaves*incurrent. Line 1695 then callsconvertDescriptorToFqBinaryName("*"), which throws.Handle
*at depth zero as a?token during the loop.Proposed parser fix
} else if (c == '>' ) { depth--; current.append(c); + } else if (c == '*' && depth == 0) { + params.add("?"); } else if (c == ';' && depth == 0) { current.append(c); params.add(convertDescriptorToFqBinaryName(current.toString()));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/chicory/Instrument24.java` around lines 1666 - 1700, Update convertTypeArgumentsToBinaryNames to handle wildcard arguments while parsing: when the loop encounters '*' at depth zero, add "?" as an individual parameter and clear the current buffer instead of passing "*" to convertDescriptorToFqBinaryName. Preserve existing nested-generic handling and the standalone wildcard behavior.
353-367: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPublish runtime metadata only after successful class generation.
Both transformers publish
MethodInfoandClassInfobefore bytecode generation completes. Returningnullpreserves the original class bytes but leaves metadata for a class that was not installed.
java/daikon/chicory/Instrument24.java#L353-L367: deferSharedData.methods,SharedData.new_classes, andSharedData.all_classespublication untilclassFile.buildsucceeds, or roll back all mutations on failure.java/daikon/chicory/Instrument.java#L332-L346: apply the same transaction or rollback aroundinstrumentClassandcg.getJavaClass().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/chicory/Instrument24.java` around lines 353 - 367, Make runtime metadata publication transactional in java/daikon/chicory/Instrument24.java at lines 353-367: defer updates to SharedData.methods, SharedData.new_classes, and SharedData.all_classes until classFile.build completes successfully, or roll back every mutation when it fails. Apply the same transaction or rollback behavior in java/daikon/chicory/Instrument.java at lines 332-346 around instrumentClass and cg.getJavaClass(), ensuring failed transformations return null without leaving MethodInfo or ClassInfo metadata published.
456-480: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not emit a partially transformed
<clinit>.Both
addInitNotifyCallsimplementations catch insertion failures and return a method that may be incomplete. The enclosing method-processing paths must propagate the failure instead of copying or replacing that method.
java/daikon/chicory/Instrument24.java#L456-L480: propagate the exception fromaddInitNotifyCallsand prevent Lines 651-654 from continuing with partial output.java/daikon/chicory/Instrument.java#L414-L448: propagate the exception fromaddInitNotifyCallsand prevent Lines 726-729 from continuing with partial output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/chicory/Instrument24.java` around lines 456 - 480, Update addInitNotifyCalls in java/daikon/chicory/Instrument24.java (lines 456-480) and java/daikon/chicory/Instrument.java (lines 414-448) to propagate insertion failures instead of catching and returning with a partially transformed method; adjust each enclosing method-processing path so Lines 651-654 and 726-729 respectively stop processing and do not copy or replace the affected <clinit> after failure.
1204-1215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRetarget exception handlers at the original method entry.
retargetStartLabelupdatesExceptionCatch.tryStart()but notExceptionCatch.handler(). If a valid handler targets the original offset-zero label, it still points before the new entry sequence. An exception can then re-enterRuntime.enteror target the wrong code.Retarget
ExceptionCatch.handler()tominfo.entryLabelwhen it equalsminfo.oldStartLabel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/chicory/Instrument24.java` around lines 1204 - 1215, Update retargetStartLabel so the ExceptionCatch branch also checks whether ec.handler() equals minfo.oldStartLabel and rebuilds the exception catch with handler set to minfo.entryLabel, while preserving the existing tryStart retargeting behavior and other fields.
651-654: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPropagate
instrumentMethodfailures and avoid emitting incomplete transformed classes.A failure in the
classBuilder.withMethodcallback only stops the method loop. Later,instrumentClassskips remaining originalMethodModels atswitch (ce) { case MethodModel mm -> {} default -> classBuilder.with(ce); }, so methods after the failure are omitted. Do not setclassInfo.shouldInclude = truefor failed methods, and lettransformroll back shared metadata before returningnull.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/chicory/Instrument24.java` around lines 651 - 654, Update the instrumentMethod failure path in the classBuilder.withMethod callback to propagate the exception instead of continuing the method loop or marking classInfo.shouldInclude true. Ensure instrumentClass does not silently skip remaining MethodModel entries after a failure, and have transform roll back shared metadata before returning null for the failed transformation.
♻️ Duplicate comments (1)
java/daikon/chicory/Instrument24.java (1)
886-910: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winAbort when entry instrumentation cannot be inserted.
generateIncrementNoncesetsminfo.nonceLocalbefore thistry. If insertion fails, the catch logs the error and returns without addingnewCode. Exit instrumentation then loads that local. The method can contain an uninitialized local access or fail verification.Rethrow the exception. Also ensure the enclosing catch at Lines 651-654 does not continue with partial class output. This repeats the unresolved failure path from the previous review.
Proposed failure propagation
} catch (Exception e) { System.err.printf("Exception encountered: %s", e); e.printStackTrace(); + throw new IllegalStateException( + "Failed to insert method-entry instrumentation for " + mgen, e); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java/daikon/chicory/Instrument24.java` around lines 886 - 910, In the entry-instrumentation block around the CodeElement iterator, rethrow any insertion exception instead of logging and continuing, so partial instrumentation cannot proceed with an uninitialized minfo.nonceLocal. Update the enclosing catch around the caller near generateIncrementNonce to propagate the failure and stop producing partial class output rather than treating the instrumentation as successful.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@java/daikon/chicory/Instrument24.java`:
- Around line 1666-1700: Update convertTypeArgumentsToBinaryNames to handle
wildcard arguments while parsing: when the loop encounters '*' at depth zero,
add "?" as an individual parameter and clear the current buffer instead of
passing "*" to convertDescriptorToFqBinaryName. Preserve existing nested-generic
handling and the standalone wildcard behavior.
- Around line 353-367: Make runtime metadata publication transactional in
java/daikon/chicory/Instrument24.java at lines 353-367: defer updates to
SharedData.methods, SharedData.new_classes, and SharedData.all_classes until
classFile.build completes successfully, or roll back every mutation when it
fails. Apply the same transaction or rollback behavior in
java/daikon/chicory/Instrument.java at lines 332-346 around instrumentClass and
cg.getJavaClass(), ensuring failed transformations return null without leaving
MethodInfo or ClassInfo metadata published.
- Around line 456-480: Update addInitNotifyCalls in
java/daikon/chicory/Instrument24.java (lines 456-480) and
java/daikon/chicory/Instrument.java (lines 414-448) to propagate insertion
failures instead of catching and returning with a partially transformed method;
adjust each enclosing method-processing path so Lines 651-654 and 726-729
respectively stop processing and do not copy or replace the affected <clinit>
after failure.
- Around line 1204-1215: Update retargetStartLabel so the ExceptionCatch branch
also checks whether ec.handler() equals minfo.oldStartLabel and rebuilds the
exception catch with handler set to minfo.entryLabel, while preserving the
existing tryStart retargeting behavior and other fields.
- Around line 651-654: Update the instrumentMethod failure path in the
classBuilder.withMethod callback to propagate the exception instead of
continuing the method loop or marking classInfo.shouldInclude true. Ensure
instrumentClass does not silently skip remaining MethodModel entries after a
failure, and have transform roll back shared metadata before returning null for
the failed transformation.
In `@java/daikon/dcomp/DCInstrument24.java`:
- Around line 1307-1321: Update the method-failure handling around the catch
block and later class-element copy loop so a failed MethodModel never produces a
partially instrumented class. When quit_if_error is false, either abandon
instrumentation and emit the complete original class, or record failed method
signatures and ensure call-site dispatch uses their uninstrumented signatures;
preserve special methods such as <clinit> and JUnit methods, and only add
DCompMarker when the resulting class is safely instrumented.
- Around line 1421-1433: Update the RuntimeVisibleAnnotationsAttribute handling
around rvaa.annotations() to collect only annotations whose className is not in
BLACKLISTED_ANNOTATIONS, logging each excluded annotation. Add a filtered
RuntimeVisibleAnnotationsAttribute.of result to methodBuilder when the filtered
list is nonempty, and omit the attribute when all annotations are blacklisted
instead of copying the original attribute unchanged.
---
Duplicate comments:
In `@java/daikon/chicory/Instrument24.java`:
- Around line 886-910: In the entry-instrumentation block around the CodeElement
iterator, rethrow any insertion exception instead of logging and continuing, so
partial instrumentation cannot proceed with an uninitialized minfo.nonceLocal.
Update the enclosing catch around the caller near generateIncrementNonce to
propagate the failure and stop producing partial class output rather than
treating the instrumentation as successful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09962ba4-62e2-4a3e-98c6-0e093178737f
📒 Files selected for processing (6)
Makefilejava/Makefilejava/daikon/chicory/Instrument.javajava/daikon/chicory/Instrument24.javajava/daikon/dcomp/BuildJDK.javajava/daikon/dcomp/DCInstrument24.java
2 participants
No description provided.