Add additional version of DynComp for Java 24 #685
Conversation
remove IntrinsicCandidate annotations clean up many UNDONEs
![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/MethodGen24.java:
- Around line 385-386: The inline comment "some need some don't. what is
difference?" in MethodGen24.java is ambiguous; remove it or replace it with a
concise, specific explanation tied to the surrounding logic that removes the
first parameter type for inner-class constructors. Update the comment near the
block that says "remove first param type so consistent with other methods" (the
code handling inner-class constructors/parameter stripping) to explicitly state
when and why the first parameter is removed (e.g., "for non-static inner-class
constructors the synthetic outer-instance parameter is removed to match other
method signatures") and delete the vague sentence.
In @java/Makefile:
- Around line 733-735: The TODO comment contains an incomplete find/remove
command (the TODO line and the commented `rm -f \`find ...\`` fragment) which
should be resolved: either complete the command so it reliably removes .class
files while pruning the listed debug paths and document its behavior in the
Makefile, or remove the TODO and commented command and instead add a tracking
issue (or reference an existing one) noting the desired cleanup; update the
Makefile by deleting the unfinished TODO block or replacing it with a finalized
command and a short explanatory comment, and if you choose to track the work,
include the issue ID in the Makefile comment for future reference.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
java/Makefilejava/daikon/chicory/MethodGen24.java
🧰 Additional context used
🪛 checkmake (0.2.2)
java/Makefile
[warning] 2140-2140: Missing required phony target "clean"
(minphony)
[warning] 2140-2140: Missing required phony target "test"
(minphony)
⏰ 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_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
🔇 Additional comments (14)
java/Makefile (4)
1465-1469: Good build system improvement with explicit dependency chain.The new explicit compilation rule for
SplitterFactoryTestUpdater.classand its addition as a dependency forSplitterFactoryTest.javaproperly captures the code generation relationship. This ensures the updater is built before being executed to generate the test file (line 1474), preventing potential build ordering issues.
770-770: Cleanup entry is consistent with other debug directory removals.The addition of
daikon/dcomp/debug/removal in thevery-cleantarget is consistent with the existing cleanup pattern (e.g., line 768 for chicory debug).
2133-2146: diff24 helper target is correctly implemented.The
diff24target is properly marked as.PHONY(line 2140) and provides a useful comparison between base and Java 24+ instrumentation files. The explanatory comment (lines 2137-2139) appropriately documents why certain utility files are excluded from the diff.Note: The checkmake warnings about missing
.PHONYdeclarations forcleanandtesttargets are unrelated to this change and were already present in the codebase.
24-25: Good abstraction for Java 24+ tool selection.The dynamic tool selection using
BuildJDKToolis a clean pattern that enables version-specific instrumentation while maintaining a single invocation point. BothBuildJDKandBuildJDK24classes implement compatible main methods, allowing the conditional variable override (defaulting toBuildJDKand switching toBuildJDK24for Java 24+) to work correctly at line 636.java/daikon/chicory/MethodGen24.java (10)
64-108: LGTM: Field organization comments added as requested.The comment markers clearly delineate MethodModel items (lines 64-85) and CodeAttribute items (lines 99-108), improving code readability and making it easier to understand which fields come from which source.
159-216: LGTM: MInfo24 class made public for instrumentation.Making this class public allows instrumentation clients to access method information during instrumentation passes. The class is well-documented and its fields are appropriately encapsulated.
225-302: LGTM: Constructor properly initializes parameter metadata.The constructor correctly initializes the new parameter-related fields by calling
getParamTypes()andgetParamNames(). UsingCollections.emptyList()(line 252) for methods without code is appropriate and efficient.
304-349: LGTM: New constructor supports instrumentation-created methods.This constructor properly initializes a MethodGen24 for methods created during instrumentation. The comment at line 340 clearly documents that
localsTablewill be populated later byfixLocals, which is the expected workflow.
396-432: LGTM: Parameter name derivation is thorough.The method correctly derives parameter names from the local variable table, handling both static and non-static methods. It appropriately accounts for 2-slot parameters (line 427) and generates placeholder names when locals are missing (line 418). The comment at lines 405-406 appropriately notes that these values may be corrected later by
fixLocals.
434-526: LGTM: fixLocals comprehensively handles local variable reconciliation.The method properly handles all the tricky cases mentioned in the documentation:
- Missing 'this' for non-static native methods (lines 452-458)
- Alignment gaps for 2-slot parameters (lines 474-491)
- Missing locals for unused parameters (lines 465-471, 492-500)
- Resorting and synchronization (lines 508-523)
The high-level invariant comment requested in past reviews is now present (lines 435-447), clearly documenting the method's postconditions.
537-567: LGTM: New method type checks are correct.The three new utility methods correctly identify constructors, class initializers, and standard main methods. The
isMaincheck properly validates all required conditions: static, void return, name "main", and exactly one String array parameter.
664-672: LGTM: setOriginalLocalVariables properly clones the input.Unlike the parameter setters, this method defensively clones the input array (line 671), preventing external mutation. This is good defensive programming.
760-767: LGTM: setInstructionList supports native method wrapping.This setter allows instrumentation to add instructions to methods, particularly for native method wrappers as documented (line 761).
769-792: LGTM: Enhanced toString produces readable method signatures.The implementation correctly renders a Java-like method signature, including proper handling of varargs (lines 783-785). It does assume
paramTypes.length == paramNames.length(line 781), which relies on the documented preconditions of the setter methods.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @java/Makefile:
- Around line 24-25: Add a brief comment explaining that the Makefile variable
BuildJDKTool selects which build tool target to use based on the Java version
(e.g., toggles between BuildJDK and BuildJDK24); insert this comment next to the
BuildJDKTool declaration so maintainers immediately understand its purpose and
that other rules reference that variable when choosing the correct JDK build
target.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/Makefile
🧰 Additional context used
🪛 checkmake (0.2.2)
java/Makefile
[warning] 2137-2137: Missing required phony target "clean"
(minphony)
[warning] 2137-2137: Missing required phony target "test"
(minphony)
⏰ 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). (12)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (3)
java/Makefile (3)
51-54: LGTM! Clean conditional logic for Java 24 support.The conditional assignment of
BuildJDKTooltoBuildJDK24follows the established pattern for version-specific handling and integrates well with the existing Java version detection logic.
2130-2143: LGTM! Well-structured diff target for Java 24 variants.The
diff24target is properly declared as.PHONYand provides a convenient way to compare the Java 24 instrumentation files with their base versions. The comment explaining excluded files is helpful for maintainers.
640-640: BuildJDK24 class is properly included in the compilation chain.The dynamic invocation
daikon.dcomp.${BuildJDKTool}is safe because BuildJDK24.java exists and is automatically compiled via the wildcard pattern in INSTRUMENTATION_JAVA_FILES on line 584. The dcomp-jdk target's dependency on dcomp_premain.jar ensures compilation completes before this command runs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @java/daikon/chicory/Instrument.java:
- Around line 1179-1181: No changes needed: the refactor of
getFullyQualifiedParameterTypes to use ArraysPlume.mapArray(Type::toString,
mgen.getArgumentTypes(), String.class) is correct and more maintainable; retain
the @SuppressWarnings("signature") annotation and ensure the method signature
and use of MethodGen and BCEL Type remain as shown.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
java/daikon/chicory/Instrument.javajava/daikon/dcomp/DCInstrument.java
⏰ 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). (12)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (7)
java/daikon/chicory/Instrument.java (7)
8-8: LGTM: Clean import additions and type descriptor introduction.The new imports (
ArraysPlume,Code,@InternedDistinct) are all used appropriately in the file. The introduction of type descriptor constants (CD_Object,CD_String,CD_int,CD_void,objectArrayCD) improves maintainability by eliminating repeated calls toType.OBJECT,Type.STRING, etc., and makes the code more readable.Also applies to: 23-23, 48-48, 92-131
248-254: LGTM: Improved error handling with conditional stack traces.The conditional stack trace printing (based on
debug_transform.enabled) is a good improvement that reduces console noise in production while still providing detailed diagnostics when debugging is enabled.Also applies to: 316-322, 336-343
240-240: LGTM: Method renames improve code clarity.The method renames are well-chosen and improve readability:
writeDebugClassFilesis more descriptive thanoutputDebugFilesaddInitNotifyCallsclarifies the purpose better thanaddInvokeToClinitisChicoryClassis more explicit thanisChicorycreate_method_info_if_instrumentedbetter describes the conditional nature of the operationAll call sites have been updated consistently.
Also applies to: 295-295, 410-410, 1190-1191
1268-1268: LGTM: Variable renames improve clarity.The variable renames enhance readability:
exit_line_numbersis more descriptive thanexit_locsprev_line_numberis clearer thanlast_line_numberabout being the previous value in the iterationAlso applies to: 1276-1276
658-661: LGTM: Past review comments properly addressed.The code changes appropriately address all three past review comments:
- Iterator contract is now documented (lines 658-661)
- Assertions verify both iterators are exhausted (lines 683-685)
- Boolean comparison now uses idiomatic style
!foundLineinstead offoundLine == false(line 1302)Also applies to: 683-685, 1302-1302
278-281: LGTM: Defensive null check improves robustness.The null check for
classNameis a good defensive programming practice that prevents potential NPEs. The comment explaining that this is "most likely a lambda-related class" provides useful context for future maintainers.
222-229: LGTM: More precise package matching prevents false positives.Adding the trailing dot to package checks (
"sun.reflect."and"jdk.internal.reflect.") ensures only classes within those packages are matched, preventing false positives from similarly-named packages likesun.reflectiveorsun.reflection.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
java/daikon/chicory/Instrument.java (3)
365-395: Constant static field capture: behavior is reasonable but narrower than Instrument24The new loop over
Fieldinstances storesfinal staticprimitive fields (BasicType) and theirConstantValuestring representations intoclassInfo.staticMap, which is useful for the runtime without touching non‑constant fields. Note that the Instrument24 version now usesformatConstantDescover allConstantValueAttributes (including strings); if you intend both instrumenters to offer the same view, you may eventually want to expand this BCEL side beyondBasicTypeor document that it only captures primitive constants.
858-868: Javadoc and return type forget_nonce_localare now misleading
get_nonce_local’s Javadoc still says “Returns null if not present” and the return type is annotated@Nullable, but the implementation now unconditionally throws anErrorif the nonce local is not found. Either update the Javadoc/annotation to reflect that it never returns null, or relax the behavior (e.g., return null and let callers decide) so the documentation remains accurate.Suggested doc/annotation tweak
- /** - * Finds the nonce local variable. Returns null if not present. - * - * @param mgen describes a method - * @return a local variable to save the nonce value, or null - */ - private @Nullable LocalVariableGen get_nonce_local(MethodGen mgen) { + /** + * Finds the nonce local variable. Throws an Error if it is not present. + * + * @param mgen describes a method + * @return a local variable to save the nonce value + */ + private LocalVariableGen get_nonce_local(MethodGen mgen) { @@ - throw new Error("Couldn't find the nonce local " + mgen); + throw new Error("Couldn't find the nonce local " + mgen);
151-195: Fix control flow inisBootClass()to preserve system class detectionThe current
if/else ifchain means that whenChicory.boot_classesis configured, system classes that don't match the regex are NOT caught by theloader == null,loader.getParent() == null,sun.reflect, andjdk.internal.reflectchecks. This changes the semantic from "extra boot classes" to "only classes matching the regex are boot classes", unexpectedly disabling built-in system class detection. Change the first block to an independentifstatement and convert remainingelse ifchecks to independentifstatements so system classes are always excluded, or update the comment to document this as intentional behavior.java/daikon/chicory/Instrument24.java (1)
381-446: Class-level instrumentation and static-constant capture are structurally sound
instrumentClassnow clearly separates concerns: it instruments methods, copies non-methodClassElements unchanged, conditionally registersclassInfoinSharedData, and then walksFieldModels to stash constant static field values viaformatConstantDescinclassInfo.staticMap. The constant-handling code will gracefully log and fall back to an empty string on resolution failures; if you’d prefer to distinguish “no value” from “empty string”, you could special-case the error path and skip inserting those entries.Possible tweak to avoid ambiguous empty-string entries
- String name = fm.fieldName().stringValue(); - String valueString = formatConstantDesc(cva.get().constant().constantValue()); - debugInstrument.log(" Constant field: %s, valueString: %s%n", name, valueString); - classInfo.staticMap.put(name, valueString); + String name = fm.fieldName().stringValue(); + String valueString = formatConstantDesc(cva.get().constant().constantValue()); + debugInstrument.log(" Constant field: %s, valueString: %s%n", name, valueString); + if (!valueString.isEmpty()) { + classInfo.staticMap.put(name, valueString); + }Also applies to: 428-440
🤖 Fix all issues with AI agents
In @java/daikon/chicory/Instrument.java:
- Around line 1180-1183: getFullyQualifiedParameterTypes currently maps BCEL
Type.toString over mgen.getArgumentTypes(), which can produce different
class-name formats than Instrument24.getFqBinaryNameParameterTypes; change it to
convert each BCEL Type using the same conversion used by the classfile path
(e.g., invoke convertDescriptorToFqBinaryName or typeToClassGetName / a shared
helper) so the returned String[] matches the PPT naming scheme used by
Instrument24.getFqBinaryNameParameterTypes; update
getFullyQualifiedParameterTypes to call that converter for each argument Type
rather than Type.toString().
In @java/daikon/test/split/SplitterFactoryTestUpdater.java:
- Around line 3-7: The generated test currently emits wildcard imports for
daikon packages on the lines that produced this file; update the generator logic
that creates imports for the SplitterFactoryTestUpdater output so it emits
explicit imports for the specific classes (Daikon, FileIO, PptMap,
daikon.split.PptSplitter, daikon.split.SplitterFactory) instead of using
wildcard imports; locate the generator routine that writes import blocks for
this test and change it to expand types into individual import statements for
those symbols.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
java/Makefilejava/daikon/chicory/Instrument.javajava/daikon/chicory/Instrument24.javajava/daikon/dcomp/DCInstrument.javajava/daikon/dcomp/DCInstrument24.javajava/daikon/test/split/SplitterFactoryTestUpdater.java
🧰 Additional context used
🧬 Code graph analysis (2)
java/daikon/test/split/SplitterFactoryTestUpdater.java (4)
java/daikon/Daikon.java (1)
Daikon(213-2786)java/daikon/FileIO.java (1)
FileIO(77-3169)java/daikon/PptMap.java (1)
PptMap(27-275)java/daikon/split/SplitterFactory.java (1)
SplitterFactory(26-336)
java/daikon/chicory/Instrument.java (1)
java/daikon/chicory/MethodInfo.java (1)
SuppressWarnings(22-277)
🪛 checkmake (0.2.2)
java/Makefile
[warning] 2137-2137: Missing required phony target "clean"
(minphony)
[warning] 2137-2137: Missing required phony target "test"
(minphony)
⏰ 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). (12)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_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 (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (14)
java/Makefile (3)
24-24: BuildJDKTool indirection looks correct and keeps 24+ logic localizedThe
BuildJDKToolindirection cleanly defaults toBuildJDKand switches toBuildJDK24whenJAVA_RELEASE_NUMBER >= 24, and the dyncomp‑jdk invocation uses it consistently (daikon.dcomp.${BuildJDKTool}). This keeps the version‑specific logic confined to the Java‑version detection block and the one call site.Also applies to: 51-54, 640-640
569-571: Explicitchicory-testprerequisite is fineMaking
chicory-testexplicitly depend ondaikon/chicory/inv_out.diffis consistent with how the rule is defined immediately below and does not change behavior.
2130-2143: diff24 helper target is well‑scoped and safeThe new
diff24section (with.PHONYand explicit Instrument/DCInstrument/BuildJDK/Chicory comparisons) is a focused developer aid and does not affect the normal build graph. Good way to keep the 24-era variants in sync.java/daikon/chicory/Instrument.java (4)
94-131: Type-descriptor refactor improves consistency with Instrument24The introduction of
CD_Object,CD_String,CD_int,CD_void, andobjectArrayCDand their consistent use throughout the file make the BCEL implementation line up with the classfile‑basedInstrument24and reduce duplication of descriptor literals. No issues here.
235-257: Centralized debug dump handling is safer under failures
writeDebugClassFiles(JavaClass c, File directory, String className)plus the updatedtransformerror handling give you a single place to manage.class/BCEL debug output and now only print stack traces whendebug_transform.enabled. On parse or instrumentation errors the transformer cleanly returnsnull, which is appropriate for a javaagent and avoids taking down the VM.Also applies to: 326-357
540-607: Exit instrumentation pipeline correctly enforces the MethodInfo iterator contractThe refactored
instrument_all_methodsplusgenerate_return_instrumentationnow drive return instrumentation viacurMethodInfo.exit_location_is_includedandcurMethodInfo.exit_locations, consuming one boolean per return and one line number only when that boolean is true, followed by assertions that both iterators are exhausted. This aligns with theMethodInfodocumentation and makes mismatches between method scanning and MethodInfo construction fail fast.Also applies to: 660-688, 764-812
1192-1239: MethodInfo construction matches documented exit-location contract
create_method_info_if_instrumentedconstructsexit_line_numbersandisIncludedsuch that there is one boolean entry for every return instruction and only those exits withtrueget an entry inexit_line_numbers. Combined with the iterator usage and post-loop assertions ininstrument_all_methods, this strictly enforces theMethodInfo.exit_locations/exit_location_is_includedinvariant and keeps Chicory’s behavior aligned with DynComp’s expectations.Also applies to: 1269-1339, 660-687
java/daikon/chicory/Instrument24.java (7)
91-105: Class-level documentation for Instrument24 is clear and accurateThe revised Javadoc succinctly describes the role of
Instrument24as a ClassFileTransformer based onjava.lang.classfile, and explains why the BCEL‑basedInstrumentis still needed for pre‑24 JDKs. No changes needed here.
232-272: Debug dump path from raw bytes is robust and failure-tolerantThe new
writeDebugClassFiles(byte[], File, String)parses the transformed bytes via BCEL only for generating human‑readable dumps, falls back cleanly if parsing fails, and guards stack traces behinddebug_transform.enabled. The transform method uses this helper both before and after instrumentation whenChicory.dumpis enabled, but always returnsnullon parse/instrumentation failure rather than propagating, which is appropriate behavior for a javaagent.Also applies to: 345-352, 369-379
531-662: Method and exit instrumentation pipeline is coherent and enforces invariantsThe classfile‑based pipeline (
instrument_all_methods→instrumentMethod→instrumentCode→instrumentInstructionList→addInstrumentationAtEntry/addInstrumentationAtExits) cleanly mirrors the BCEL version: a per‑methodMInfo24tracks locals/labels, entry instrumentation creates a nonce local and callsRuntime.enter, andaddInstrumentationAtExitsdrives exit instrumentation fromMethodInfo.exit_location_is_includedandexit_locations. The post‑loop assertions on both iterators ensure any future mismatch between MethodInfo construction and bytecode scanning is detected early.Also applies to: 758-838, 840-1000
860-951: Entry/exit code generation usingMethodGen24.MInfo24looks correct
generateIncrementNonceandcallEnterOrExitcorrectly handle static vs instance vs constructor cases, lay out the (object, nonce, method index, parameter array[, return value, line]) stack discipline expected by the Chicory runtime, and respect multi-slot primitive types viaTypeKind.slotSize(). UsingMInfo24’snonceLocal,returnLocal, andnextLocalIndexkeeps the locals table and maxLocals in sync with the instrumentation.Also applies to: 1103-1187
1363-1479: Descriptor→name helpers and MethodInfo construction are careful and match BCEL semanticsThe new helpers (
classDescToClassGetName,convertDescriptorToFqBinaryName,convertClassTypeDescriptorToFqBinaryName,convertTypeArgumentsToBinaryNames) cover primitives, arrays, and a subset of generic signatures and feed intogetFqBinaryNameParameterTypesandcreate_method_info_if_instrumented. The MethodInfo creation logic (parameter names/types, inner-class constructor handling, and exit line discovery withprev_line_numberand!foundLinehandling) mirrors the BCEL version, so Chicory’s ppt naming and exit-location contracts stay aligned across both instrumenters.Also applies to: 1480-1535
1573-1627: Utility helpers (convertDescriptorToFqBinaryName, generics handling,formatConstantDesc) are reasonable
convertDescriptorToFqBinaryNameand its helpers provide a consistent “extended binary name” format for descriptors (including arrays and a constrained subset of generics), which is appropriate for logging and ppt naming.convertTypeArgumentsToBinaryNames’s depth‑tracking approach should behave well for simple nested generics and explicitly treats a lone"*"as?, which is a pragmatic compromise.formatConstantDescusingConstantDesc.resolveConstantDescis a clean way to get actual constant values, and the try/catch boundary prevents constant‑resolution problems from breaking instrumentation.Also applies to: 1665-1700, 1709-1716
1727-1735:createLocalWithMethodScopecorrectly maintains locals table and maxLocalsCreating method‑scope locals via
LocalVariable.of(..., minfo.startLabel, minfo.endLabel), bumpingminfo.nextLocalIndexbyTypeKind.from(localType).slotSize(), and updatingmgen.setMaxLocalskeeps the locals metadata consistent with the added instrumented locals (nonce and return value). This matches how the classfile API expects locals to be described.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @java/daikon/chicory/Instrument24.java:
- Around line 550-554: The synchronized(this) block around MethodGen24
construction serializes instrumentation for all methods and may be a performance
bottleneck; determine if shared mutable state accessed during instrumentation
(e.g., classBuilder or any fields of Instrument24) requires that global lock,
and if not either remove the synchronized(this) or replace it with a narrower
lock (synchronize only the minimal shared-state updates) or switch to
per-class/per-method instrumenter instances (create a MethodGen24 per method and
confine mutable state to it) so that MethodGen24 mm/classBuilder usage is
thread-safe without blocking unrelated method instrumentation.
In @java/daikon/dcomp/StackMapUtils24.java:
- Line 189: Remove or properly gate the commented-out debug statement in
StackMapUtils24.java: the line "// DCInstrument24.debugInstrument.log("New
LocalVariableTable:%n%s%n", mgen.localsTable);" should either be deleted if no
longer needed, or restored behind a conditional debug flag (e.g., use an
existing debug/config check or a logger.isDebugEnabled() guard) so that debug
output for mgen.localsTable is produced only when intended and does not remain
as commented clutter.
- Around line 139-151: Add a defensive bounds check in addNewSpecialLocal before
accessing locals.get(newIndex - 1): verify that newIndex is <= locals.size() and
if not throw a DynCompError with a clear message referencing mgen.getClassName()
and mgen.getName() and the expected minimum entries, instructing that
fixLocals() must be called first; this will prevent an IndexOutOfBoundsException
when computing lastArg and newOffset.
- Around line 101-122: Update the Javadoc for the method that adds a new local
variable to explicitly state that it mutates mgen.localsTable in place: add a
sentence after the current description (before the @param list) saying that the
method inserts the new LocalVariable into mgen.localsTable and shifts/adjusts
subsequent entries (changing their slot numbers), so callers should expect this
side effect; keep references to MethodGen24#fixLocals and the isParam behavior
intact.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
java/daikon/chicory/Instrument24.javajava/daikon/dcomp/StackMapUtils24.java
🧰 Additional context used
🧬 Code graph analysis (1)
java/daikon/dcomp/StackMapUtils24.java (1)
java/daikon/chicory/MethodGen24.java (1)
MethodGen24(52-846)
⏰ 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). (12)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- 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_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (12)
java/daikon/dcomp/StackMapUtils24.java (5)
1-34: LGTM!The class structure follows the standard utility class pattern with a private constructor to prevent instantiation. The documentation clearly explains the purpose and relationship to the original StackMapUtils from plume-lib/bcel-util.
36-48: LGTM!Excellent documentation clarifying the terminology used throughout the class. This helps prevent confusion between 'index' (subscript into data structure) and 'offset' (slot number in stack frame).
59-99: LGTM!The method correctly adjusts slot numbers for all instruction types that reference local variables. The use of modern Java pattern matching in the switch expression is idiomatic for Java 24, and the exception handling appropriately wraps unexpected exceptions with context.
101-138: LGTM!The javadoc clearly documents the method's purpose, use cases, and preconditions. The initialization logic is straightforward.
153-192: The insertion and adjustment logic is correct.The code properly:
- Inserts the new local variable at the correct position
- Updates maxLocals and nextLocalIndex
- Adjusts parameter arrays when adding a parameter
- Updates subsequent locals' slot numbers
- Calls
adjustCodeForLocalsChangeto fix instruction referencesjava/daikon/chicory/Instrument24.java (7)
91-146: LGTM: Class structure and initialization.The class javadoc clearly explains the purpose and Java 24+ API usage. Constructor properly initializes debug logging and output directories based on Chicory configuration.
997-999: LGTM: Iterator exhaustion checks added.The assertions at the end of
addInstrumentationAtExitsproperly verify that bothshouldIncludeIterandexitLocationIterhave been fully consumed, addressing the previous review comment about sanity checking iterator usage.
1091-1187: LGTM: Enter/exit call generation is well-structured.The
callEnterOrExitmethod correctly handles:
- Static methods and constructors (pushing null for object)
- Parameter array construction with primitive wrapping
- Return value handling at exits
- Line number tracking
The dependency on
addInstrumentationAtEntryhaving created the nonce local is properly documented and enforced with@RequiresNonNull.
1753-1775: LGTM: Efficient integer constant loading.The
loadIntegerConstantmethod optimally selects between intrinsic constants (iconst_m1 through iconst_5), BIPUSH, SIPUSH, and LDC based on value range. This follows JVM best practices for constant loading efficiency.
416-427: Lock ordering is consistent throughout the codebase—no deadlock risk.The sequential synchronization on
SharedData.new_classesfollowed bySharedData.all_classesis used consistently inInstrument.javaandInstrument24.java. Elsewhere, these locks are acquired individually, never in reverse order. No deadlock scenario exists.
306-309: The proxy class detection pattern is intentional but limited to JDK proxies.The check
className.contains("/$Proxy")is consistently applied across all instrument classes and correctly targets JDK's standard proxy naming convention. However, this pattern will not detect proxies created by cglib or javassist, which use different naming schemes (e.g.,ClassName$$EnhancerBySpringCGLIB$$,ClassName_$$_javassist_). If support for such frameworks is needed in the future, additional patterns should be added.
1502-1508: Line number incrementing for synthetic exit points is sound and necessary.The code safely generates unique line numbers for multiple return instructions when bytecode lacks line number information. Each incremented line number creates a distinct program point name via
DaikonWriter.methodExitName(), ensuring no duplicate exit points. The strategy avoids conflicts with actual line numbers because incrementing only occurs when no line information is available (!foundLine), andprev_line_numberprevents incrementing after legitimate line directives are encountered. Downstream consumers handle these synthetic line numbers correctly as simple program point identifiers.
| /** | ||
| * Add a new local variable to the method. This will be inserted after any parameters and before | ||
| * any existing local variables. If there are existing local variables this will have the side | ||
| * effect of rewritting the method byte codes to adjust the slot numbers for the existing local | ||
| * variables - see below for details. | ||
| * | ||
| * <p>DCInstrument24 uses this routine for two special variables: | ||
| * | ||
| * <ol> | ||
| * <li>the dcomp marker - added as a parameter | ||
| * <li>the tag frame array - added as a local | ||
| * </ol> | ||
| * | ||
| * <p>Must call {@link MethodGen24#fixLocals} before calling this routine. | ||
| * | ||
| * @param mgen MethodGen to be modified | ||
| * @param minfo for the given method's code | ||
| * @param varName name of new parameter | ||
| * @param varType type of new parameter | ||
| * @param isParam if true, the new local is a new parameter (the DCompMarker) | ||
| * @return a LocalVariable for the new variable | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document that the method modifies mgen.localsTable in place.
The javadoc should explicitly state that this method side-effects mgen.localsTable by inserting a new local variable and adjusting subsequent entries. This is important information for callers to understand.
📝 Suggested javadoc enhancement
Add to the javadoc (after the current description, before the parameter list):
* <p>Must call {@link MethodGen24#fixLocals} before calling this routine.
+ *
+ * <p>This method modifies {@code mgen.localsTable} in place by inserting the new local
+ * variable and adjusting the slot numbers of subsequent locals. It also updates
+ * {@code mgen.maxLocals} and {@code minfo.nextLocalIndex}.
*
* @param mgen MethodGen to be modifiedNote: This addresses a previous review comment about documenting side effects.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Add a new local variable to the method. This will be inserted after any parameters and before | |
| * any existing local variables. If there are existing local variables this will have the side | |
| * effect of rewritting the method byte codes to adjust the slot numbers for the existing local | |
| * variables - see below for details. | |
| * | |
| * <p>DCInstrument24 uses this routine for two special variables: | |
| * | |
| * <ol> | |
| * <li>the dcomp marker - added as a parameter | |
| * <li>the tag frame array - added as a local | |
| * </ol> | |
| * | |
| * <p>Must call {@link MethodGen24#fixLocals} before calling this routine. | |
| * | |
| * @param mgen MethodGen to be modified | |
| * @param minfo for the given method's code | |
| * @param varName name of new parameter | |
| * @param varType type of new parameter | |
| * @param isParam if true, the new local is a new parameter (the DCompMarker) | |
| * @return a LocalVariable for the new variable | |
| */ | |
| /** | |
| * Add a new local variable to the method. This will be inserted after any parameters and before | |
| * any existing local variables. If there are existing local variables this will have the side | |
| * effect of rewritting the method byte codes to adjust the slot numbers for the existing local | |
| * variables - see below for details. | |
| * | |
| * <p>DCInstrument24 uses this routine for two special variables: | |
| * | |
| * <ol> | |
| * <li>the dcomp marker - added as a parameter | |
| * <li>the tag frame array - added as a local | |
| * </ol> | |
| * | |
| * <p>Must call {@link MethodGen24#fixLocals} before calling this routine. | |
| * | |
| * <p>This method modifies {@code mgen.localsTable} in place by inserting the new local | |
| * variable and adjusting the slot numbers of subsequent locals. It also updates | |
| * {@code mgen.maxLocals} and {@code minfo.nextLocalIndex}. | |
| * | |
| * @param mgen MethodGen to be modified | |
| * @param minfo for the given method's code | |
| * @param varName name of new parameter | |
| * @param varType type of new parameter | |
| * @param isParam if true, the new local is a new parameter (the DCompMarker) | |
| * @return a LocalVariable for the new variable | |
| */ |
🤖 Prompt for AI Agents
In @java/daikon/dcomp/StackMapUtils24.java around lines 101 - 122, Update the
Javadoc for the method that adds a new local variable to explicitly state that
it mutates mgen.localsTable in place: add a sentence after the current
description (before the @param list) saying that the method inserts the new
LocalVariable into mgen.localsTable and shifts/adjusts subsequent entries
(changing their slot numbers), so callers should expect this side effect; keep
references to MethodGen24#fixLocals and the isParam behavior intact.
| if (!mgen.isStatic()) { | ||
| // Skip the 'this' pointer. | ||
| newIndex++; | ||
| newOffset++; // size of 'this' is 1 | ||
| } | ||
|
|
||
| if (paramTypes.length > 0) { | ||
| LocalVariable lastArg; | ||
| newIndex = newIndex + paramTypes.length; | ||
| // newIndex is now positive, because paramTypes.length is | ||
| lastArg = locals.get(newIndex - 1); | ||
| newOffset = lastArg.slot() + TypeKind.from(lastArg.typeSymbol()).slotSize(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a defensive check before accessing the locals table.
While the javadoc states that fixLocals must be called before this routine (which should ensure locals has entries for all parameters), the access at line 149 could throw an IndexOutOfBoundsException if that precondition is violated. Adding an explicit check would make failures easier to diagnose:
if (newIndex > locals.size()) {
throw new DynCompError(
String.format(
"Locals table size (%d) is insufficient for method %s.%s; expected at least %d entries. "
+ "Ensure fixLocals() was called before addNewSpecialLocal().",
locals.size(), mgen.getClassName(), mgen.getName(), newIndex));
}Note: This echoes a previous review comment about adding assertions for safety.
🤖 Prompt for AI Agents
In @java/daikon/dcomp/StackMapUtils24.java around lines 139 - 151, Add a
defensive bounds check in addNewSpecialLocal before accessing
locals.get(newIndex - 1): verify that newIndex is <= locals.size() and if not
throw a DynCompError with a clear message referencing mgen.getClassName() and
mgen.getName() and the expected minimum entries, instructing that fixLocals()
must be called first; this will prevent an IndexOutOfBoundsException when
computing lastArg and newOffset.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @java/daikon/dcomp/BuildJDK24.java:
- Line 382: Fix the grammar in the inline comment in BuildJDK24.java that
currently reads "// For JDK 9+ they have not be added to class_stream_map." —
change it to proper English such as "// For JDK 9+ they have not been added to
class_stream_map." (or "// For JDK 9+ they were not added to class_stream_map.")
so the comment on the class_stream_map handling is grammatically correct.
- Around line 260-276: The current loop in BuildJDK24 leaves the JarFile open
and stores raw InputStream objects from jfile into class_stream_map, causing a
resource leak; instead, for each JarEntry read all bytes from the InputStream
into a byte[] (using a buffered read), create a new java.io.ByteArrayInputStream
from that byte[] and store that in class_stream_map, then close the original
InputStream (and after the loop close the JarFile); add import
java.io.ByteArrayInputStream and ensure you replace storing the original
InputStream with the ByteArrayInputStream so streams remain valid without
keeping the JarFile open.
- Around line 476-479: Before calling Files.write(...) ensure the target
directory exists: after building outputFile (the File constructed from destDir,
"java", "lang", and className + ".class"), create its parent directories (e.g.,
via outputFile.getParentFile().mkdirs() or
Files.createDirectories(outputFile.toPath().getParent())) and only then write
classBytes produced by finishCreateDCompClass(classBuilder, dcompInstrumented)
to avoid a failure when parent directories are missing.
In @java/daikon/dcomp/ClassGen24.java:
- Around line 112-118: Fix the typo in the Javadoc comment: change "indicatated"
to "indicated" in the Javadoc block that documents the method returning a method
by name and descriptor (the comment with @param name and @param descriptor and
@return MethodModel). Ensure the corrected word appears in the first sentence so
the comment reads "Returns the indicated method, or null if this class does not
contain it."
- Around line 203-210: The getInterfaceList() accessor in ClassGen24 exposes the
mutable field interfaceList; change it to return an unmodifiable view (e.g.,
wrap with Collections.unmodifiableList(interfaceList) or use
List.copyOf(interfaceList)) so callers cannot mutate the internal list while
preserving the current read-only behavior; update the getInterfaceList() method
to return that unmodifiable collection and keep the field name interfaceList
unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
java/daikon/dcomp/BuildJDK.javajava/daikon/dcomp/BuildJDK24.javajava/daikon/dcomp/CalcStack24.javajava/daikon/dcomp/ClassGen24.javajava/daikon/dcomp/DCInstrument.javajava/daikon/dcomp/DCInstrument24.java
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (11)
java/daikon/dcomp/BuildJDK.java (3)
56-56: LGTM! Class correctly marked asfinal.Making
BuildJDKafinalclass is appropriate since it's a utility class with a private constructor and shouldn't be subclassed.
468-468: LGTM! Correct use ofstatic finalfor immutable formatter.
DateTimeFormatteris immutable and thread-safe, so makingtimeFormatterastatic finalfield is the right approach.
489-489: LGTM! Method rename aligns with updated API.The rename from
instrument_jdk()toinstrument_jdk_class()is consistent with the broader instrumentation API changes in this PR.java/daikon/dcomp/BuildJDK24.java (1)
115-207: LGTM! Main method structure is well-organized.The main flow properly handles both explicit class file arguments and full JDK instrumentation, with appropriate error handling and progress reporting.
java/daikon/dcomp/ClassGen24.java (1)
29-93: LGTM! Well-structured class with appropriate BCEL analogy.The class effectively mirrors BCEL's
ClassGenusing Java 24'sjava.lang.classfileAPIs. Fields are properlyfinal, and the caching ofClassModeldata improves performance and aligns with BCEL's design patterns.java/daikon/dcomp/CalcStack24.java (6)
59-67: LGTM! Well-designed utility class with clear sentinel.The class is properly
finalwith a private throwing constructor to prevent instantiation. TheNULL_CDsentinel is clearly documented as a marker for null values on the operand stack.
85-134: LGTM! Clean CodeElement dispatch with proper Label handling.The
simulateCodeElementmethod correctly handles Labels by caching/validating stack states, which is essential for control flow analysis. The worklist pattern for divergent control flow is well-implemented.
148-160: LGTM! Error message now includes instruction index for debugging.The error message improvement (adding
instIndex) addresses past feedback and will make debugging easier.
714-716: LGTM! JSR/RET handling correctly removed.Since JSR and RET have been illegal since JDK 7 (class file version 51.0), and this code targets Java 24+, falling through to the default error case is the correct approach.
922-955: LGTM! Clean helper for LoadableConstantEntry → ClassDesc mapping.The
lceToCDmethod correctly handles allLoadableConstantEntryvariants including the critical fix forClassEntry(returningCD_Classinstead of the described class).
964-977: LGTM! Clean switch expression for primitive array element types.The
npaiToElementTypeDescriptormethod uses Java's enhanced switch expression effectively, making the code concise and readable.
| // We intentionally do not close the jar file as we save | ||
| // input streams into it to be read later. | ||
| try { | ||
| JarFile jfile = new JarFile(jar_name); | ||
| // Get each class to be instrumented and store it away | ||
| Enumeration<JarEntry> entries = jfile.entries(); | ||
| while (entries.hasMoreElements()) { | ||
| JarEntry entry = entries.nextElement(); | ||
| // System.out.printf("processing entry %s%n", entry); | ||
| final String entryName = entry.getName(); | ||
| if (entryName.endsWith("/") || entryName.endsWith("~")) { | ||
| continue; | ||
| } | ||
|
|
||
| // Get the InputStream for this file | ||
| InputStream is = jfile.getInputStream(entry); | ||
| class_stream_map.put(entryName, is); |
There was a problem hiding this comment.
Critical: InputStreams become invalid after JarFile is intentionally left open.
The comment on lines 260-261 says the JarFile is intentionally not closed to keep InputStreams valid. However, this differs from BuildJDK.java which has the same pattern but also doesn't close the JarFile.
While this works, it's a resource leak. A past review suggested reading bytes into memory:
- // Get the InputStream for this file
- InputStream is = jfile.getInputStream(entry);
- class_stream_map.put(entryName, is);
+ // Read the bytes into memory so we can close the JarFile
+ try (InputStream is = jfile.getInputStream(entry)) {
+ byte[] bytes = is.readAllBytes();
+ class_stream_map.put(entryName, new ByteArrayInputStream(bytes));
+ }Add import java.io.ByteArrayInputStream; if applying this fix.
🤖 Prompt for AI Agents
In @java/daikon/dcomp/BuildJDK24.java around lines 260 - 276, The current loop
in BuildJDK24 leaves the JarFile open and stores raw InputStream objects from
jfile into class_stream_map, causing a resource leak; instead, for each JarEntry
read all bytes from the InputStream into a byte[] (using a buffered read),
create a new java.io.ByteArrayInputStream from that byte[] and store that in
class_stream_map, then close the original InputStream (and after the loop close
the JarFile); add import java.io.ByteArrayInputStream and ensure you replace
storing the original InputStream with the ByteArrayInputStream so streams remain
valid without keeping the JarFile open.
| } else { | ||
| String entryName = path.toString().substring(modulePrefixLength + 1); | ||
| if (!entryName.endsWith(".class") || entryName.equals("java/lang/Object.class")) { | ||
| // For JDK 9+ we do not process non-.class files and Object.class. | ||
| return; | ||
| } | ||
| // System.out.printf("processing entry %s%n", entryName); | ||
| try { | ||
| // Get the InputStream for this file | ||
| InputStream is = Files.newInputStream(path); | ||
| class_stream_map.put(entryName, is); | ||
| } catch (Exception e) { | ||
| throw new Error(e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Resource leak: module-info.class streams not closed.
The filter on line 335 excludes non-.class files and Object.class, but module-info.class passes through because it ends with .class. Later at lines 376-378, it's skipped without closing the InputStream.
🔧 Proposed fix - filter out module-info.class at source
String entryName = path.toString().substring(modulePrefixLength + 1);
- if (!entryName.endsWith(".class") || entryName.equals("java/lang/Object.class")) {
+ if (!entryName.endsWith(".class")
+ || entryName.equals("java/lang/Object.class")
+ || entryName.equals("module-info.class")) {
// For JDK 9+ we do not process non-.class files and Object.class.
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| String entryName = path.toString().substring(modulePrefixLength + 1); | |
| if (!entryName.endsWith(".class") || entryName.equals("java/lang/Object.class")) { | |
| // For JDK 9+ we do not process non-.class files and Object.class. | |
| return; | |
| } | |
| // System.out.printf("processing entry %s%n", entryName); | |
| try { | |
| // Get the InputStream for this file | |
| InputStream is = Files.newInputStream(path); | |
| class_stream_map.put(entryName, is); | |
| } catch (Exception e) { | |
| throw new Error(e); | |
| } | |
| } | |
| } | |
| } else { | |
| String entryName = path.toString().substring(modulePrefixLength + 1); | |
| if (!entryName.endsWith(".class") | |
| || entryName.equals("java/lang/Object.class") | |
| || entryName.equals("module-info.class")) { | |
| // For JDK 9+ we do not process non-.class files and Object.class. | |
| return; | |
| } | |
| // System.out.printf("processing entry %s%n", entryName); | |
| try { | |
| // Get the InputStream for this file | |
| InputStream is = Files.newInputStream(path); | |
| class_stream_map.put(entryName, is); | |
| } catch (Exception e) { | |
| throw new Error(e); | |
| } | |
| } | |
| } |
| classBuilder -> finishCreateDCompClass(classBuilder, dcompInstrumented)); | ||
| // Write the byte array to a .class file. | ||
| File outputFile = new File(new File(new File(destDir, "java"), "lang"), className + ".class"); | ||
| Files.write(outputFile.toPath(), classBytes); |
There was a problem hiding this comment.
Ensure parent directories exist before writing class file.
Files.write() will fail if the parent directory doesn't exist. The destination directory structure should be created first.
🔧 Proposed fix
// Write the byte array to a .class file.
File outputFile = new File(new File(new File(destDir, "java"), "lang"), className + ".class");
+ File parent = outputFile.getParentFile();
+ if (parent != null) {
+ parent.mkdirs();
+ }
Files.write(outputFile.toPath(), classBytes);🤖 Prompt for AI Agents
In @java/daikon/dcomp/BuildJDK24.java around lines 476 - 479, Before calling
Files.write(...) ensure the target directory exists: after building outputFile
(the File constructed from destDir, "java", "lang", and className + ".class"),
create its parent directories (e.g., via outputFile.getParentFile().mkdirs() or
Files.createDirectories(outputFile.toPath().getParent())) and only then write
classBytes produced by finishCreateDCompClass(classBuilder, dcompInstrumented)
to avoid a failure when parent directories are missing.
| /** | ||
| * Returns the interfaces of this class. | ||
| * | ||
| * @return the interfaces of this class | ||
| */ | ||
| public List<ClassEntry> getInterfaceList() { | ||
| return interfaceList; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider returning an unmodifiable view from getInterfaceList().
The method returns the mutable internal list directly. While current callers only read from it, returning an unmodifiable view would prevent accidental modifications.
+import java.util.Collections;
...
public List<ClassEntry> getInterfaceList() {
- return interfaceList;
+ return Collections.unmodifiableList(interfaceList);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Returns the interfaces of this class. | |
| * | |
| * @return the interfaces of this class | |
| */ | |
| public List<ClassEntry> getInterfaceList() { | |
| return interfaceList; | |
| } | |
| /** | |
| * Returns the interfaces of this class. | |
| * | |
| * @return the interfaces of this class | |
| */ | |
| public List<ClassEntry> getInterfaceList() { | |
| return Collections.unmodifiableList(interfaceList); | |
| } |
🤖 Prompt for AI Agents
In @java/daikon/dcomp/ClassGen24.java around lines 203 - 210, The
getInterfaceList() accessor in ClassGen24 exposes the mutable field
interfaceList; change it to return an unmodifiable view (e.g., wrap with
Collections.unmodifiableList(interfaceList) or use List.copyOf(interfaceList))
so callers cannot mutate the internal list while preserving the current
read-only behavior; update the getInterfaceList() method to return that
unmodifiable collection and keep the field name interfaceList unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @java/daikon/dcomp/BuildJDK24.java:
- Around line 516-520: Update the Javadoc for the parameter classFileName in
BuildJDK24 (the method that takes ClassLoader loader, File outputDir, String
classFileName, int classTotal) to explicitly state that classFileName is a file
name in internal form that includes the “.class” extension (for example
"java/lang/String.class") and that the code converts it to a binary class name
(slashes -> dots, removing the ".class") before instrumentation.
- Around line 78-79: Update the comment for the static_field_id_filename
constant in class BuildJDK24 to explain what the "static-fields map" contains
and add a reference (URL, Javadoc, or internal doc name) to the
design/specification that documents the map; edit the comment above private
static String static_field_id_filename = "dcomp_jdk_static_field_id" to include
a short description of the map's purpose and a link or tag to the authoritative
documentation.
In @java/Makefile:
- Around line 24-25: Add a brief inline comment above the BuildJDKTool variable
explaining its purpose: that BuildJDKTool selects either BuildJDK or BuildJDK24
depending on the Java runtime version (JAVA_RELEASE_NUMBER) so downstream
targets can use a single name; reference the conditional logic that changes
BuildJDKTool to BuildJDK24 when JAVA_RELEASE_NUMBER indicates Java 24+. Ensure
the comment mentions BuildJDKTool, BuildJDK, BuildJDK24, and JAVA_RELEASE_NUMBER
for clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
java/Makefilejava/daikon/dcomp/BuildJDK24.java
🧰 Additional context used
🪛 checkmake (0.2.2)
java/Makefile
[warning] 2083-2083: Missing required phony target "clean"
(minphony)
⏰ 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). (12)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (8)
java/Makefile (4)
533-535: LGTM! Explicit dependency improves build reliability.Adding the dependency on
daikon/Daikon.classensures the Daikon codebase is compiled before buildingChicoryPremain.jar, which prevents potential build failures.
537-539: LGTM! Test dependency chain is correct.The updated dependency ensures
ChicoryPremain.jaris built beforechicory-testruns, maintaining proper build ordering.
606-606: LGTM! Dynamic tool selection enables Java 24+ support.The use of
${BuildJDKTool}allows the build system to automatically selectBuildJDK24when running on Java 24+, fulfilling the PR's objective of adding Java 24 support.
2076-2089: LGTM! Helpful diff target for tracking Java 24 changes.The
diff24target provides a convenient way to compare the original instrumentation files with their Java 24 counterparts, which is useful during development and maintenance. The.PHONYdeclaration is correctly added.java/daikon/dcomp/BuildJDK24.java (4)
203-204: Should BuildJDK24 fail when methods are skipped?A past reviewer asked whether the build should crash by default when methods cannot be instrumented, with an optional flag to allow skipping. Currently, it prints warnings and continues. This could mask instrumentation failures that cause runtime issues.
Consider adding a
--strictflag that fails the build whenskipped_methodsis non-empty, or making that the default behavior with a--allow-skippedoverride.As noted by a past reviewer: "Should this crash by default if there are any skipped methods, possibly with an override for use during debugging or to work around bugs?"
114-207: LGTM! Well-structured main method with clear control flow.The main method properly:
- Parses options using the Options framework
- Handles both test mode (specific class files) and production mode (full JDK instrumentation)
- Restores/saves the static-field map appropriately
- Provides helpful output and progress information
The separation of concerns between gathering class streams and instrumenting them is clean.
507-569: LGTM! Good progress reporting and error handling.The instrumentation flow properly:
- Uses a correctly-declared
static final DateTimeFormatter(addressing a past review)- Writes debug output when requested
- Provides progress updates every 100 files with timestamps
- Handles directory creation for output files
- Accumulates skipped methods for later reporting
The progress display will be helpful for monitoring long-running JDK instrumentation jobs.
571-606: LGTM! Helpful error reporting distinguishes known from unknown issues.The
print_skipped_methods()implementation:
- Clearly explains the impact of skipped methods
- Separates known issues from new problems
- Provides actionable guidance (report unknown issues, ignore known ones)
- Uses appropriate error output stream
This will help users understand when they need to report problems versus when issues are already known.
| /** Name of file in output jar containing the static-fields map. */ | ||
| private static String static_field_id_filename = "dcomp_jdk_static_field_id"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add reference to static-fields map documentation.
The comment describes that this file contains "the static-fields map" but doesn't explain what that map is or reference where it's documented. As noted in a past review, adding a reference would help maintainers understand this component's purpose.
- /** Name of file in output jar containing the static-fields map. */
+ /** Name of file in output jar containing the static-fields map. See [link to doc or class that explains the map format/purpose]. */
private static String static_field_id_filename = "dcomp_jdk_static_field_id";Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @java/daikon/dcomp/BuildJDK24.java around lines 78 - 79, Update the comment
for the static_field_id_filename constant in class BuildJDK24 to explain what
the "static-fields map" contains and add a reference (URL, Javadoc, or internal
doc name) to the design/specification that documents the map; edit the comment
above private static String static_field_id_filename =
"dcomp_jdk_static_field_id" to include a short description of the map's purpose
and a link or tag to the authoritative documentation.
| BuildJDKTool = BuildJDK | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a documentation comment for BuildJDKTool.
The BuildJDKTool variable dynamically selects between BuildJDK and BuildJDK24 based on the Java version, but this purpose isn't immediately clear without reading the conditional logic below. A brief inline comment would help maintainers quickly understand its role.
# Selects BuildJDK or BuildJDK24 based on JAVA_RELEASE_NUMBER
BuildJDKTool = BuildJDK🤖 Prompt for AI Agents
In @java/Makefile around lines 24 - 25, Add a brief inline comment above the
BuildJDKTool variable explaining its purpose: that BuildJDKTool selects either
BuildJDK or BuildJDK24 depending on the Java runtime version
(JAVA_RELEASE_NUMBER) so downstream targets can use a single name; reference the
conditional logic that changes BuildJDKTool to BuildJDK24 when
JAVA_RELEASE_NUMBER indicates Java 24+. Ensure the comment mentions
BuildJDKTool, BuildJDK, BuildJDK24, and JAVA_RELEASE_NUMBER for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @java/daikon/chicory/Instrument.java:
- Around line 1180-1183: The two methods getFullyQualifiedParameterTypes and
getFqBinaryNameParameterTypes produce the same @BinaryName output via different
code paths (Type::toString vs convertDescriptorToFqBinaryName), so extract a
shared utility that converts a BCEL Type or a method/field descriptor into the
canonical FQ binary-name string and use that helper from both Instrument.java
and Instrument24.java (replace ArraysPlume.mapArray(Type::toString, ...) and
convertDescriptorToFqBinaryName usages to call the new utility); keep the
@BinaryName contract on the public-facing methods and ensure the helper accepts
both a Type and a descriptor string (or provide two thin overloads that delegate
to a single implementation) so both BCEL API variants reuse identical formatting
logic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
java/daikon/chicory/Instrument.javajava/daikon/chicory/Instrument24.javajava/daikon/dcomp/DCInstrument.javajava/daikon/dcomp/DCInstrument24.java
🧰 Additional context used
🧬 Code graph analysis (1)
java/daikon/chicory/Instrument24.java (1)
java/daikon/dcomp/Instrument24.java (1)
Instrument24(40-375)
⏰ 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). (12)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (10)
java/daikon/chicory/Instrument.java (4)
94-131: Type descriptor declarations look good.The introduction of type descriptor constants (CD_Object, CD_String, CD_int, CD_void, objectArrayCD) improves code readability and consistency. The commented-out descriptors provide useful reference without cluttering the compiled code.
224-227: Boot class package checks are correct.The use of trailing dots in
"sun.reflect."and"jdk.internal.reflect."correctly prevents false positives from classes that merely start with those prefixes but aren't in those packages.
372-395: Constant field storage refactoring is sound.The refactored logic for storing constant static fields handles null values correctly and only populates
classInfo.staticMapwith valid constant values. The intermediate variables (constantValue,valueString) improve code clarity.
242-257: Method renames and error handling improvements look good.The renamed methods (
writeDebugClassFiles,addInitNotifyCalls,isChicoryClass,create_method_info_if_instrumented) improve code clarity. The enhanced error handling with conditional stack traces (controlled bydebug_transform.enabled) provides better debugging capabilities without overwhelming production logs.Also applies to: 412-447, 1192-1341, 1369-1382
java/daikon/chicory/Instrument24.java (6)
3-8: Type descriptor imports and declarations are well-organized.The use of static imports for type descriptors from
java.lang.constant.ConstantDescs(CD_Object, CD_String, CD_int, CD_void) and access flags improves code readability. This approach is consistent with modern Java 24 classfile API usage.Also applies to: 107-111
840-911: Instrumentation entry point insertion logic is sound.The
instrumentInstructionListmethod provides a clean separation between entry and exit instrumentation. TheaddInstrumentationAtEntrymethod correctly identifies the insertion point (after local variable declarations and the initial label, but before the first line number or instruction) and usesListIteratorfor precise insertion. TheentryLabelsetup properly handles label retargeting for any instructions that originally referenced bytecode offset 0.
952-1000: Exit instrumentation with iterator contract validation is excellent.The
addInstrumentationAtExitsmethod correctly implements the iterator contract: exactly one boolean per return instruction inshouldIncludeIter, and an integer inexitLocationIteronly when that boolean is true. The assertions at lines 997-999 provide valuable runtime validation that this contract is maintained, catching any potential mismatches betweencreate_method_info_if_instrumentedand the actual code scan.
1561-1701: Descriptor conversion methods handle complex type scenarios correctly.The
convertDescriptorToFqBinaryNameand related methods comprehensively handle primitives, object types, arrays, and generic type parameters. The implementation correctly:
- Converts primitive type descriptors to their Java names (lines 1587-1622)
- Handles array dimensions (lines 1579-1626)
- Processes generic type arguments with proper depth tracking (lines 1674-1693)
- Replaces '/' with '.' for proper binary name format
This custom conversion ensures consistent type name representation in the classfile-based instrumentation path.
1703-1777: Utility methods for constant handling and local variable creation are well-implemented.The
formatConstantDescmethod appropriately usesMethodHandles.lookup()to resolve constant values for storage. ThecreateLocalWithMethodScopemethod correctly manages local variable table updates and adjustsmaxLocalsto account for the new variable's slot usage. TheloadIntegerConstantandbuildLDCInstructionhelpers optimize bytecode generation by selecting the most compact instruction form based on the value or constant pool index.
232-272: Java 24 classfile API adaptations are well-executed.The adaptations for Java 24's classfile API demonstrate good understanding of the new API:
writeDebugClassFilescorrectly handles byte array input and usesFiles.writefor output- The ClassElement and MethodElement copying logic uses modern switch expressions effectively
copyMethodToOutputUnchangedandcopyMethodhelpers provide clean abstractions for unchanged method copying- Label retargeting logic comprehensively handles all instruction types that can reference labels (branches, exception handlers, switches)
The
ModifiedSwitchInforecord (line 1191) provides a clean data structure for passing switch instruction modifications.Also applies to: 407-446, 663-699, 1190-1272
| @SuppressWarnings("signature") // BCEL is not annotated | ||
| private @BinaryName String[] getFullyQualifiedParameterTypes(MethodGen mgen) { | ||
|
|
||
| Type[] paramTypes = mgen.getArgumentTypes(); | ||
| @BinaryName String[] param_type_strings = new @BinaryName String[paramTypes.length]; | ||
|
|
||
| for (int ii = 0; ii < paramTypes.length; ii++) { | ||
| Type t = paramTypes[ii]; | ||
| /*if (t instanceof ObjectType) | ||
| param_type_strings[ii] = ((ObjectType) t).getClassName(); | ||
| else { | ||
| param_type_strings[ii] = t.getSignature().replace('/', '.'); | ||
| } | ||
| */ | ||
| param_type_strings[ii] = t.toString(); | ||
| } | ||
|
|
||
| return param_type_strings; | ||
| return ArraysPlume.mapArray(Type::toString, mgen.getArgumentTypes(), String.class); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of getFullyQualifiedParameterTypes and getFqBinaryNameParameterTypes
# to understand if naming consistency is critical for correctness.
echo "=== Usages of getFullyQualifiedParameterTypes in Instrument.java ==="
rg -n "getFullyQualifiedParameterTypes" --type java
echo ""
echo "=== Usages of getFqBinaryNameParameterTypes in Instrument24.java ==="
rg -n "getFqBinaryNameParameterTypes" --type java
echo ""
echo "=== Check if DaikonWriter expects specific format ==="
rg -n "methodEntryName|methodExitName" --type java -A 3Repository: codespecs/daikon
Length of output: 9620
🏁 Script executed:
#!/bin/bash
# Get the implementations of both methods to compare their output formats
echo "=== getFullyQualifiedParameterTypes implementation (Instrument.java) ==="
sed -n '1180,1183p' java/daikon/chicory/Instrument.java
echo ""
echo "=== getFqBinaryNameParameterTypes implementation (Instrument24.java) ==="
sed -n '1381,1386p' java/daikon/chicory/Instrument24.java
echo ""
echo "=== convertDescriptorToFqBinaryName implementation (Instrument24.java) ==="
rg -A 15 "private static String convertDescriptorToFqBinaryName" java/daikon/chicory/Instrument24.java
echo ""
echo "=== Check how Type::toString is used in BCEL context ==="
rg -B 2 -A 2 "Type::toString" java/daikon/chicory/Instrument.java
echo ""
echo "=== Check if @BinaryName annotation reveals the expected format ==="
rg -B 3 "@BinaryName" java/daikon/chicory/Instrument.java | head -30Repository: codespecs/daikon
Length of output: 2049
🏁 Script executed:
#!/bin/bash
# Search more carefully for convertDescriptorToFqBinaryName
echo "=== Search for convertDescriptorToFqBinaryName definition ==="
rg -n "convertDescriptorToFqBinaryName" java/daikon/chicory/Instrument24.java
echo ""
echo "=== Get the actual implementation ==="
rg -B 2 -A 20 "static.*convertDescriptorToFqBinaryName" java/daikon/chicory/Instrument24.java
echo ""
echo "=== Check if there's a similar method in Instrument.java ==="
rg -n "convertDescriptor\|BinaryName\|descriptorString\|Signatures" java/daikon/chicory/Instrument.java | head -20
echo ""
echo "=== Check Signatures class usage for format conversion ==="
rg -n "Signatures\." java/daikon/chicory/Instrument.java | head -10
</parameter>Repository: codespecs/daikon
Length of output: 1905
🏁 Script executed:
#!/bin/bash
# Get the full implementation of convertDescriptorToFqBinaryName
echo "=== Full convertDescriptorToFqBinaryName implementation ==="
sed -n '1575,1620p' java/daikon/chicory/Instrument24.java
echo ""
echo "=== Check what Type.toString() produces in BCEL ==="
rg -n "class Type\|public.*toString" java/daikon/ --type java -A 2 | grep -A 5 "Type.*toString"
echo ""
echo "=== Check if there's documentation about BCEL Type.toString format ==="
rg -n "BCEL\|Type\\.toString" java/daikon/chicory/Instrument.java -B 2 -A 2
</parameter>Repository: codespecs/daikon
Length of output: 1683
🏁 Script executed:
#!/bin/bash
# Get the full implementation of convertDescriptorToFqBinaryName
sed -n '1575,1640p' java/daikon/chicory/Instrument24.java
echo ""
echo "=== Check what format Type.toString actually returns by looking at usage context ==="
# Look at the BCEL library to understand Type.toString
find . -name "*.java" -type f | xargs grep -l "class Type" | head -5
echo ""
echo "=== Check comments around getFullyQualifiedParameterTypes ==="
sed -n '1175,1185p' java/daikon/chicory/Instrument.java
echo ""
echo "=== Check comments around getFqBinaryNameParameterTypes ==="
sed -n '1375,1390p' java/daikon/chicory/Instrument24.javaRepository: codespecs/daikon
Length of output: 3459
🏁 Script executed:
#!/bin/bash
# Let's check if there are any tests or documentation about what Type.toString produces
echo "=== Search for Type.toString usage in test or doc context ==="
rg -B 3 -A 3 "Type\\.toString\|Type::toString" --type java
echo ""
echo "=== Check if there's any test comparing formats ==="
rg -n "getFullyQualifiedParameterTypes\|getFqBinaryNameParameterTypes" java/daikon/ --type java -B 3 -A 3 | head -40
echo ""
echo "=== Check what convertClassTypeDescriptorToFqBinaryName produces for binary names ==="
sed -n '1620,1660p' java/daikon/chicory/Instrument24.javaRepository: codespecs/daikon
Length of output: 1722
🏁 Script executed:
#!/bin/bash
# Let's check for any runtime tests or integration tests that might reveal format issues
echo "=== Search for tests using these methods ==="
fd -e java -path "*test*" | xargs grep -l "getFullyQualifiedParameterTypes\|getFqBinaryNameParameterTypes" 2>/dev/null
echo ""
echo "=== Check if there are any format comparisons or assertions ==="
rg -n "assertEquals.*Type\|assert.*toString\|format.*Binary" --type java java/daikon/chicory/
echo ""
echo "=== Look for any known issues or TODOs about type formatting ==="
rg -n "TODO\|FIXME\|XXX\|HACK" java/daikon/chicory/Instrument.java | grep -i "type\|format\|binary"
echo ""
echo "=== Check BCEL documentation or comments about Type class ==="
rg -B 5 "import.*Type\|BCEL" java/daikon/chicory/Instrument.java | head -30Repository: codespecs/daikon
Length of output: 484
🏁 Script executed:
#!/bin/bash
# Let's check BCEL documentation or comments about Type class
echo "=== Check BCEL imports and Type usage in Instrument.java ==="
head -100 java/daikon/chicory/Instrument.java | grep -i "import\|bcel"
echo ""
echo "=== Check if there are comments explaining the Type format ==="
rg -B 10 "Type\\.toString\|BCEL is not annotated" java/daikon/chicory/Instrument.java | head -30
echo ""
echo "=== Search for any comparison or discussion of formats ==="
rg -n "Type\|format\|binary\|signature" java/daikon/chicory/Instrument.java | grep -i "toString\|format" | head -10
echo ""
echo "=== Check how DaikonWriter.methodEntryName uses the type array ==="
sed -n '63,75p' java/daikon/chicory/DaikonWriter.java
</parameter>Repository: codespecs/daikon
Length of output: 3376
🏁 Script executed:
#!/bin/bash
# Check how methodPptName uses the type array
echo "=== methodPptName implementation ==="
sed -n '110,160p' java/daikon/chicory/DaikonWriter.java
echo ""
echo "=== Look at what happens with the type strings in methodPptName ==="
rg -A 20 "private static String methodPptName" java/daikon/chicory/DaikonWriter.java | head -40Repository: codespecs/daikon
Length of output: 3521
🏁 Script executed:
#!/bin/bash
# Let's verify what Type.toString actually produces by checking BCEL documentation or source
echo "=== Check if there's any BCEL Type class in the repo ==="
find . -name "Type.java" -type f | head -5
echo ""
echo "=== Check for any test cases that show Type.toString output ==="
rg -n "Type\\.toString\|Type::toString" . --type java -B 2 -A 5
echo ""
echo "=== Check for type format examples in tests or code ==="
rg -n "java\\.lang\\.String\|int\|boolean" java/daikon/chicory/Instrument.java | grep -i "type\|format" | head -10
echo ""
echo "=== Summary: Check the difference between Type.toString vs convertDescriptorToFqBinaryName ==="
echo "Creating comparison test..."
python3 << 'PYTHON_EOF'
# Let's think about what Type.toString produces vs what convertDescriptorToFqBinaryName produces
# Type.toString from BCEL for:
# - Primitive: int -> "int", boolean -> "boolean"
# - Object: "Ljava/lang/String;" in BCEL -> "java.lang.String"
# - Array: int[] -> "int[]"
# convertDescriptorToFqBinaryName:
# - Primitive: "I" -> "int", "Z" -> "boolean"
# - Object: "Ljava/lang/String;" -> "java.lang.String"
# - Array: "[I" -> "int[]"
print("=== Format Comparison ===")
print("Both methods should produce the same fully qualified binary name format:")
print("- Primitives: int, boolean, byte, char, double, float, long, short, void")
print("- Classes: java.lang.String (not java/lang/String with slashes)")
print("- Arrays: int[], java.lang.String[]")
print("")
print("Type::toString (BCEL Type) produces binary name format directly")
print("convertDescriptorToFqBinaryName converts descriptor format to binary name")
print("")
print("Result: Both should produce identical output for the same input types")
PYTHON_EOFRepository: codespecs/daikon
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Simpler check: Look for Type.toString usage examples in the codebase
echo "=== Search for how Type.toString output is used ==="
rg -n "Type\\.toString\|Type::toString" java/daikon/chicory/ -B 2 -A 2
echo ""
echo "=== Check if Instrument.java and Instrument24.java differ in other ways ==="
echo "File sizes:"
wc -l java/daikon/chicory/Instrument.java java/daikon/chicory/Instrument24.java
echo ""
echo "=== Check if the methods are truly meant to produce identical output ==="
echo "Looking at method signatures and return types:"
grep -A 3 "getFullyQualifiedParameterTypes\|getFqBinaryNameParameterTypes" java/daikon/chicory/Instrument*.java | head -20Repository: codespecs/daikon
Length of output: 1798
🏁 Script executed:
#!/bin/bash
# Final check: verify that these are truly parallel implementations for different versions
echo "=== Check class structure differences ==="
echo "Instrument.java uses MethodGen (BCEL old API)"
grep "class.*MethodGen\|import.*MethodGen" java/daikon/chicory/Instrument.java | head -3
echo ""
echo "Instrument24.java uses MethodGen24 (BCEL 6.0+ API)"
grep "class.*MethodGen24\|import.*MethodGen24" java/daikon/chicory/Instrument24.java | head -3
echo ""
echo "=== Verify if this is a version compatibility issue ==="
head -20 java/daikon/chicory/Instrument.java | grep -i "version\|bcel\|comment"
head -20 java/daikon/chicory/Instrument24.java | grep -i "version\|bcel\|comment"Repository: codespecs/daikon
Length of output: 734
Consider consolidating type name formatting to reduce parallel implementation paths.
getFullyQualifiedParameterTypes uses Type::toString while getFqBinaryNameParameterTypes uses convertDescriptorToFqBinaryName. Both methods are annotated with @BinaryName and produce the same output format, but through different conversion approaches. The different implementations exist because Instrument.java and Instrument24.java work with different BCEL API versions. To reduce maintenance burden and the risk of format divergence, consider extracting a shared utility method for binary name conversion that works with both BCEL APIs.
🤖 Prompt for AI Agents
In @java/daikon/chicory/Instrument.java around lines 1180 - 1183, The two
methods getFullyQualifiedParameterTypes and getFqBinaryNameParameterTypes
produce the same @BinaryName output via different code paths (Type::toString vs
convertDescriptorToFqBinaryName), so extract a shared utility that converts a
BCEL Type or a method/field descriptor into the canonical FQ binary-name string
and use that helper from both Instrument.java and Instrument24.java (replace
ArraysPlume.mapArray(Type::toString, ...) and convertDescriptorToFqBinaryName
usages to call the new utility); keep the @BinaryName contract on the
public-facing methods and ensure the helper accepts both a Type and a descriptor
string (or provide two thin overloads that delegate to a single implementation)
so both BCEL API variants reuse identical formatting logic.
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.
2 participants
No description provided.