Skip to content

Commit

Permalink
Write end offsets to _debug even if validation fails
Browse files Browse the repository at this point in the history
Fixes a regression introduced in
7154301 that was well noticeable in the
devel Web IDE with the
kaitai-io/kaitai_struct_webide#156 feature: a
field with a failed validation (specified by the `contents` or `valid`
key) no longer had its byte range highlighted in the hex dump. This is
because the aforementioned commit changed the order of operations in the
generated JavaScript code (in debug mode, or more precisely with
`--read-pos`) so that the assignment of the end offset to the `_debug`
map for a field with `contents` or `valid` was done *after* the `if`
statement performing the validation, which meant that the end offset was
not assigned if the validation failed.

A nice side effect of this commit is that it eliminates duplicate
`this._debug.{ATTR_NAME}.arr[i]` assignments in the case of a field with
type switching and `repeat`, such as when you compile
https://github.com/kaitai-io/kaitai_struct_tests/blob/9f782819b70db6069674995b969d2b801c86a515/formats/switch_repeat_expr.ksy
in `--read-pos` mode (in JavaScript, it removes the
`this._debug.body.arr[i] = ...` assignments from every branch of the
`switch` statement, which are indeed unnecessary, because the exact same
assignments already exist around the `switch` statement as well).

Note: this commit unfortunately doesn't have any impact on the current
set of test formats in
[`formats/`](https://github.com/kaitai-io/kaitai_struct_tests/tree/9f782819b70db6069674995b969d2b801c86a515/formats)
when compiled with the usual settings in
[`build-formats`](https://github.com/kaitai-io/kaitai_struct_tests/blob/9f782819b70db6069674995b969d2b801c86a515/build-formats).
This is because we only test the debug mode on 4 formats that have the
`meta/ks-debug: true` key, and none of them has a field with `contents`
or `valid`. But if you add `--debug` to the KSC incantation in
`build-formats`, you will see the changes.
  • Loading branch information
generalmimon committed Aug 26, 2024
1 parent 3b69fb1 commit 6048662
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,11 @@ trait CommonReads extends LanguageCompiler {
normalIO
}

val needsDebug = attrDebugNeeded(id)
val needsArrayDebug = attrDebugNeeded(id) && attr.cond.repeat != NoRepeat

if (needsDebug) {
if (needsArrayDebug) {
attrDebugStart(id, attr.dataType, Some(io), NoRepeat)
if (attr.cond.repeat != NoRepeat)
attrDebugArrInit(id, attr.dataType)
attrDebugArrInit(id, attr.dataType)
}

defEndian match {
Expand All @@ -45,7 +44,7 @@ trait CommonReads extends LanguageCompiler {
attrParse0(id, attr, io, Some(fe))
}

if (needsDebug)
if (needsArrayDebug)
attrDebugEnd(id, attr.dataType, io, NoRepeat)

// More position management + set calculated flag after parsing for ParseInstanceSpecs
Expand Down Expand Up @@ -75,7 +74,16 @@ trait CommonReads extends LanguageCompiler {
condRepeatUntilHeader(id, io, attr.dataType, untilExpr)
case NoRepeat =>
}

val needsDebug = attrDebugNeeded(id)
if (needsDebug)
attrDebugStart(id, attr.dataType, Some(io), attr.cond.repeat)

attrParse2(id, attr.dataType, io, attr.cond.repeat, false, defEndian)

if (needsDebug)
attrDebugEnd(id, attr.dataType, io, attr.cond.repeat)

attrValidateAll(attr)
attr.cond.repeat match {
case RepeatEos =>
Expand All @@ -94,7 +102,15 @@ trait CommonReads extends LanguageCompiler {
def attrDebugArrInit(attrId: Identifier, attrType: DataType): Unit = {}
def attrDebugEnd(attrName: Identifier, attrType: DataType, io: String, repeat: RepeatSpec): Unit = {}

def attrDebugNeeded(attrId: Identifier): Boolean = false
def attrDebugNeeded(attrId: Identifier): Boolean = {
if (!config.readStoresPos)
return false

attrId match {
case _: NamedIdentifier | _: NumberedIdentifier | _: InstanceIdentifier => true
case _ => false
}
}

/**
* Runs all validation procedures requested for an attribute.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ trait EveryReadIsExpression
assignTypeOpt: Option[DataType] = None
): Unit = {
val assignType = assignTypeOpt.getOrElse(dataType)
val needsDebug = attrDebugNeeded(id) && rep != NoRepeat

if (needsDebug)
attrDebugStart(id, dataType, Some(io), rep)

dataType match {
case t: UserType =>
Expand All @@ -61,9 +57,6 @@ trait EveryReadIsExpression
val expr = parseExpr(dataType, assignType, io, defEndian)
handleAssignment(id, expr, rep, isRaw)
}

if (needsDebug)
attrDebugEnd(id, dataType, io, rep)
}

def attrBytesTypeParse(
Expand Down Expand Up @@ -255,14 +248,4 @@ trait EveryReadIsExpression
attrDebugStart(instName, dataType, None, NoRepeat)
handleAssignmentSimple(instName, expression(value))
}

override def attrDebugNeeded(attrId: Identifier): Boolean = {
if (!config.readStoresPos)
return false

attrId match {
case _: NamedIdentifier | _: NumberedIdentifier | _: InstanceIdentifier => true
case _ => super.attrDebugNeeded(attrId)
}
}
}

0 comments on commit 6048662

Please sign in to comment.