Add additional version of DynComp for Java 24 #685
Conversation
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
java/daikon/chicory/Instrument.java (1)
621-647: Document iterator contract next to iterator creation for future maintainersThe pairing between
curMethodInfo.exit_location_is_includedandcurMethodInfo.exit_locationsis sound, and the comment ingenerate_return_instrumentation(lines 742-745) now explains the “one boolean per return; line only when boolean is true” contract. However, the iterators are created and drained here, and the only enforcement is via the finalassert !shouldIncludeIter.hasNext()/assert !exitLocationIter.hasNext().Consider adding a brief comment where the iterators are initialized (lines 621-622) that:
- States there is exactly one boolean per return instruction.
- States an integer exists only when the corresponding boolean is
true.- Mentions that
generate_return_instrumentationconsumes one boolean per return and conditionally one integer, and that the post-loop asserts ensure both iterators are exhausted.That keeps the producer/consumer invariant discoverable where the iterators are managed, not just down in the helper.
Also applies to: 742-745
java/daikon/chicory/Instrument24.java (3)
114-116: Publicdebug_ppt_omitstill creates tight coupling with DynComp
debug_ppt_omitremainspublic staticso thatdaikon.dcomp.Instrument24can toggle it. This shares mutable logging state across modules and couples the dcomp side tightly to Chicory’s internals.If feasible, consider one of the previously suggested decoupling options:
- Extract a shared “ppt filter”/logging utility that both Chicory and DynComp depend on, or
- Keep the log field package-visible and expose a small accessor or configuration hook instead of a public mutable field.
960-992: Add post-loop sanity checks for exit-location iterators
addInstrumentationAtExitswalks the instruction list and, for eachReturnInstruction, callsgenerate_return_instrumentation, which:
- Consumes exactly one element from
shouldIncludeIterper return, and- Consumes one element from
exitLocationIteronly when that boolean istrue.You already guard for too few entries via
!hasNext()checks that throw aRuntimeException. However, ifcreate_method_info_if_instrumentedever produces too many booleans or exit locations, the extra entries will silently remain unused after the loop.To mirror the stronger contract enforced in the BCEL implementation, consider adding a short post-loop check, e.g.:
if (shouldIncludeIter.hasNext() || exitLocationIter.hasNext()) { throw new IllegalStateException( "Mismatched exit_location_* for " + mgen.getName() + " (extra entries after return scan)"); }(or equivalent logging-based handling). This will catch producer/consumer mismatches early instead of letting stale iterator entries linger.
Also applies to: 1002-1051
1493-1500: Use!foundLineinstead offoundLine == falseThe condition:
if (line_number == prev_line_number && foundLine == false) {could be simplified to:
if (line_number == prev_line_number && !foundLine) {This is the idiomatic Java style and matches common usage elsewhere.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
java/daikon/chicory/Instrument.java(47 hunks)java/daikon/chicory/Instrument24.java(57 hunks)
⏰ 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). (8)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_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 (misc_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (2)
java/daikon/chicory/Instrument24.java (2)
1354-1376: Unable to verify review comment — file not found in repositoryThe referenced file
java/daikon/chicory/Instrument24.javaand the methodsconvertDescriptorToFqBinaryName,classDescToClassGetName, andcreate_method_info_if_instrumentedcould not be located in the codebase. Verification of the concern regarding type variable and wildcard handling in generic signatures cannot proceed without access to the actual implementation.To properly assess this review:
- Confirm the file path and repository structure
- Provide the implementation of
convertDescriptorToFqBinaryNameand related helpers- Clarify how type variables and wildcards in generic descriptors are currently handled
426-438: Align static constant-field collection and value representation with BCEL pathThe JDK 24 path collects all fields with a
ConstantValueAttribute(including non-primitives such asString) and converts each viaformatConstantDesc, which callsitem.resolveConstantDesc(MethodHandles.lookup()).toString(). In contrast, the BCEL-based implementation inInstrument.javaonly recordsfinal staticfields ofBasicType(primitives) and derivesvalueStringviaConstantValue.toString().This divergence means:
- The set of keys in
classInfo.staticMapcan differ between implementations (e.g.,static final Stringfields included here but not there)- The string representation of the same constant may differ (
resolveConstantDesc(...).toString()vs. BCEL'sConstantValue.toString())If downstream Chicory/DynComp logic expects identical
staticMapcontents regardless of which Instrument variant runs, align both implementations by either including non-primitives in both paths or excluding them from both, and centralize the constant-to-string conversion in a shared helper or documented format.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (7)
java/daikon/dcomp/StackMapUtils24.java (3)
51-52: Consider wiringdebugInstrumentto an existing DynComp debug flag.The
debugInstrumentis initialized as disabled and only enabled insideaddNewSpecialLocalvia direct assignment toDCInstrument24.bcelDebug. This couples the utility class tightly toDCInstrument24. Consider initializing it from the debug flag directly or making theSimpleLogaccept a supplier.
146-158: Consider abstracting offset calculation into a helper method.The logic to compute
newIndexandnewOffsetafter skippingthisand parameters could be extracted into a helper that returns both values (e.g., via a record). This would improve readability and testability.
175-177: Inconsistent format string style.The format string mixes
%splaceholders with string concatenation (+). Consider using one style consistently throughout.debugInstrument.log( - "Added a %s at slot %s.%n name: %s type: %s size: %s%n", - isParam ? "parameter" : "local", varNew.slot(), varNew.name(), varNew.type(), argSize); + "Added a %s at slot %d.%n name: %s type: %s size: %d%n", + isParam ? "parameter" : "local", varNew.slot(), varNew.name().stringValue(), varNew.type(), argSize);java/daikon/dcomp/OperandStack24.java (1)
78-139: Fixequals/hashCodecontract violation.The
equalsmethod uses coarse equivalence (e.g.,nullmatches any non-primitive, all non-primitives are treated as equivalent), buthashCodedelegates tostack.hashCode()which uses exactClassDescvalues. This violates the Java contract that equal objects must have equal hash codes.If
OperandStack24instances are used in hash-based collections (likely inCalcStack24), this will cause incorrect behavior.Apply this diff to align
hashCodewith the coarse equality semantics:@Override public int hashCode(@GuardSatisfied OperandStack24 this) { - return stack.hashCode(); + int result = 1; + for (ClassDesc item : stack) { + int elementHash; + if (item == null) { + elementHash = 0; // matches any non-primitive in equals + } else if (item.isPrimitive()) { + elementHash = 1; // all primitives are equivalent in equals + } else { + elementHash = 2; // all non-primitive, non-null items are equivalent in equals + } + result = 31 * result + elementHash; + } + return result; }java/daikon/dcomp/CalcStack24.java (2)
142-941: ThesimulateInstructionmethod is comprehensive but very long.At ~800 lines, this method is difficult to maintain and test. Previous reviews suggested decomposing it into helper methods (e.g.,
handleArithmeticOp,handleFieldAccess,handleInvoke). While the current implementation is functionally correct, consider this refactoring for improved maintainability.
992-1022: Consider using enhanced switch expression for conciseness.The
npaiToElementTypeDescriptormethod can be simplified using Java's enhanced switch expression syntax.static String npaiToElementTypeDescriptor(NewPrimitiveArrayInstruction npai) { - switch (npai.typeKind()) { - case BOOLEAN -> { - return "Z"; - } - case BYTE -> { - return "B"; - } - case CHAR -> { - return "C"; - } - case DOUBLE -> { - return "D"; - } - case FLOAT -> { - return "F"; - } - case INT -> { - return "I"; - } - case LONG -> { - return "J"; - } - case SHORT -> { - return "S"; - } - default -> { - throw new DynCompError("unknown primitive type " + npai.typeKind() + " in: " + npai); - } - } + return switch (npai.typeKind()) { + case BOOLEAN -> "Z"; + case BYTE -> "B"; + case CHAR -> "C"; + case DOUBLE -> "D"; + case FLOAT -> "F"; + case INT -> "I"; + case LONG -> "J"; + case SHORT -> "S"; + default -> throw new DynCompError("unknown primitive type " + npai.typeKind() + " in: " + npai); + }; }java/daikon/dcomp/ClassGen24.java (1)
108-113: Fix Javadoc typo: “indicatated” → “indicated”.The Javadoc for
containsMethodstill contains the typo previously reported:- * Determine if this class contains the indicatated method. + * Determine if this class contains the indicated method.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
java/daikon/dcomp/CalcStack24.java(1 hunks)java/daikon/dcomp/ClassGen24.java(1 hunks)java/daikon/dcomp/OperandStack24.java(1 hunks)java/daikon/dcomp/StackMapUtils24.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java/daikon/dcomp/StackMapUtils24.java (2)
java/daikon/chicory/MethodGen24.java (1)
MethodGen24(51-832)java/daikon/dcomp/DCInstrument24.java (1)
DCInstrument24(136-4952)
⏰ 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 (7)
java/daikon/dcomp/StackMapUtils24.java (2)
63-104: LGTM! The instruction slot adjustment logic is correct.The method properly handles all local-variable-referencing instructions (
RetInstruction,IncrementInstruction,LoadInstruction,StoreInstruction) and correctly adjusts slots at or above the insertion point. The error handling wraps exceptions inDynCompErrorwith context.
128-199: LGTM! TheaddNewSpecialLocalimplementation is well-structured.The method correctly:
- Handles both static and instance methods
- Computes the proper insertion point after parameters
- Updates locals table, parameter types/names when
isParamis true- Adjusts subsequent local slots and instruction references when code exists
The integration with
MethodGen24.MInfo24for scope labels andnextLocalIndextracking is appropriate.java/daikon/dcomp/OperandStack24.java (3)
45-48: LGTM! Constructor properly initializes maxStack.The constructor correctly stores the maximum stack size bound.
59-69: LGTM! Clone implementation is correct.The clone creates a new
ArrayListto ensure copy-on-write semantics while sharingClassDescobjects (which are immutable). UsingDynCompErrorfor the impossibleCloneNotSupportedExceptionis appropriate.
235-241: LGTM!slotSizecorrectly handles null and delegates toTypeKind.Returning 1 for
nullis appropriate sincenullrepresents a reference type which always occupies 1 slot.java/daikon/dcomp/CalcStack24.java (2)
50-56: LGTM! Class documentation properly notes side effects.The Javadoc correctly indicates that methods may modify
DCInstrument24.localsandDCInstrument24.stacks, addressing previous review feedback.
943-983: LGTM!lceToCDhelper properly handles allLoadableConstantEntrytypes.The method correctly maps:
ClassEntry→CD_Class(not the described class - fixes previous issue)ConstantDynamicEntry→ its declared typeMethodHandleEntry/MethodTypeEntry→ appropriate runtime typesThis addresses the critical issues flagged in previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
java/daikon/dcomp/OperandStack24.java (2)
71-98: AlignhashCodewith the coarse equality defined inequals/compareOperandStackElements.
equalsdeliberately uses a very coarse notion of equality (e.g.,nullmatches any non‑primitive; all non‑primitive arrays/classes are equivalent; all primitives must match exactly, otherwise aDynCompErroris thrown), buthashCodecurrently delegates tostack.hashCode(), which depends on the exactClassDescvalues and onnullvs non‑null. Two stacks thatequalsconsiders equal can therefore have different hash codes, which violates theequals/hashCodecontract and can break behavior ifOperandStack24is ever used as a key in hash‑based collections.I recommend making
hashCodemirror the same coarse partitioning ascompareOperandStackElements:/** * @return a hash code value for the object */ @Override public int hashCode(@GuardSatisfied OperandStack24 this) { - return stack.hashCode(); + int result = 1; + for (ClassDesc item : stack) { + final int elementHash; + if (item == null) { + // Matches any non-primitive in equals() + elementHash = 0; + } else if (item.isPrimitive()) { + // All primitives form a single equivalence class in equals() + elementHash = 1; + } else { + // All non-primitive, non-null types (arrays/classes) form a single class in equals() + elementHash = 2; + } + result = 31 * result + elementHash; + } + return result; }If there’s a reason
OperandStack24is guaranteed never to be used in a hash‑based collection, it’d be good to document that explicitly; otherwise this change is important for correctness.Also applies to: 100-153, 164-170
42-48: EnforcemaxStackin terms of slots, not element count, to handlelong/doublecorrectly.Javadoc and naming indicate
maxStackis a bound on stack slots (matching the JVM), andslotsUsed()carefully accounts for wide values viaslotSize(...). However,pushcurrently checksstack.size() >= maxStack, which ignores wide types and can let the simulated stack exceed its slot capacity wheneverlong/doublevalues are present.For example, with
maxStack == 2, pushing twolongvalues would use 4 slots but still pass thestack.size()check. This defeats the purpose ofmaxStackas a validation guard and can mask bugs in the symbolic simulation that mishandle wide values.Consider enforcing capacity using slots instead:
/** Pushes a ClassDesc object onto the stack. */ public void push(final ClassDesc type) { - if (stack.size() >= maxStack) { - throw new DynCompError("Operand stack size exceeded: " + stack); - } - stack.add(type); + // Enforce the JVM-style maxStack in terms of *slots*, not element count. + if (slotsUsed() + slotSize(type) > maxStack) { + throw new DynCompError("Operand stack size (in slots) exceeded: " + stack); + } + stack.add(type); }If you prefer not to pay the
slotsUsed()cost on everypush, a cheaper alternative is to track a runningslotsUsedcounter updated inpush/popinstead of recomputing each time.Also applies to: 177-181, 215-221, 232-244
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/daikon/dcomp/OperandStack24.java(1 hunks)
⏰ 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). (8)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_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 (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
java/daikon/dcomp/CalcStack24.java (3)
68-72: Clarify null sentinel ClassDesc naming and intent
nullCDis a sentinel value, but both the name and"fake.ClassDecs.for.null"are opaque. Consider renaming to something likeNULL_SENTINEL_CD(or similar) and/or updating the Javadoc to explicitly state this is a sentinel, not a real class, to make its purpose clearer.
162-162: Fix typo in commentThe inline comment has a typo; please update it for clarity:
- // caculate stack changes + // calculate stack changes
717-731: Resolve JSR/RET handling or explicitly scope to legacy classfilesJSR/RET opcodes have been illegal in class file version 51.0+ (Java 7+), but this code still contains TODOs and a partial simulation using a fake return-address value. For a Java 24-focused tool, this is brittle.
Consider either:
- Failing fast with an
UnsupportedOperationExceptionwhen encounteringJSR/JSR_W/RET/RET_W, or- Clearly documenting that you intend to support legacy classfile versions (< 51.0) and explaining the limitations of the current approximation.
For a fail-fast approach, a minimal change would be:
- // TODO: JSR and RET have been illegal since JDK 7 (class file version 51.0). - // Consider throwing an UnsupportedOperationException for these opcodes - // if we're only supporting modern class files (version 51.0+). - // Currently maintaining support for completeness. - - // operand stack before: ... - // operand stack after: ..., [return address] case Opcode.JSR: case Opcode.JSR_W: - // Perhaps we should add label of next instruction to work list but we have no idea - // what stack will be after return from the JSR. - stack.push(CD_Object); // there is no way to represent a return address, fake it - DiscontinuedInstruction.JsrInstruction ji = (DiscontinuedInstruction.JsrInstruction) inst; - addLabelsToWorklist(ji.target(), null, stack); - return false; + throw new UnsupportedOperationException( + "JSR/JSR_W opcodes are illegal in class files version 51.0+ and not supported: " + + inst);- // TODO: JSR and RET have been illegal since JDK 7 (class file version 51.0). - // Consider throwing an UnsupportedOperationException for these opcodes - // if we're only supporting modern class files (version 51.0+). - // Currently maintaining support for completeness. - // operand stack: no change case Opcode.RET: case Opcode.RET_W: - // the variable referenced must contain a return address - // that we would treat as target of jump; no way to do that - return false; + throw new UnsupportedOperationException( + "RET/RET_W opcodes are illegal in class files version 51.0+ and not supported: " + + inst);Also applies to: 865-874
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/daikon/dcomp/CalcStack24.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java/daikon/dcomp/CalcStack24.java (1)
java/daikon/dcomp/DCInstrument24.java (1)
DCInstrument24(136-4952)
⏰ 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). (8)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
java/daikon/dcomp/OperandStack24.java (4)
71-98: AlignhashCodewith coarse equality semantics inequals.
equalstreats all non‑primitive (includingnull) stack entries as equivalent and only distinguishes primitives, buthashCodedelegates tostack.hashCode(), which depends on the exactClassDesc(and onnullvs non‑null). Two stacks thatequalsconsiders equal can currently have different hash codes, violating theequals/hashCodecontract.You should compute the hash code in terms of the same coarse partition used by
equals(primitive vs non‑primitive/null), ignoring the specific reference types. For example:/** * @return a hash code value for the object */ @Override public int hashCode(@GuardSatisfied OperandStack24 this) { - return stack.hashCode(); + int result = 1; + for (ClassDesc item : stack) { + // In equals(), any non-primitive or null value is treated as equivalent, + // and primitives are treated as equivalent among themselves when they match. + int elementHash = (item == null || !item.isPrimitive()) ? 0 : 1; + result = 31 * result + elementHash; + } + return result; }This preserves the intended approximate equality while restoring the required contract.
Also applies to: 165-171
204-214: Clarifypop(int count)’s Javadoc and BCEL-compatibility behavior.The Javadoc still refers to “i elements” while the parameter is named
count, and the method always returnsnullwithout explaining why. Tighten the docs to reduce confusion:- /** - * Pops i elements off the stack. Always returns {@code null}. - * - * @return {@code null} - */ + /** + * Pops {@code count} elements off the stack. Always returns {@code null}. + * + * <p>The return value is retained for BCEL API compatibility and is always {@code null}. + * + * @return {@code null} + */If you no longer need BCEL signature compatibility at all call sites, consider changing the return type to
voidin a follow‑up.
216-222: EnforcemaxStackin terms of slots, not element count, inpush.
maxStackis documented as “maximum number of stack slots,” butpushenforces onlystack.size() >= maxStack. A singlelong/doubleconsumes 2 slots, so you can overflowmaxStackin terms of slots whilestack.size()is still below the bound (e.g.,maxStack == 1, pushing along).Use
slotsUsed()plus the incoming element’sslotSizeto enforce the true slot capacity:/** Pushes a ClassDesc object onto the stack. */ public void push(final ClassDesc type) { - if (stack.size() >= maxStack) { + if (slotsUsed() + slotSize(type) > maxStack) { throw new DynCompError("Operand stack size exceeded: " + stack); } stack.add(type); }This aligns the runtime check with both the Javadoc and JVM stack semantics.
233-245: Iterate directly overstackinslotsUsed()instead of viapeek(i).
slotsUsed()currently callspeek(i)inside the loop, which in turn callssize()and reverse‑indexes the list. Functionally it’s fine, but it adds indirection and unnecessary work.A direct iteration over
stackis simpler and avoids depending onpeek’s indexing convention:public @NonNegative int slotsUsed(@GuardSatisfied OperandStack24 this) { - int slots = 0; - for (int i = 0; i < stack.size(); i++) { - slots += slotSize(peek(i)); - } - return slots; + int slots = 0; + for (ClassDesc item : stack) { + slots += slotSize(item); + } + return slots; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/daikon/dcomp/OperandStack24.java(1 hunks)
⏰ 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). (8)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (1)
java/daikon/dcomp/OperandStack24.java (1)
262-275: ConfirmslotSize’s range and reliance onTypeKind.from(ClassDesc).Given the Javadoc (“2 for long/double, 1 for everything else”) and the use of
TypeKind.from(item).slotSize(), this method should always return 1 or 2 (withnullexplicitly mapped to 1). That matches JVM slot semantics and answers the earlier question about its range.Please double‑check against the Java 24
TypeKind/ClassDescAPIs to ensure no new kinds can appear here with a differentslotSize.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
java/daikon/dcomp/ClassGen24.java (3)
111-113: Typo: "indicatated" → "indicated"This typo was flagged in a previous review but appears to still be present.
- * Determine if this class contains the indicatated method. + * Determine if this class contains the indicated method.
202-209: Consider returning an unmodifiable view fromgetInterfaceList().As noted in a previous review, callers (e.g.,
classBuilder.withInterfaces(classGen.getInterfaceList())) only read this list. ReturningCollections.unmodifiableList(interfaceList)would prevent accidental modifications.public List<ClassEntry> getInterfaceList() { - return interfaceList; + return Collections.unmodifiableList(interfaceList); }
37-37: Consider makingclassModelfinal for consistency.All other cached fields (
accessFlags,interfaceList,superclassName,className,classBuilder,isInterface,isStatic) are declaredfinal, butclassModelis not. Since it's also assigned only in the constructor and never reassigned, making itfinalwould be consistent with the other fields.- private ClassModel classModel; + private final ClassModel classModel;java/daikon/chicory/MethodGen24.java (2)
614-622: Defensive validation still missing forsetParameterNames.As flagged in a previous review, this setter stores the caller's array directly without validating that
paramNames.length == paramTypes.length. The Javadoc warns users, but a mismatch will causeArrayIndexOutOfBoundsExceptioninfixLocals,toString, and other methods that iterate both arrays in lockstep.Consider adding defensive validation:
public void setParameterNames(final @Identifier String[] paramNames) { + if (paramNames.length != this.paramTypes.length) { + throw new IllegalArgumentException( + "paramNames.length (" + paramNames.length + ") != paramTypes.length (" + this.paramTypes.length + ")"); + } this.paramNames = paramNames; }
643-651: Defensive validation still missing forsetParameterTypes.Same issue as
setParameterNames- the setter doesn't validate the length invariant.public void setParameterTypes(final ClassDesc[] paramTypes) { + if (paramTypes.length != this.paramNames.length) { + throw new IllegalArgumentException( + "paramTypes.length (" + paramTypes.length + ") != paramNames.length (" + this.paramNames.length + ")"); + } this.paramTypes = paramTypes; }java/daikon/chicory/Instrument24.java (1)
1254-1254: Use diamond operator for consistency.This was flagged in a previous review. The explicit type argument should be removed to match the style used elsewhere in the file.
- List<SwitchCase> newCaseList = new ArrayList<SwitchCase>(); + List<SwitchCase> newCaseList = new ArrayList<>();java/daikon/dcomp/BuildJDK24.java (4)
194-198: Filter only .class entries when writing jdk_classes.txtThe loop writes all entries from
class_stream_mapincluding non-.class resources. This should be restricted to only.classfiles to avoid polluting the class list.try (PrintWriter pw = new PrintWriter(jdk_classes_file, UTF_8.name())) { for (String classFileName : class_stream_map.keySet()) { - pw.println(classFileName.replace(".class", "")); + if (classFileName.endsWith(".class")) { + pw.println(classFileName.substring(0, classFileName.length() - ".class".length())); + } } }
364-390: Close InputStreams for skipped entries to prevent resource leaksWhen skipping
module-info.class, non-.class files, orObject.class, the associatedInputStreamfromclass_stream_mapis never closed. For large JDK instrumentation runs, this can exhaust file descriptors.if (classFileName.equals("module-info.class")) { System.out.printf("Skipping file %s%n", classFileName); + try (InputStream ignored = class_stream_map.get(classFileName)) { /* close */ } continue; } // Handle non-.class files and Object.class. In JDK 8, copy them unchanged. // For JDK 9+ we do not copy as these items will be loaded from the original module file. if (!classFileName.endsWith(".class") || classFileName.equals("java/lang/Object.class")) { if (BcelUtil.javaVersion > 8) { if (verbose) { System.out.printf("Skipping file %s%n", classFileName); } + try (InputStream ignored = class_stream_map.get(classFileName)) { /* close */ } continue; }
451-467: Ensure parent directories exist before writing interface classes
createDCompClasswrites todestDir/java/lang/but does not ensure the directory exists. While it may exist from prior instrumentation, this method could fail if called independently.classBytes = ClassFile.of() .build( ClassDesc.of(Signatures.addPackage("java.lang", className)), 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"); + File parent = outputFile.getParentFile(); + if (parent != null) { + parent.mkdirs(); + } Files.write(outputFile.toPath(), classBytes);
491-492: Consider makingtimeFormattera constant
DateTimeFormatteris immutable and thread-safe. Declaring it asprivate static finalwith an uppercase name follows Java conventions.- private static final DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss"); + private static final DateTimeFormatter TIME_FORMATTER = DateTimeFormatter.ofPattern("HH:mm:ss");And update the usage at line 551:
- LocalDateTime.now(ZoneId.systemDefault()).format(timeFormatter)); + LocalDateTime.now(ZoneId.systemDefault()).format(TIME_FORMATTER));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
java/daikon/chicory/Instrument.java(47 hunks)java/daikon/chicory/Instrument24.java(57 hunks)java/daikon/chicory/MethodGen24.java(16 hunks)java/daikon/chicory/MethodInfo.java(2 hunks)java/daikon/dcomp/BuildJDK.java(6 hunks)java/daikon/dcomp/BuildJDK24.java(1 hunks)java/daikon/dcomp/ClassGen24.java(1 hunks)java/daikon/dcomp/Instrument.java(10 hunks)java/daikon/dcomp/Instrument24.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
java/daikon/dcomp/Instrument24.java (2)
java/daikon/chicory/ClassInfo.java (1)
ClassInfo(20-154)java/daikon/dcomp/Premain.java (1)
Premain(32-417)
java/daikon/dcomp/BuildJDK24.java (3)
java/daikon/DynComp.java (1)
DynComp(25-541)java/daikon/chicory/ClassInfo.java (1)
ClassInfo(20-154)java/daikon/dcomp/DCInstrument24.java (1)
DCInstrument24(136-4952)
⏰ 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). (8)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_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 (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (30)
java/daikon/chicory/MethodInfo.java (1)
104-104: LGTM: Consistent field renaming.The constructor parameter and field assignment have been updated consistently with the field rename. The new name
exit_location_is_includedis more descriptive and improves code clarity.Also applies to: 111-111
java/daikon/dcomp/ClassGen24.java (1)
94-109: LGTM: Cross-poolClassEntrycomparison fixed.The
addInterfacemethod now correctly compares by internal name rather than relying onClassEntry.equals(), which avoids the cross-constant-pool equality issue flagged in a previous review.java/daikon/chicory/Instrument.java (4)
8-8: LGTM: ArraysPlume import added for cleaner parameter type mapping.The new import enables the simplified
getFullyQualifiedParameterTypesimplementation usingArraysPlume.mapArray.
1142-1145: LGTM: Cleaner implementation using ArraysPlume.Replacing the manual loop with
ArraysPlume.mapArrayis more concise and idiomatic.
621-624: LGTM: Iterator contract documented as requested.The comment at lines 621-622 now documents the invariant that
exit_location_is_includedcontains exactly one boolean per return instruction andexit_locationscontains an integer only when that boolean is true. This addresses previous review feedback.
1265-1267: LGTM: Boolean check simplified.The condition now uses the idiomatic
!foundLineinstead offoundLine == false.java/daikon/chicory/MethodGen24.java (3)
315-348: LGTM: New constructor for DCInstrument24-created methods.The constructor properly initializes all required fields and follows the same pattern as the main constructor. The empty
localsTablewill be populated whenfixLocalsis called.
447-525: LGTM:fixLocalsimplementation with clear postconditions.The method handles the tricky cases well: missing locals for parameters, alignment gaps for category-2 parameters, and absent
thisfor non-static native methods. The Javadoc clearly documents the postconditions. The re-sorting by slot after modifications is correct.
554-566: LGTM:isMain()implementation is correct.The method correctly checks all criteria for a standard main method: static, void return type, name "main", single parameter of type
String[].java/daikon/chicory/Instrument24.java (4)
995-998: LGTM: Iterator exhaustion checks added.The assertions at lines 996-997 verify that both
shouldIncludeIterandexitLocationIterare fully consumed after instrumenting all returns, catching any mismatch betweencreate_method_info_if_instrumentedand the code scan.
1500-1502: LGTM: Boolean check uses idiomatic form.The condition now uses
!foundLineinstead offoundLine == false.
838-856: LGTM: Clean decomposition of instrumentation pipeline.The
instrumentInstructionListmethod cleanly decomposes intoaddInstrumentationAtEntryandaddInstrumentationAtExits, making the instrumentation flow easy to follow.
1665-1669: LGTM: Wildcard handling added for type arguments.The
convertTypeArgumentsToBinaryNamesmethod now handles the unbounded wildcard*by returning?, which is needed for signatures containing wildcards.java/daikon/dcomp/BuildJDK.java (4)
249-267: LGTM: JarFile now uses try-with-resources.The
JarFileis now properly closed via try-with-resources, ensuring resources are released even if an exception occurs during processing.
464-464: LGTM:timeFormattermadestatic final.
DateTimeFormatteris thread-safe and immutable, so making this a static final field is appropriate and avoids recreating the formatter on each use.
55-55: LGTM: Class madefinal.Making
BuildJDKfinal prevents unintended subclassing, which is appropriate for a utility class with a private constructor.
485-485: LGTM: Method renamed toinstrument_jdk_class().The rename aligns with the broader refactoring in
DCInstrumentto clarify that this instruments a single JDK class.java/daikon/dcomp/Instrument.java (5)
27-30: LGTM: Section markers improve code organization.The "Start/End of diagnostics" comments help readers navigate the file structure.
81-96: LGTM: Renamed towriteDebugClassFileswith improved error handling.The method is renamed consistently with
Instrument24.java, and the error handling now conditionally prints stack traces only when debug is enabled.
273-292: LGTM:is_dcompnow uses@InternalFormannotation.The parameter type annotation correctly reflects that the method receives class names in internal form (with
/separators) directly from thetransformmethod. The string comparisons are updated to use/instead of..
302-319: LGTM:is_transformerupdated consistently.The
@InternalFormannotation and/-based comparisons are consistent with theis_dcompchanges.
175-178: LGTM: Added check forjava/lang/instrument/classes.This new guard prevents instrumenting Java's own instrumentation classes, which could cause circular dependencies or failures.
java/daikon/dcomp/BuildJDK24.java (3)
113-131: LGTM!The main method structure is well-organized with clear branching for the two usage modes (JDK instrumentation vs. specific class files). The progress reporting and error handling are appropriate.
207-241: LGTM!The
check_java_homemethod properly validates the Java environment with clear error messages and appropriate exit conditions.
506-553: LGTM!The
instrumentClassFilemethod is well-structured with proper directory creation, error handling, and progress reporting. The console check before printing progress is a nice touch.java/daikon/dcomp/Instrument24.java (5)
156-161: LGTM!The null check for
classNameis now correctly placed before the call toSignatures.internalFormToBinaryName(), preventing the NPE that was flagged in the previous review.
94-126: LGTM!The
writeDebugClassFilesmethod has robust error handling that prevents instrumentation failures due to debug output issues. The design to catch and log errors without rethrowing is appropriate for diagnostic code.
142-301: LGTM!The
transformmethod is well-structured with:
- Proper null handling for className and loader
- Appropriate checks for shutdown, pre-instrumented classes, JDK classes, and known transformers
- Graceful error handling that returns null (no transformation) rather than crashing
- Optional debug output when
DynComp.dumpis enabledThe one-time warning for detected transformers (lines 229-236) is a good UX pattern.
310-330: LGTM!The
is_dcomphelper correctly identifies internal DynComp classes while allowing test classes (DcompTest,ChicoryTest) to be instrumented. The explicit exclusions for plumelib and specific Daikon classes are appropriate.
339-358: LGTM!The
is_transformerheuristic covers common bytecode transformation libraries (Groovy, Mockito, Objenesis). The list can be extended if additional conflicts are discovered.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
java/daikon/dcomp/Instrument.java (1)
298-320: Add @InternalForm annotation to is_transformer parameter.The
is_transformermethod checks slash-separated class name patterns (lines 304-318), which is InternalForm notation, and is called with an@InternalForm Stringat line 197. However, its signature (line 302) lacks the@InternalFormannotation, unlike the consistentis_dcompmethod at line 273. This type annotation inconsistency should be corrected.Apply this diff:
- protected static boolean is_transformer(String className) { + protected static boolean is_transformer(@InternalForm String className) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/daikon/dcomp/Instrument.java(10 hunks)
⏰ 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). (8)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_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: build
🔇 Additional comments (9)
java/daikon/dcomp/Instrument.java (9)
20-24: LGTM: Improved documentation clarity.The javadoc updates better explain the class's role as a ClassFileTransformer and clarify the transform method's decision-making behavior.
27-30: LGTM: Clear section boundaries.The diagnostic block markers improve code organization and readability.
81-96: LGTM: Method rename and improved error handling.The rename to
writeDebugClassFilesis more descriptive, and the conditional stack trace printing (lines 91-93) reduces noise when debug mode is disabled while preserving diagnostic information when needed.
121-132: LGTM: Early binaryClassName introduction improves consistency.Computing
binaryClassNameearly (line 131) and using it uniformly throughout the method improves logging consistency and readability.
139-178: LGTM: Improved pre-check guards and comments.The consolidated problem class check (lines 165-168) and the explicit skip for
java/lang/instrument/classes (lines 175-178) improve code clarity and robustness. The comment updates enhance readability.
214-219: LGTM: Accurate loader terminology.Using "loaders" (plural) correctly reflects that multiple loader instances are being logged.
227-251: LGTM: Consistent and improved error handling.The standardized error handling for both parsing (lines 227-233) and transformation (lines 239-251) errors with conditional stack traces reduces noise while preserving diagnostic information when needed. Returning
nullcorrectly signals no bytecode changes.
253-262: LGTM: Clear return flow.The refactored return logic is clear and consistent with the use of
binaryClassNamethroughout.
269-292: LGTM: Proper type annotations for is_dcomp.The signature change to
private static boolean is_dcomp(@InternalForm String className)correctly documents that the method expects slash-separated class names, matching its implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
java/daikon/dcomp/BuildJDK24.java (4)
194-198: Filter only.classentries when writing tojdk_classes.txt.The loop writes all keys from
class_stream_mapincluding potential non-.classresources. This should filter for.classentries only.try (PrintWriter pw = new PrintWriter(jdk_classes_file, UTF_8.name())) { for (String classFileName : class_stream_map.keySet()) { - pw.println(classFileName.replace(".class", "")); + if (classFileName.endsWith(".class")) { + pw.println(classFileName.substring(0, classFileName.length() - ".class".length())); + } } }
343-344: Typo: "indentified" → "identified".- * Instrument each of the classes indentified by the class_stream_map argument. + * Instrument each of the classes identified by the class_stream_map argument.
367-394: CloseInputStreams for skipped entries to avoid resource leaks.For entries skipped (e.g.,
module-info.class, non-.classfiles, orObject.classon JDK 9+), the correspondingInputStreamfromclass_stream_mapis never closed, leading to resource exhaustion when processing many files.if (classFileName.equals("module-info.class")) { System.out.printf("Skipping file %s%n", classFileName); + try (InputStream ignored = class_stream_map.get(classFileName)) { /* close */ } continue; } // Handle non-.class files and Object.class. In JDK 8, copy them unchanged. // For JDK 9+ we do not copy as these items will be loaded from the original module file. if (!classFileName.endsWith(".class") || classFileName.equals("java/lang/Object.class")) { if (BcelUtil.javaVersion > 8) { if (verbose) { System.out.printf("Skipping file %s%n", classFileName); } + try (InputStream ignored = class_stream_map.get(classFileName)) { /* close */ } continue; }
464-466: Ensure parent directories exist before writing.
Files.write()will throwNoSuchFileExceptionif the parent directory doesn't exist. Whilejava/langis likely created duringinstrument_classes, it's safer to explicitly ensure it exists.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);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
java/daikon/dcomp/BuildJDK.java(6 hunks)java/daikon/dcomp/BuildJDK24.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java/daikon/dcomp/BuildJDK24.java (3)
java/daikon/DynComp.java (1)
DynComp(25-541)java/daikon/chicory/ClassInfo.java (1)
ClassInfo(20-154)java/daikon/dcomp/DCInstrument24.java (1)
DCInstrument24(136-4952)
⏰ 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). (8)
- GitHub Check: codespecs.daikon (typecheck_latest_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: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (7)
java/daikon/dcomp/BuildJDK.java (3)
55-58: LGTM! Class made final with appropriate constructor.Making
BuildJDKafinalclass with a private constructor is appropriate since this is a utility class that should not be subclassed and is only instantiated withinmain.
467-467: LGTM! DateTimeFormatter is now static final.
DateTimeFormatteris immutable and thread-safe, so makingtimeFormatteraprivate static finalfield is the correct approach.
486-489: LGTM! API call updated to align with refactored instrumentation.The change from
instrument_jdk()toinstrument_jdk_class()properly aligns with the DCInstrument API refactoring introduced in this PR.java/daikon/dcomp/BuildJDK24.java (4)
59-62: LGTM! Proper utility class structure.The class is correctly declared as
public finalwith a private constructor, consistent withBuildJDK.java.
253-280: LGTM! JarFile handling is consistent with BuildJDK.java.The intentional non-closure of the
JarFileis documented and necessary to keepInputStreamobjects valid until they are consumed byinstrument_classes. For a batch tool that exits after completion, this is acceptable.
478-492: LGTM! Interface class generation using ClassFile API.The implementation correctly uses the Java ClassFile API to build interface classes with appropriate flags, version, and optional method declaration.
510-556: LGTM! Instrumentation flow is well-structured.The method properly creates
ClassInfo, delegates toDCInstrument24, handles instrumentation failure, writes debug files when enabled, and provides progress feedback.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
java/daikon/dcomp/OperandStack24.java (3)
75-102: Fix equals/hashCode contract violation.The
equalsmethod uses coarse equality (e.g.,nullmatches any non-primitive; all non-primitive arrays/classes are treated as equivalent), buthashCodedelegates tostack.hashCode(), which depends on exactClassDescvalues. This violates theequals/hashCodecontract: two stacks thatequalsdeems equal can have different hash codes.Apply this diff to fix the contract:
@Override public int hashCode(@GuardSatisfied OperandStack24 this) { - return stack.hashCode(); + int result = 1; + for (ClassDesc item : stack) { + int elementHash; + if (item == null) { + elementHash = 0; // matches any non-primitive in equals + } else if (item.isPrimitive()) { + elementHash = item.hashCode(); // distinguish different primitive types + } else { + elementHash = 2; // all non-primitive, non-null items are equivalent in equals + } + result = 31 * result + elementHash; + } + return result; }Also applies to: 171-177
119-122: Annotate parameters as@Nullablefor accuracy.The method explicitly handles
thisItem == nullandotherItem == null, but the parameters are declared without nullness annotations. Marking them@Nullabledocuments the intended contract and aligns with the implementation.protected boolean compareOperandStackElements( @GuardSatisfied OperandStack24 this, - @Nullable ClassDesc thisItem, - @Nullable ClassDesc otherItem) { + @Nullable ClassDesc thisItem, + @Nullable ClassDesc otherItem) {Note: The code already has
@Nullableannotations (lines 121-122), so this appears to be addressed.
210-220: Clarifypop(int count)contract.The method always returns
nullregardless of what was popped. If this is for BCEL API compatibility, the current implementation is fine. Consider changing the return type tovoidif all call sites ignore the return value.java/daikon/dcomp/BuildJDK24.java (5)
253-280: Critical: InputStreams become invalid after JarFile is closed.The comment at lines 258-259 claims the JarFile is intentionally not closed, but the code structure still risks issues. More importantly, for JDK 8 paths, if the JarFile is closed (or the code is refactored to use try-with-resources), the stored InputStreams become invalid.
Consider reading entry contents into memory:
+import java.io.ByteArrayInputStream; ... - // Get the InputStream for this file - InputStream is = jfile.getInputStream(entry); - class_stream_map.put(entryName, is); + // Read the bytes into memory so streams remain valid + try (InputStream is = jfile.getInputStream(entry)) { + byte[] bytes = is.readAllBytes(); + class_stream_map.put(entryName, new ByteArrayInputStream(bytes)); + }
194-198: Filter only .class entries when writing jdk_classes.txt.The loop writes all keys from
class_stream_map, including non-.class resources. This could cause issues when the list is consumed later.try (PrintWriter pw = new PrintWriter(jdk_classes_file, UTF_8.name())) { for (String classFileName : class_stream_map.keySet()) { - pw.println(classFileName.replace(".class", "")); + if (classFileName.endsWith(".class")) { + pw.println(classFileName.replace(".class", "")); + } } }
343-344: Typo: "indentified" → "identified".- * Instrument each of the classes indentified by the class_stream_map argument. + * Instrument each of the classes identified by the class_stream_map argument.
367-370: Close InputStreams for skipped entries to avoid resource leaks.When entries are skipped (module-info.class, non-.class files, Object.class), the InputStreams from
class_stream_mapare never closed, potentially exhausting file descriptors when processing modules.if (classFileName.equals("module-info.class")) { System.out.printf("Skipping file %s%n", classFileName); + try (InputStream ignored = class_stream_map.get(classFileName)) { /* close */ } continue; } ... if (!classFileName.endsWith(".class") || classFileName.equals("java/lang/Object.class")) { if (BcelUtil.javaVersion > 8) { if (verbose) { System.out.printf("Skipping file %s%n", classFileName); } + try (InputStream ignored = class_stream_map.get(classFileName)) { /* close */ } continue; }Also applies to: 374-380
464-466: Ensure parent directories exist before writing interface classes.The code creates
outputFilebut doesn't ensure the parent directory (java/lang) exists underdestDir.File outputFile = new File(new File(new File(destDir, "java"), "lang"), className + ".class"); + File parent = outputFile.getParentFile(); + if (parent != null && !parent.exists()) { + parent.mkdirs(); + } Files.write(outputFile.toPath(), classBytes);java/daikon/dcomp/CalcStack24.java (1)
962-992: Consider using enhanced switch expression for conciseness.The switch statement is verbose. Java's enhanced switch expression would be more concise:
static String npaiToElementTypeDescriptor(NewPrimitiveArrayInstruction npai) { - switch (npai.typeKind()) { - case BOOLEAN -> { - return "Z"; - } - case BYTE -> { - return "B"; - } - // ... more cases - } + return switch (npai.typeKind()) { + case BOOLEAN -> "Z"; + case BYTE -> "B"; + case CHAR -> "C"; + case DOUBLE -> "D"; + case FLOAT -> "F"; + case INT -> "I"; + case LONG -> "J"; + case SHORT -> "S"; + default -> throw new DynCompError( + "unknown primitive type " + npai.typeKind() + " in: " + npai); + }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
java/daikon/chicory/MethodInfo.java(2 hunks)java/daikon/dcomp/BuildJDK24.java(1 hunks)java/daikon/dcomp/CalcStack24.java(1 hunks)java/daikon/dcomp/Instrument.java(10 hunks)java/daikon/dcomp/Instrument24.java(1 hunks)java/daikon/dcomp/OperandStack24.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java/daikon/dcomp/CalcStack24.java (2)
java/daikon/chicory/MethodGen24.java (1)
MethodGen24(51-840)java/daikon/dcomp/DCInstrument24.java (1)
DCInstrument24(136-4956)
⏰ 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). (8)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (23)
java/daikon/chicory/MethodInfo.java (3)
52-64: LGTM! Excellent documentation.The expanded Javadoc clearly explains the different treatment of exit locations by Chicory vs. DynComp, and properly documents the relationship with
exit_location_is_included. The typo fix ("Bassed" → "Based") and the structured formatting enhance readability.
67-72: LGTM! Field rename and documentation improve clarity.The rename from
is_includedtoexit_location_is_includedmakes the purpose immediately clear. The expanded Javadoc effectively addresses past review feedback by explicitly stating that the two lists are not index-aligned and thatexit_locationsis a filtered subset. This prevents potential misuse.
108-108: LGTM! Constructor parameter rename is consistent.The constructor parameter rename matches the field rename, maintaining consistency throughout the codebase. The assignment correctly uses the new field name.
Also applies to: 115-115
java/daikon/dcomp/Instrument.java (8)
27-42: Good improvement: diagnostic boundaries now correctly encompass all diagnostic fields.The
transformer_seenfield is now properly placed before the "End of diagnostics" marker, addressing the previous review concern about its confusing placement.
84-99: LGTM: Improved error handling with conditional stack traces.The method rename to
writeDebugClassFilesis more descriptive, and the conditional stack trace printing (lines 94-96) appropriately limits diagnostic output to debug mode.
121-144: LGTM: Clear early returns and improved logging.The introduction of
binaryClassName(line 131) improves logging consistency throughout the method, and the clarifying comments enhance code readability.
175-178: LGTM: Appropriate skip for Java instrumentation API classes.The new check correctly skips classes in
java/lang/instrument/package, preventing instrumentation of Java's own instrumentation infrastructure which could cause conflicts.
214-219: LGTM: More accurate logging terminology.The use of "loaders" (plural) correctly reflects that two loader references are being logged in each message.
227-233: LGTM: Consistent error handling pattern with graceful degradation.The consistent conditional stack trace printing and explicit comments about returning
null(lines 232, 250, 260) improve both debugging capability and code clarity. The graceful degradation approach (returningnullto leave bytecode unchanged) is appropriate for a class transformer.Also applies to: 245-251
273-292: LGTM: Improved consistency and type safety.The parameter rename to
classNamewith@InternalFormannotation improves type safety, and the consistent early return pattern enhances readability.
302-320: LGTM: Visibility change supports extensibility for Java 24.The change from
privatetoprotected(line 302) enables subclass access, likely needed forInstrument24mentioned in the PR objectives. The parameter rename and early return refactoring maintain consistency withis_dcomp.java/daikon/dcomp/OperandStack24.java (1)
275-281: LGTM!slotSizecorrectly handles null and uses TypeKind.The implementation properly handles null (returns 1) and delegates to
TypeKind.from(item).slotSize()for non-null items, correctly returning 2 forlong/doubleand 1 for everything else.java/daikon/dcomp/BuildJDK24.java (2)
494-495: LGTM!timeFormatteris already properly declared.The
timeFormatteris declared asprivate static final, which is correct for an immutable, thread-safeDateTimeFormatter.
558-561: Javadoc correctly references ClassFile API's verifier.Previous feedback about incorrect BCEL reference has been addressed. The comment now correctly references "the ClassFile API's verifier".
java/daikon/dcomp/Instrument24.java (4)
154-161: LGTM! Null check correctly placed beforeinternalFormToBinaryName.The previous review flagged a potential NPE where
Signatures.internalFormToBinaryName(className)was called before the null check. This has been correctly fixed - the null check at line 156 now occurs before the conversion at line 161.
97-129: LGTM! Debug file writing with robust error handling.The
writeDebugClassFilesmethod properly handles errors during BCEL parsing and file writing without affecting the main instrumentation flow. Errors are logged but don't propagate, which is appropriate for debug output.
176-216: LGTM! JDK class filtering is comprehensive.The transform method properly filters:
- Pre-instrumented classes (line 171)
- JDK classes when not using instrumented JDK (line 181)
- Problem packages (line 189)
- Problem classes for Java 9+ (line 195)
- Proxy classes (line 200)
- Instrumentation classes (line 205)
- DCRuntime (line 210)
339-358: LGTM! Transformer detection heuristic is appropriate.The
is_transformermethod correctly identifies known bytecode transformation tools (Groovy, Mockito, Objenesis) that may conflict with DynComp. The one-time warning (viatransformer_seenflag) is good UX.java/daikon/dcomp/CalcStack24.java (5)
53-68: LGTM! Class documentation and NULL_CD sentinel are well-defined.The class Javadoc now correctly documents that methods may modify
DCInstrument24.localsandDCInstrument24.stacks. TheNULL_CDsentinel is appropriately named and documented.
714-716: LGTM! JSR/RET opcodes correctly fall through to error.The comment explains that JSR/RET have been illegal since JDK 7, and the opcodes correctly fall through to the default case which throws
DynCompError("Unexpected instruction opcode"). This is the appropriate handling for modern class files.
920-953: LGTM!lceToCDcorrectly handles all LoadableConstantEntry types.The method properly handles:
ClassEntry→CD_Class(line 923) - correctly pushesjava.lang.Class, not the described classConstantDynamicEntry→cde.typeSymbol()(line 926)MethodHandleEntry→java.lang.invoke.MethodHandle(line 941)MethodTypeEntry→java.lang.invoke.MethodType(line 944)Previous critical issues about
ldcclass literals have been resolved.
994-1001: LGTM! Javadoc@paramdescription is now correct.The
@param resultdescription correctly states "the result type descriptor to push (may be void or a primitive/reference type)" which matches theClassDescparameter type.
165-181: LGTM! AALOAD handling correctly addresses null arrayref case.The code properly handles the case where the arrayref is null or NULL_CD, pushing NULL_CD to maintain stack consistency. This is important for symbolic simulation where the actual value isn't known.
| @SuppressWarnings("fallthrough") | ||
| static boolean simulateInstruction(Instruction inst, int inst_index, OperandStack24 stack) { | ||
| if (DCInstrument24.stacks[inst_index] != null) { | ||
| throw new DynCompError("instruction revisited: " + inst); | ||
| } else { | ||
| DCInstrument24.stacks[inst_index] = stack.getClone(); | ||
| } | ||
| if (DCInstrument24.debugOperandStack) { | ||
| System.out.println( | ||
| "save stack state at: " + inst_index + ", " + DCInstrument24.stacks[inst_index]); | ||
| System.out.println("opcode: " + inst.opcode()); | ||
| } | ||
| // calculate stack changes |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding method name to error message for easier debugging.
When an instruction is revisited, the error message only shows the instruction. Adding method context would help debugging.
if (DCInstrument24.stacks[inst_index] != null) {
- throw new DynCompError("instruction revisited: " + inst);
+ throw new DynCompError("instruction revisited at index " + inst_index + ": " + inst);
}📝 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.
| @SuppressWarnings("fallthrough") | |
| static boolean simulateInstruction(Instruction inst, int inst_index, OperandStack24 stack) { | |
| if (DCInstrument24.stacks[inst_index] != null) { | |
| throw new DynCompError("instruction revisited: " + inst); | |
| } else { | |
| DCInstrument24.stacks[inst_index] = stack.getClone(); | |
| } | |
| if (DCInstrument24.debugOperandStack) { | |
| System.out.println( | |
| "save stack state at: " + inst_index + ", " + DCInstrument24.stacks[inst_index]); | |
| System.out.println("opcode: " + inst.opcode()); | |
| } | |
| // calculate stack changes | |
| @SuppressWarnings("fallthrough") | |
| static boolean simulateInstruction(Instruction inst, int inst_index, OperandStack24 stack) { | |
| if (DCInstrument24.stacks[inst_index] != null) { | |
| throw new DynCompError("instruction revisited at index " + inst_index + ": " + inst); | |
| } else { | |
| DCInstrument24.stacks[inst_index] = stack.getClone(); | |
| } | |
| if (DCInstrument24.debugOperandStack) { | |
| System.out.println( | |
| "save stack state at: " + inst_index + ", " + DCInstrument24.stacks[inst_index]); | |
| System.out.println("opcode: " + inst.opcode()); | |
| } | |
| // calculate stack changes |
🤖 Prompt for AI Agents
In java/daikon/dcomp/CalcStack24.java around lines 148-160, the thrown
DynCompError lacks the method context; update the throw to include the method
name (e.g. throw new DynCompError("instruction revisited: " + inst + " in
method: " + methodName)). If a method name is accessible from the Instruction
(e.g. inst.method() or inst.getOwner()/inst.getName()), use that; otherwise add
a methodName parameter to simulateInstruction and propagate it from callers so
you can append " in method: " + methodName to the error message.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
java/daikon/chicory/Instrument24.java (2)
743-836: Entry/exit instrumentation split improves clarity; iterator checks guard consistency
instrumentCodenow delegates toinstrumentInstructionList, which in turn separates entry instrumentation (addInstrumentationAtEntry) from exit handling (addInstrumentationAtExits), making the control flow easier to follow.- Entry instrumentation:
- Allocates a
this_invocation_nonceviagenerateIncrementNonceandcreateLocalWithMethodScope.- Inserts
Runtime.enterbefore the firstLineNumber/Instructionwhile preserving leadingLocalVariableand optional initialLabel.- Tracks the original first‑instruction label via
minfo.entryLabelandlabelMap.- Exit instrumentation:
- Uses
exit_location_is_includedandexit_locationsiterators, with asserts ensuring both are fully consumed after scanning allCodeElements.- For included returns, duplicates the result into
return__$trace2_valwith correct handling for category‑2 types (DUP2) and then appends a call toRuntime.exitwith the correct source line.This decomposition is a solid structural improvement, and the iterator assertions should help catch mismatches between metadata and actual return instructions.
Also applies to: 838-999
1361-1383: Descriptor and type‑name helpers are reasonable; note limitations on generics
classDescToClassGetNamecorrectly maps primitives and reference types to aClass.getName()‑like form ([Ljava.lang.String;‑style for arrays,java.lang.Foofor classes).getFqBinaryNameParameterTypesusesArraysPlume.mapArrayto derive fully qualified binary names from parameterClassDescs, which simplifies string handling increate_method_info_if_instrumented.convertDescriptorToFqBinaryName,convertClassTypeDescriptorToFqBinaryName, andconvertTypeArgumentsToBinaryNamesprovide extended support for decoding descriptors and simple signature forms (including trivial"*"wildcards).These helpers match the documented intent (“some support for signatures” and no wildcard/type‑variable handling); if you later start feeding full generic signatures (with
+,-, orT...;) through them, consider adding explicit handling or a guarded fallback to avoidIllegalArgumentExceptions on such descriptors.Also applies to: 1451-1477, 1558-1698
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
java/daikon/chicory/Instrument24.java(56 hunks)java/daikon/dcomp/BuildJDK.java(7 hunks)java/daikon/dcomp/BuildJDK24.java(1 hunks)java/daikon/dcomp/CalcStack24.java(1 hunks)java/daikon/dcomp/ClassGen24.java(1 hunks)java/daikon/dcomp/Instrument24.java(1 hunks)java/daikon/dcomp/OperandStack24.java(1 hunks)java/daikon/dcomp/StackMapUtils24.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
java/daikon/dcomp/StackMapUtils24.java (2)
java/daikon/chicory/MethodGen24.java (1)
MethodGen24(51-840)java/daikon/dcomp/DCInstrument24.java (1)
DCInstrument24(136-4946)
java/daikon/dcomp/BuildJDK.java (1)
java/daikon/dcomp/BuildJDK24.java (1)
SuppressWarnings(55-594)
java/daikon/dcomp/Instrument24.java (3)
java/daikon/DynComp.java (1)
DynComp(25-541)java/daikon/chicory/ClassInfo.java (1)
ClassInfo(20-154)java/daikon/dcomp/Premain.java (1)
Premain(32-417)
java/daikon/dcomp/CalcStack24.java (1)
java/daikon/dcomp/DCInstrument24.java (1)
DCInstrument24(136-4946)
⏰ 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). (8)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_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: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: build
🔇 Additional comments (6)
java/daikon/dcomp/BuildJDK.java (1)
466-511: StatictimeFormatter+ doc tweaks look correctMaking
timeFormatterprivate static finaland documentingclassFileNameas "internal form" aligns this implementation withBuildJDK24and avoids unnecessary per-instance formatter allocation. No functional issues spotted in the updated instrumentation method.java/daikon/dcomp/CalcStack24.java (1)
66-71: Symbolic stack simulation and helpers look consistent with JVM semantics
NULL_CDis used consistently as the null sentinel, and array / class / null merging logic appears coherent.- LDC handling via
lceToCDnow covers all modern constant kinds (class, dynamic, handle, method type, primitives, strings) with appropriateClassDescs.npaiToElementTypeDescriptorandpushResultClassDesccorrectly model NEWARRAY element descriptors and integral‑widening of stack values (e.g., byte/short/char/boolean →int).simulateCodeElement/simulateInstructionandaddLabelsToWorklistinteract cleanly withDCInstrument24’s stacks/worklist representation.No functional problems spotted in these refactored regions.
Also applies to: 85-135, 171-201, 751-759, 913-975, 977-995, 997-1013
java/daikon/dcomp/StackMapUtils24.java (1)
50-101: Local-slot shifting logic for inserted parameters/locals looks sound
addNewSpecialLocalcomputes the insertion slot afterthisand existing parameters usinglastArg.slot() + slotSize(lastArg), then adjusts later locals and instruction operands viaadjust_code_for_locals_change.adjust_code_for_locals_changecorrectly updatesRetInstruction,IncrementInstruction,LoadInstruction, andStoreInstructionwhose slot is ≥ the insertion offset.minfo.nextLocalIndexandmaxLocalsare kept in sync with the new variable’s slot size.This should safely handle both 1‑slot and 2‑slot locals for the two DynComp special variables.
Also applies to: 125-194
java/daikon/dcomp/Instrument24.java (1)
154-178: Transform flow and null‑safety forclassNamelook solid
- The
className == nullguard now precedesinternalFormToBinaryName, avoiding the NPE risk the JVM’s transform contract allows.- JDK vs non‑JDK routing (problem packages/classes, proxies, instrumentation classes, DCRuntime, dcomp/chicory/plumelib, other transformers) is consistent with the existing DynComp heuristics.
- Class parsing and instrumentation both fail soft: parse or
DCInstrument24.instrumenterrors log a message and returnnull, leaving the bytecodes unchanged.- Debug dumping of uninstrumented and instrumented bytes is correctly wrapped in try/catch so failures there do not break instrumentation.
No additional issues spotted in this transformer.
Also applies to: 241-305
java/daikon/chicory/Instrument24.java (2)
111-190: Shared ppt‑filtering (shouldIgnore) wiring across Chicory/DynComp is coherent
debug_ppt_omitis explicitly enabled from both Chicory and DynComp sides, andshouldIgnoreconsultsRuntime.ppt_omit_patternandppt_select_patternin a single place.- Exclusion precedes inclusion, and the “include only” semantics when
ppt_select_patternis non‑empty are clear and documented in the finalif (!Runtime.ppt_select_pattern.isEmpty())block.This centralizes the ppt‑filter policy cleanly for both instruments.
Also applies to: 192-229
230-270: Refactored transform path and debug dumping behave correctly
- The new
writeDebugClassFileshelper is robust: it tolerates BCEL parse failures and IO errors, logging them only when debug is enabled and otherwise not interfering with instrumentation.transformnow logs entry with the Chicory prefix, checksclassName == nullearly, filters out boot classes, Chicory’s own classes (isChicoryClass), and proxies, then parses and instruments via ClassFile API in a try/catch that returnsnullon failure.- Instrumented vs non‑instrumented cases are handled cleanly:
classInfo.shouldIncludegates the final return and optional debug class dumps.Overall, the transform flow is clearer and more failure‑tolerant than the prior version.
Also applies to: 288-377
| // Write out the list of all classes in the jar file | ||
| File jdk_classes_file = new File(dest_dir, "java/lang/jdk_classes.txt"); | ||
| System.out.printf("Writing a list of class names to %s%n", jdk_classes_file); | ||
| // Class names are written in internal form. | ||
| try (PrintWriter pw = new PrintWriter(jdk_classes_file, UTF_8.name())) { | ||
| for (String classFileName : class_stream_map.keySet()) { | ||
| pw.println(classFileName.replace(".class", "")); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Restrict jdk_classes.txt to .class entries only
The current loop writes every key from class_stream_map into jdk_classes.txt:
for (String classFileName : class_stream_map.keySet()) {
pw.println(classFileName.replace(".class", ""));
}This means non‑class resources (and module-info.class, etc.) are recorded as if they were class names, and replace(".class", "") will also silently truncate any resource name that merely contains that substring.
For downstream consumers, it’s more accurate to record only actual class entries and to remove the suffix precisely:
- for (String classFileName : class_stream_map.keySet()) {
- pw.println(classFileName.replace(".class", ""));
- }
+ for (String classFileName : class_stream_map.keySet()) {
+ if (classFileName.endsWith(".class")) {
+ pw.println(
+ classFileName.substring(0, classFileName.length() - ".class".length()));
+ }
+ }This keeps jdk_classes.txt aligned with the set of class files actually being instrumented.
🤖 Prompt for AI Agents
In java/daikon/dcomp/BuildJDK24.java around lines 190 to 199, the loop writes
every key from class_stream_map and uses replace(".class", "") which can include
non-class resources and truncate names containing ".class"; change the loop to
only handle real class files by checking that classFileName.endsWith(".class")
and skipping entries like "module-info.class", and then remove the suffix
precisely (e.g., take substring(0, classFileName.length() - 6)) before writing
to jdk_classes.txt so only actual class names are recorded.
| Map<String, InputStream> gather_runtime_from_modules() { | ||
|
|
||
| Map<String, InputStream> class_stream_map = new HashMap<>(); | ||
| FileSystem fs = FileSystems.getFileSystem(URI.create("jrt:/")); | ||
| Path modules = fs.getPath("/modules"); | ||
| // The path java_home+/lib/modules is the file in the host file system that | ||
| // corresponds to the modules file in the jrt: file system. | ||
| System.out.printf("using modules directory %s/lib/modules%n", java_home); | ||
| try (DirectoryStream<Path> directoryStream = Files.newDirectoryStream(modules, "java.base*")) { | ||
| for (Path moduleDir : directoryStream) { | ||
| gather_runtime_from_modules_directory( | ||
| moduleDir, moduleDir.toString().length(), class_stream_map); | ||
| } | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); | ||
| } | ||
| return class_stream_map; | ||
| } | ||
|
|
||
| /** | ||
| * This is a helper method for {@link #gather_runtime_from_modules}. It recurses down the module | ||
| * directory tree, selects the classes we want to instrument, creates an InputStream for each of | ||
| * these classes, and adds this information to the {@code class_stream_map} argument. | ||
| * | ||
| * @param path module file, which might be subdirectory | ||
| * @param modulePrefixLength length of "/module/..." path prefix before start of actual member | ||
| * path | ||
| * @param class_stream_map a map from class file name to InputStream that collects the results | ||
| */ | ||
| @SuppressWarnings("builder:required.method.not.called") // assignment into collection of @Owning | ||
| void gather_runtime_from_modules_directory( | ||
| Path path, int modulePrefixLength, Map<String, InputStream> class_stream_map) { | ||
|
|
||
| if (Files.isDirectory(path)) { | ||
| try (DirectoryStream<Path> directoryStream = Files.newDirectoryStream(path)) { | ||
| for (Path subpath : directoryStream) { | ||
| gather_runtime_from_modules_directory(subpath, modulePrefixLength, class_stream_map); | ||
| } | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); | ||
| } | ||
| } else { | ||
| String entryName = path.toString().substring(modulePrefixLength + 1); | ||
| 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.
Close module InputStreams for skipped entries (resource leak on JDK 9+ path)
For Java 9+ you populate class_stream_map in gather_runtime_from_modules_directory with Files.newInputStream(path) for every module file:
InputStream is = Files.newInputStream(path);
class_stream_map.put(entryName, is);In instrument_classes:
- You never close the streams for:
module-info.class(explicitcontinue;).- Non‑
.classentries whenBcelUtil.javaVersion > 8(you log andcontinue;without touching the stream).
Only .class files that reach the parsing block are wrapped in try (InputStream is = class_stream_map.get(classFileName)) { ... }, so their streams are closed.
On a full java.base run this leaves a number of InputStreams open until process exit, which is avoidable and was flagged in an earlier review.
One minimal fix is to close streams when you skip:
if (classFileName.equals("module-info.class")) {
System.out.printf("Skipping file %s%n", classFileName);
+ try (InputStream in = class_stream_map.get(classFileName)) {
+ // close skipped stream
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
continue;
}
@@
if (!classFileName.endsWith(".class") || classFileName.equals("java/lang/Object.class")) {
if (BcelUtil.javaVersion > 8) {
if (verbose) {
System.out.printf("Skipping file %s%n", classFileName);
}
+ try (InputStream in = class_stream_map.get(classFileName)) {
+ // close skipped stream
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
continue;
}Alternatively, you could avoid putting non‑.class entries into class_stream_map in the first place for the modules path.
Also applies to: 349-380, 396-425
🤖 Prompt for AI Agents
java/daikon/dcomp/BuildJDK24.java lines 290-341 (also apply similar fixes to
349-380 and 396-425): the code opens InputStreams for every module file and
stores them in class_stream_map, but later skips many entries (module-info.class
and non-*.class on JDK>8) without closing their streams, causing resource leaks;
fix by either (A) changing gather_runtime_from_modules_directory to only put
.class entries into class_stream_map (avoid opening streams for non-class
files), or (B) if retaining the current map, ensure any time you skip an entry
in instrument_classes you first close the associated InputStream (retrieve it
from class_stream_map and call close(), handling IOExceptions) and remove it
from the map before continue; apply the same pattern to the other referenced
ranges.
| /** | ||
| * Add an interface to this class. | ||
| * | ||
| * @param name the interface name, in binary format | ||
| */ | ||
| public void addInterface(@BinaryName String name) { | ||
| String internalName = name.replace('.', '/'); | ||
| for (ClassEntry existing : interfaceList) { | ||
| if (existing.asInternalName().equals(internalName)) { | ||
| return; | ||
| } | ||
| } | ||
| ClassDesc ue = ClassDesc.of(name); | ||
| ClassEntry ce = classBuilder.constantPool().classEntry(ue); | ||
| interfaceList.add(ce); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider returning an unmodifiable interfaceList view
addInterface correctly avoids duplicates by comparing internal names, and interfaceList is now instance‑scoped. However, getInterfaceList() returns the mutable internal list directly:
public List<ClassEntry> getInterfaceList() {
return interfaceList;
}Current callers (e.g., passing into ClassBuilder.withInterfaces) appear to only read, but exposing the raw list makes it easy for future code to mutate interfaceList accidentally and diverge from the underlying ClassModel.
If external mutation isn’t required, prefer:
@@
- public List<ClassEntry> getInterfaceList() {
- return interfaceList;
- }
+ public List<ClassEntry> getInterfaceList() {
+ return java.util.Collections.unmodifiableList(interfaceList);
+ }This keeps the internal state under ClassGen24’s control while preserving existing read‑only usage.
Also applies to: 202-209
🤖 Prompt for AI Agents
In java/daikon/dcomp/ClassGen24.java around lines 94-109 (and similarly at lines
202-209), getInterfaceList currently returns the mutable internal interfaceList
exposing internal state; change the getter to return an unmodifiable view (e.g.,
wrap with Collections.unmodifiableList or use List.copyOf) so callers can read
but not mutate the internal list, and keep internal methods (like addInterface)
operating on the modifiable field.
| /** | ||
| * Returns true if and only if this OperandStack equals another, meaning equal lengths and equal | ||
| * objects on the stacks. This method is used to verify that the operand stacks match when two | ||
| * execution paths meet. A special case is {@code null} on an operand stack, which matches | ||
| * anything but a primitive. As we are assuming that the class file is valid, then it is okay for | ||
| * an object to exist on one execution path and be null on the other. | ||
| */ | ||
| @Override | ||
| public boolean equals(@GuardSatisfied OperandStack24 this, @GuardSatisfied @Nullable Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (!(o instanceof OperandStack24)) { | ||
| return false; | ||
| } | ||
| final OperandStack24 other = (OperandStack24) o; | ||
| if (this.stack.size() != other.stack.size()) { | ||
| return false; | ||
| } | ||
| for (int i = 0; i < this.stack.size(); i++) { | ||
| ClassDesc thisItem = this.stack.get(i); | ||
| ClassDesc otherItem = other.stack.get(i); | ||
| if (!compareOperandStackElements(thisItem, otherItem)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if and only if the two OperandStack elements are equal. An element may be one of | ||
| * four possible items: | ||
| * | ||
| * <ul> | ||
| * <li>{@code null} a special case as it matches anything but a primitive | ||
| * <li>a primitive (such as {@code int}, {@code long}, etc.) | ||
| * <li>an array reference | ||
| * <li>an object reference | ||
| * </ul> | ||
| * | ||
| * @param thisItem one OperandStack element | ||
| * @param otherItem another OperandStack element | ||
| * @return true if and only if the items match | ||
| */ | ||
| protected boolean compareOperandStackElements( | ||
| @GuardSatisfied OperandStack24 this, | ||
| @Nullable ClassDesc thisItem, | ||
| @Nullable ClassDesc otherItem) { | ||
| if (thisItem == null) { | ||
| if (otherItem != null && otherItem.isPrimitive()) { | ||
| return false; | ||
| } | ||
| // We assume null matches any array or any class. | ||
| return true; | ||
| } else if (otherItem == null) { | ||
| // we know thisItem != null | ||
| if (thisItem.isPrimitive()) { | ||
| return false; | ||
| } | ||
| // We assume null matches any array or any class. | ||
| return true; | ||
| } else if (thisItem.isArray()) { | ||
| if (otherItem.isPrimitive()) { | ||
| return false; | ||
| } | ||
| // We assume an array matches any array or any class as they all have Object as a superclass | ||
| // at some point. | ||
| return true; | ||
| } else if (thisItem.isClassOrInterface()) { | ||
| if (otherItem.isPrimitive()) { | ||
| return false; | ||
| } | ||
| // We assume a class matches any array or any class as they all have Object as a superclass | ||
| // at some point. | ||
| return true; | ||
| } else if (thisItem.isPrimitive() && !otherItem.isPrimitive()) { | ||
| return false; | ||
| } | ||
| // Both operands are primitives - they better match. | ||
| if (thisItem.equals(otherItem)) { | ||
| return true; | ||
| } else { | ||
| throw new DynCompError( | ||
| "Operand stack primitives don't match: " + thisItem + ", " + otherItem); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix equals/hashCode contract violation for OperandStack24
equals deliberately treats many different values as equivalent:
nullmatches any non‑primitive (arrayorclass).- Any array matches any non‑primitive.
- Any class matches any non‑primitive.
- Primitives must match exactly or a
DynCompErroris thrown.
However, hashCode distinguishes null from non‑primitive entries:
null→elementHash = 0- any non‑primitive, non‑null →
elementHash = 2
So stacks like [null] and [someNonPrimitive] compare equal but get different hash codes, breaking the equals/hashCode contract. This is dangerous if OperandStack24 is ever used as a key in hash‑based collections (which is likely in the symbolic execution / worklist logic).
You need to ensure all values that are equal under compareOperandStackElements produce the same per‑element hash bucket. One straightforward fix is:
- Treat all non‑primitive (including
null, arrays, classes) as a single bucket. - Distinguish primitives by their
ClassDesc.hashCode()as you do now.
For example:
@Override
public int hashCode(@GuardSatisfied OperandStack24 this) {
int result = 1;
for (ClassDesc item : stack) {
- int elementHash;
- if (item == null) {
- elementHash = 0; // matches any non-primitive in equals
- } else if (item.isPrimitive()) {
- elementHash = item.hashCode(); // distinguish different primitive types
- } else {
- elementHash = 2; // all non-primitive, non-null items are equivalent in equals
- }
+ int elementHash;
+ if (item != null && item.isPrimitive()) {
+ // Primitives must match exactly in equals, so use their own hash code.
+ elementHash = item.hashCode();
+ } else {
+ // `null`, arrays, and classes are all treated as equivalent in equals.
+ elementHash = 2;
+ }
result = 31 * result + elementHash;
}
return result;
}This preserves the coarse equivalence from equals while restoring the required equals/hashCode contract.
Also applies to: 171-189
🤖 Prompt for AI Agents
In java/daikon/dcomp/OperandStack24 around lines 75-160 (and also apply same fix
for the other hashCode region at 171-189), the equals implementation treats
null, any array, and any class as equivalent but the current hashCode
distinguishes null vs non-null non-primitives; update the per-element hash
computation so all non-primitive entries (null, arrays, classes) map to the same
bucket (e.g., a constant value like 0 or 2) while primitives use their
ClassDesc.hashCode() as before; modify the loop that builds the overall hash to
compute elementHash = (item == null || !item.isPrimitive()) ?
NON_PRIMITIVE_BUCKET : item.hashCode(), combine these into the final result, and
ensure both hashCode locations use the same logic so equals/hashCode contract
holds.
2 participants
No description provided.