From 604866249cab73c51838ffedb2c672073dd55e7a Mon Sep 17 00:00:00 2001 From: Petr Pucil Date: Mon, 26 Aug 2024 16:53:18 +0200 Subject: [PATCH] Write end offsets to `_debug` even if validation fails Fixes a regression introduced in 7154301a257423e5d68b200730be1c029e660433 that was well noticeable in the devel Web IDE with the https://github.com/kaitai-io/kaitai_struct_webide/issues/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. --- .../languages/components/CommonReads.scala | 28 +++++++++++++++---- .../components/EveryReadIsExpression.scala | 17 ----------- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/shared/src/main/scala/io/kaitai/struct/languages/components/CommonReads.scala b/shared/src/main/scala/io/kaitai/struct/languages/components/CommonReads.scala index 57ee5a1ea..8fcc66ae1 100644 --- a/shared/src/main/scala/io/kaitai/struct/languages/components/CommonReads.scala +++ b/shared/src/main/scala/io/kaitai/struct/languages/components/CommonReads.scala @@ -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 { @@ -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 @@ -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 => @@ -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. diff --git a/shared/src/main/scala/io/kaitai/struct/languages/components/EveryReadIsExpression.scala b/shared/src/main/scala/io/kaitai/struct/languages/components/EveryReadIsExpression.scala index 85eda7842..887239370 100644 --- a/shared/src/main/scala/io/kaitai/struct/languages/components/EveryReadIsExpression.scala +++ b/shared/src/main/scala/io/kaitai/struct/languages/components/EveryReadIsExpression.scala @@ -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 => @@ -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( @@ -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) - } - } }