ctf: Add new field classes to complete ctf2 parsing#400
Open
arfio wants to merge 1 commit into
Open
Conversation
Signed-off-by: Arnaud Fiorini <fiorini.arnaud@gmail.com>
📝 WalkthroughWalkthroughThis PR adds boolean and dynamic blob field types to the CTF core event system, including declaration/definition pairs, TSDL metadata parsing, type alias integration, and event field rendering. The bundle version is bumped to 5.2.0. ChangesCTF Type System Expansion: Boolean and Dynamic Blob Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java (1)
583-599: ⚡ Quick winCache formatted blob output to avoid repeated Base64 work.
getFormattedValue()re-encodes bytes on every call; with dynamic blobs this can become expensive in repeated rendering paths. Cache the formatted string once (same pattern already used by other field classes here).♻️ Proposed refactor
final class CTFHexBlobField extends CtfTmfEventField { - private String fBlobType; + private final String fBlobType; + private `@Nullable` String fFormattedValue; @@ `@Override` - public String getFormattedValue() { - String encoded = Base64.getEncoder().encodeToString(getValue()); - return "[" + fBlobType + "]<" + encoded + '>'; //$NON-NLS-1$ //$NON-NLS-2$ + public synchronized String getFormattedValue() { + if (fFormattedValue == null) { + String encoded = Base64.getEncoder().encodeToString(getValue()); + fFormattedValue = "[" + fBlobType + "]<" + encoded + '>'; //$NON-NLS-1$ //$NON-NLS-2$ + } + return fFormattedValue; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java` around lines 583 - 599, getFormattedValue() currently Base64-encodes getValue() on every call; add a lazily-initialized cached String field (e.g., fFormattedValue) on CTFHexBlobField and compute the "[" + fBlobType + "]<" + Base64.getEncoder().encodeToString(getValue()) + '>' only once in getFormattedValue(), store it in the cache and return the cached value on subsequent calls; follow the same lazy-cache pattern and naming conventions used by other field classes, and make the cache safe for concurrent reads (e.g., a volatile field or simple double-check idiom) so repeated rendering avoids repeated Base64 work.ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDefinition.java (1)
58-60: ⚡ Quick winReturn a defensive copy from
getBytes().
getBytes()currently exposes internal mutable state. Any caller can mutatefArrayand alter this definition instance unexpectedly.Proposed fix
`@Override` public byte[] getBytes() { - return fArray; + return fArray.clone(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDefinition.java` around lines 58 - 60, DynamicBlobDefinition.getBytes exposes the internal mutable byte[] fArray; change getBytes() to return a defensive copy (e.g., clone or Arrays.copyOf) of fArray instead of the array reference, and handle the null case consistently (return null or an empty array as appropriate) so callers cannot mutate the internal state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BooleanDeclaration.java`:
- Around line 50-55: In createDefinition, ensure the original ByteOrder
(variable byteOrder) is always restored on the BitBuffer even if read(input)
throws: wrap the call to read(input) (and any code that depends on the temporary
fByteOrder) in a try/finally and call input.setByteOrder(byteOrder) in the
finally block so the buffer state is restored before returning or propagating
the exception; reference createDefinition, read(Input)/read(input), fByteOrder,
input.setByteOrder(...), and BooleanDefinition to locate the change.
- Around line 58-63: The BooleanDeclaration.read method incorrectly increments
the chunk index by 1 bit; change the loop to advance by 64-bit chunks and
compute the remaining bits per iteration. Specifically, in read(BitBuffer input)
update the for-loop to iterate with "for (int i = 0; i < fLength; i += 64)" and
use "int remaining = fLength - i" then call input.get(Math.min(64, remaining),
false) so BitBuffer.get is always passed a valid chunk length and the reader
advances by full 64-bit chunks.
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDeclaration.java`:
- Around line 41-43: The constructor DynamicBlobDeclaration currently accepts a
nullable lengthName and stores it in fLengthName, but later code (including
other occurrences around the constructor and at the usages noted) dereferences
or compares fLengthName directly; modify all comparisons and dereferences of
fLengthName (including in methods referenced near lines 51-52 and 122-123) to
handle null safely by using null-safe checks (e.g., check fLengthName != null
before calling equals/compare, or use Objects.equals(fLengthName, other) /
Objects.equals(other, fLengthName)) and avoid direct method calls on fLengthName
when it may be null, ensuring the constructor and any equality/lookup logic
account for a null lengthName value.
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/DynamicBlobDeclarationParser.java`:
- Around line 72-79: The code currently dereferences
lengthFieldLocation/length-field-location.path without validation in
DynamicBlobDeclarationParser, which can cause runtime exceptions; update the
parsing logic to first confirm lengthFieldLocation.isJsonObject() and that the
object has JsonMetadataStrings.PATH, then ensure the path element is either a
non-empty JsonArray whose last element is a JsonPrimitive string or a
JsonPrimitive string; if any check fails (missing path, path not
object/array/string, empty array, or non-string last element) throw a
ParseException with a clear message; replace direct calls to
getAsJsonObject/getAsJsonArray/getAsString for
lengthFieldLocation/pathElement/lengthName with guarded checks that produce
deterministic ParseException errors.
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.java`:
- Around line 196-201: The switch branches for
JsonMetadataStrings.FIXED_LENGTH_BIT_ARRAY and
JsonMetadataStrings.FIXED_LENGTH_BIT_MAP in TypeAliasParser currently break
without assigning targetDeclaration, which later causes a null registration;
change these branches to throw a ParseException (with a clear message mentioning
the unsupported field class) instead of breaking so parsing fails fast. Locate
the switch handling field class in TypeAliasParser (the cases for
FIXED_LENGTH_BIT_ARRAY and FIXED_LENGTH_BIT_MAP) and replace the break logic
with code that raises a ParseException referencing the offending field class and
the alias name being processed, ensuring the method never reaches the later
register-type-alias path with a null targetDeclaration.
---
Nitpick comments:
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDefinition.java`:
- Around line 58-60: DynamicBlobDefinition.getBytes exposes the internal mutable
byte[] fArray; change getBytes() to return a defensive copy (e.g., clone or
Arrays.copyOf) of fArray instead of the array reference, and handle the null
case consistently (return null or an empty array as appropriate) so callers
cannot mutate the internal state.
In
`@ctf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java`:
- Around line 583-599: getFormattedValue() currently Base64-encodes getValue()
on every call; add a lazily-initialized cached String field (e.g.,
fFormattedValue) on CTFHexBlobField and compute the "[" + fBlobType + "]<" +
Base64.getEncoder().encodeToString(getValue()) + '>' only once in
getFormattedValue(), store it in the cache and return the cached value on
subsequent calls; follow the same lazy-cache pattern and naming conventions used
by other field classes, and make the cache safe for concurrent reads (e.g., a
volatile field or simple double-check idiom) so repeated rendering avoids
repeated Base64 work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26a2a8a9-0946-48b0-805a-b76f0a0e7a86
📒 Files selected for processing (13)
ctf/org.eclipse.tracecompass.ctf.core/META-INF/MANIFEST.MFctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDeclaration.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BlobDefinition.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BooleanDeclaration.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BooleanDefinition.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDeclaration.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDefinition.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/BlobDeclarationParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/DynamicBlobDeclarationParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/TypeAliasParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/event/metadata/tsdl/bool/BooleanDeclarationParser.javactf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/internal/ctf/core/utils/JsonMetadataStrings.javactf/org.eclipse.tracecompass.tmf.ctf.core/src/org/eclipse/tracecompass/tmf/ctf/core/event/CtfTmfEventField.java
Comment on lines
+58
to
+63
| private boolean read(BitBuffer input) throws CTFException { | ||
| for (int i = 0; i < fLength; i++) { | ||
| if (input.get(Math.min(64, fLength - i * 64), false) != 0) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix chunk iteration in boolean bit reader.
The loop advances by 1 bit-index but subtracts i * 64, so after the first iteration it can request invalid lengths and misread input for booleans wider than 1 bit.
Proposed fix
private boolean read(BitBuffer input) throws CTFException {
- for (int i = 0; i < fLength; i++) {
- if (input.get(Math.min(64, fLength - i * 64), false) != 0) {
+ int remaining = fLength;
+ while (remaining > 0) {
+ int chunkSize = Math.min(64, remaining);
+ if (input.get(chunkSize, false) != 0) {
return true;
}
+ remaining -= chunkSize;
}
return false;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/BooleanDeclaration.java`
around lines 58 - 63, The BooleanDeclaration.read method incorrectly increments
the chunk index by 1 bit; change the loop to advance by 64-bit chunks and
compute the remaining bits per iteration. Specifically, in read(BitBuffer input)
update the for-loop to iterate with "for (int i = 0; i < fLength; i += 64)" and
use "int remaining = fLength - i" then call input.get(Math.min(64, remaining),
false) so BitBuffer.get is always passed a valid chunk length and the reader
advances by full 64-bit chunks.
Comment on lines
+41
to
+43
| public DynamicBlobDeclaration(@Nullable String lengthName, String mediaType) { | ||
| fLengthName = lengthName; | ||
| fMediaType = mediaType; |
There was a problem hiding this comment.
Guard nullable lengthName and compare safely.
fLengthName is nullable at construction but later dereferenced directly, which can cause runtime failures.
Proposed fix
+import java.util.Objects;
import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jdt.annotation.Nullable;
@@
- private String fLengthName;
- private String fMediaType;
+ private final `@Nullable` String fLengthName;
+ private final String fMediaType;
@@
public `@NonNull` Definition createDefinition(IDefinitionScope definitionScope, `@NonNull` String fieldName, `@NonNull` BitBuffer input) throws CTFException {
+ if (fLengthName == null || fLengthName.isBlank()) {
+ throw new CTFException("Dynamic blob length field location is missing"); //$NON-NLS-1$
+ }
IDefinition lenDef = null;
@@
- return (fLengthName.equals(otherBlob.fLengthName));
+ return Objects.equals(fLengthName, otherBlob.fLengthName);
}Also applies to: 51-52, 122-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ctf/org.eclipse.tracecompass.ctf.core/src/org/eclipse/tracecompass/ctf/core/event/types/DynamicBlobDeclaration.java`
around lines 41 - 43, The constructor DynamicBlobDeclaration currently accepts a
nullable lengthName and stores it in fLengthName, but later code (including
other occurrences around the constructor and at the usages noted) dereferences
or compares fLengthName directly; modify all comparisons and dereferences of
fLengthName (including in methods referenced near lines 51-52 and 122-123) to
handle null safely by using null-safe checks (e.g., check fLengthName != null
before calling equals/compare, or use Objects.equals(fLengthName, other) /
Objects.equals(other, fLengthName)) and avoid direct method calls on fLengthName
when it may be null, ensuring the constructor and any equality/lookup logic
account for a null lengthName value.
MatthewKhouzam
approved these changes
May 22, 2026
MatthewKhouzam
approved these changes
May 22, 2026
Contributor
MatthewKhouzam
left a comment
MatthewKhouzam
left a comment
There was a problem hiding this comment.
Looks great. add unit tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What it does
How to test
Follow-ups
Review checklist
Summary by CodeRabbit