tuxji commented on code in PR #901: URL: https://github.com/apache/daffodil/pull/901#discussion_r1080665074
########## daffodil-runtime2/src/test/resources/org/apache/daffodil/runtime2/variablelen.dfdl.xsd: ########## @@ -0,0 +1,121 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> + +<xs:schema + elementFormDefault="qualified" + xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"; + xmlns:xs="http://www.w3.org/2001/XMLSchema";> + + <!-- Binary data format bindings --> + + <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/> + <xs:annotation> + <xs:appinfo source="http://www.ogf.org/dfdl/";> + <dfdl:format + ref="GeneralFormat" + binaryBooleanFalseRep="0" + binaryBooleanTrueRep="1" + fillByte="%NUL;" + prefixIncludesPrefixLength="no" + representation="binary"/> + </xs:appinfo> + </xs:annotation> + + <!-- Types for testing --> + + <!-- Unsuitable since variable length array must allow 0 to 16 numbers --> Review Comment: I mean that this fixedType definition is unsuitable for implementing the intended data format since it has a variablelen array element which must be able to hold from 0 to 16 numbers in the array. I only wanted to demonstrate that arrays can have fixed sizes which can be suitable for some data formats but unsuitable for other data formats. ########## daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala: ########## @@ -404,76 +542,117 @@ class CodeGeneratorState(private val root: ElementBase) { finalStructs += struct } - def addSimpleTypeStatements(initERDStatement: String, initSelfStatement: String, parseStatement: String, unparseStatement: String): Unit = { - if (initERDStatement.nonEmpty) structs.top.initERDStatements += initERDStatement - if (initSelfStatement.nonEmpty) structs.top.initSelfStatements += initSelfStatement + // Add C code to initialize, parse, and unparse a primitive value element + def addSimpleTypeStatements(initERDStatement: String, parseStatement: String, unparseStatement: String): Unit = { + if (initERDStatement.nonEmpty) { + if (hasChoice) + structs.top.initChoiceStatements += initERDStatement + else + structs.top.initERDStatements += initERDStatement + } if (parseStatement.nonEmpty) structs.top.parserStatements += parseStatement if (unparseStatement.nonEmpty) structs.top.unparserStatements += unparseStatement } - def addComplexTypeStatements(child: ElementBase): Unit = { + // Generate C code to initialize, parse, and unparse a child element + def addPerChildStatements(child: ElementBase): Unit = { val C = cStructName(child) - val e = child.name - val hasChoice = structs.top.initChoiceStatements.nonEmpty - val arraySize = if (child.occursCountKind == OccursCountKind.Fixed) child.maxOccurs else 0 + val e = child.namedQName.local + // Generate a choice statement case if the child is in a choice element + val hasChoice = structs.top.initChoiceStatements.nonEmpty if (hasChoice) { + val position = child.position structs.top.initChoiceStatements ++= ChoiceBranchKeyCooker.convertConstant( child.choiceBranchKey, child, forUnparse = false).map { key => s" case $key:"} - - val offset = child.position - 1 - val initChoiceStatement = s" instance->_choice = $offset;" - val parseStatement = s" case $offset:" - val unparseStatement = s" case $offset:" - - structs.top.initChoiceStatements += initChoiceStatement - structs.top.parserStatements += parseStatement - structs.top.unparserStatements += unparseStatement + structs.top.initChoiceStatements += s" instance->_choice = $position;" + structs.top.parserStatements += s" case $position:" + structs.top.unparserStatements += s" case $position:" } + val indent1 = if (hasChoice) " " else "" - def addStatements(deref: String): Unit = { - val initChoiceStatement = s" ${C}_initERD(&instance->$e$deref);" - val initERDStatement = s" ${C}_initERD(&instance->$e$deref);" - val initSelfStatement = s" ${C}_initSelf(&instance->$e$deref);" - val moreIndent = if (hasChoice) " " else "" - val parseStatement = - s"""$moreIndent ${C}_parseSelf(&instance->$e$deref, pstate); - |$moreIndent if (pstate->error) return;""".stripMargin - val unparseStatement = - s"""$moreIndent ${C}_unparseSelf(&instance->$e$deref, ustate); - |$moreIndent if (ustate->error) return;""".stripMargin + // Add an array if the child occurs multiple times + val hasArray = arrayMaxOccurs(child) > 0 + val deref = if (hasArray) "[i]" else "" + val indent2 = if (hasArray) " " else "" + if (hasArray) pushArray(child) + // Generate statements to initialize, parse, and unparse the child + Runtime2CodeGenerator.generateCode(child.enclosedElement, this) + if (!child.isSimpleType) { if (hasChoice) - structs.top.initChoiceStatements += initChoiceStatement + structs.top.initChoiceStatements += s"$indent2 ${C}_initERD(&instance->$e$deref, (InfosetBase *)instance);" else - structs.top.initERDStatements += initERDStatement - structs.top.initSelfStatements += initSelfStatement - structs.top.parserStatements += parseStatement - structs.top.unparserStatements += unparseStatement + structs.top.initERDStatements += s"$indent2 ${C}_initERD(&instance->$e$deref, (InfosetBase *)instance);" + structs.top.parserStatements += + s"""$indent1$indent2 ${C}_parseSelf(&instance->$e$deref, pstate); + |$indent1$indent2 if (pstate->error) return;""".stripMargin + structs.top.unparserStatements += + s"""$indent1$indent2 ${C}_unparseSelf(&instance->$e$deref, ustate); + |$indent1$indent2 if (ustate->error) return;""".stripMargin } - if (arraySize > 0) - for (i <- 0 until arraySize) - addStatements(s"[$i]") - else - addStatements("") - if (hasChoice) { - val initChoiceStatement = s" break;" - val parseStatement = s" break;" - val unparseStatement = s" break;" + // Generate statements to align the next element + val mustAlign = child.endingAlignmentApprox.nBits > child.priorAlignmentApprox.nBits Review Comment: That's a worthwhile idea for a future PR, not part of this PR. ########## daffodil-core/src/main/scala/org/apache/daffodil/grammar/AlignedMixin.scala: ########## @@ -138,7 +138,7 @@ trait AlignedMixin extends GrammarMixin { self: Term => // The separator is text in some encoding, might not be the same as this term's encoding, and // the alignment will be left where that text leaves it. // - private lazy val priorAlignmentApprox: AlignmentMultipleOf = LV('priorAlignmentApprox) { + lazy val priorAlignmentApprox: AlignmentMultipleOf = LV('priorAlignmentApprox) { Review Comment: I have been trying Steve's suggestion to replace the elementChildren for-loop in `elementParseAndUnspecifiedLengthGenerateCode(g, cgState)` with a recursive call of `Runtime2CodeGenerator.generateCode(g.eGram, cgState)`. I have confirmed that the padtest test case steps into an AlignmentFill with 32-bit alignment which makes Steve's suggestion better than calling isKnownToBeAligned (besides, I have already tried calling isKnownToBeAligned and found it is true for every element in the padtest test case). Unfortunately, the elementChildren for-loop in `elementParseAndUnspecifiedLengthGenerateCode(g, cgState)` was the key piece of logic used by the code generator to generate C code for elements. I have been working on replacing the logic since yesterday, but it is taking a lot of time to get the C code generated correctly again. The schema compiler and the grammar intermediate objects it produces really need to be refactored and given a walk API so that separable runtimes/back-ends can convert the intermediate objects into runtime objects (runtime1), generated code (runtime2), or other formal artifacts (see [DAFFODIL-2536](https://issues.apache.org/jira/browse/DAFFODIL-2536) and Refactoring for Separable Runtimes/Back-ends in [Roadmap for Upcoming Releases](https://cwiki.apache.org/confluence/display/DAFFODIL/Roadmap+for+Upcoming+Releases)). For now, I'm trying to fix the logic to walk grammar objects manually class by class using a large match expression in Runtime2CodeGenerator .generateCode and further logic in generator trait methods as I've done before. ########## daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/HexBinaryCodeGenerator.scala: ########## @@ -60,76 +62,70 @@ trait HexBinaryCodeGenerator extends BinaryValueCodeGenerator { val primType = s"$intType$intLen" val conv = if (e.prefixedLengthElementDecl.byteOrderEv.constValue == ByteOrder.BigEndian) "be" else "le" val function = s"${conv}_$primType" - val i = if (deref.length > 2) deref.substring(1, deref.length - 1) else "" - val lenVar = s"_l_$localName$i" + val lenVar = s"_l_$localName" val initERDStatement = - s""" $field.array = NULL; - | $field.lengthInBytes = 0; - | $field.dynamic = true;""".stripMargin - val initSelfStatement = "" + s"""$indent1$indent2 $field.dynamic = true;""".stripMargin val parseStatement = - s""" ${primType}_t $lenVar; - | parse_$function(&$lenVar, $intLen, pstate); - | if (pstate->error) return; - | alloc_hexBinary(&$field, $lenVar, pstate); - | if (pstate->error) return; - | parse_hexBinary(&$field, pstate); - | if (pstate->error) return;""".stripMargin + s"""$indent1$indent2 ${primType}_t $lenVar; + |$indent1$indent2 parse_$function(&$lenVar, $intLen, pstate); + |$indent1$indent2 if (pstate->error) return; + |$indent1$indent2 alloc_hexBinary(&$field, $lenVar, pstate); + |$indent1$indent2 if (pstate->error) return; + |$indent1$indent2 parse_hexBinary(&$field, pstate); + |$indent1$indent2 if (pstate->error) return;""".stripMargin val unparseStatement = - s""" unparse_$function($field.lengthInBytes, $intLen, ustate); - | if (ustate->error) return; - | unparse_hexBinary($field, ustate); - | if (ustate->error) return;""".stripMargin - cgState.addSimpleTypeStatements(initERDStatement, initSelfStatement, parseStatement, unparseStatement) + s"""$indent1$indent2 unparse_$function($field.lengthInBytes, $intLen, ustate); + |$indent1$indent2 if (ustate->error) return; + |$indent1$indent2 unparse_hexBinary($field, ustate); + |$indent1$indent2 if (ustate->error) return;""".stripMargin + cgState.addSimpleTypeStatements(initERDStatement, parseStatement, unparseStatement) } // Generate C code to initialize, parse, and unparse a hexBinary specified length element private def hexBinarySpecifiedLengthAddField(e: ElementBase, deref: String, cgState: CodeGeneratorState): Unit = { + val indent1 = if (cgState.hasChoice) " " else "" + val indent2 = if (deref.nonEmpty) " " else "" val localName = e.namedQName.local val field = s"instance->$localName$deref" val fieldArray = s"instance->_a_$localName$deref" val specifiedLength = e.elementLengthInBitsEv.constValue.get val initERDStatement = if (specifiedLength > 0) - s""" $field.array = $fieldArray; - | $field.lengthInBytes = sizeof($fieldArray); - | $field.dynamic = false;""".stripMargin + s"""$indent1$indent2 $field.array = $fieldArray; + |$indent1$indent2 $field.lengthInBytes = sizeof($fieldArray); + |$indent1$indent2 $field.dynamic = false;""".stripMargin else - s""" $field.array = NULL; - | $field.lengthInBytes = 0; - | $field.dynamic = false;""".stripMargin - val initSelfStatement = if (specifiedLength > 0) - s" memset($fieldArray, 0x77, sizeof($fieldArray));" - else - "" + s"""$indent1$indent2 $field.array = NULL; + |$indent1$indent2 $field.lengthInBytes = 0; + |$indent1$indent2 $field.dynamic = false;""".stripMargin val parseStatement = - s""" parse_hexBinary(&$field, pstate); - | if (pstate->error) return;""".stripMargin + s"""$indent1$indent2 parse_hexBinary(&$field, pstate); + |$indent1$indent2 if (pstate->error) return;""".stripMargin val unparseStatement = - s""" unparse_hexBinary($field, ustate); - | if (ustate->error) return;""".stripMargin - cgState.addSimpleTypeStatements(initERDStatement, initSelfStatement, parseStatement, unparseStatement) + s"""$indent1$indent2 unparse_hexBinary($field, ustate); + |$indent1$indent2 if (ustate->error) return;""".stripMargin + cgState.addSimpleTypeStatements(initERDStatement, parseStatement, unparseStatement) } // Generate C code to validate a hexBinary element against its fixed value private def hexBinaryValidateFixed(e: ElementBase, deref: String, cgState: CodeGeneratorState): Unit = { + val indent1 = if (cgState.hasChoice) " " else "" + val indent2 = if (deref.nonEmpty) " " else "" val localName = e.namedQName.local val field = s"instance->$localName$deref" - val i = if (deref.length > 2) deref.substring(1, deref.length - 1) else "" - val fixed = s"${localName}_fixed$i" + val fixed = s"${localName}_fixed" val array = e.fixedValueAsString.grouped(2).mkString("0x", ", 0x", "") val initERDStatement = "" - val initSelfStatement = "" val parseStatement = - s""" uint8_t $fixed[] = {$array}; - | parse_validate_fixed(memcmp($field.array, $fixed, sizeof($fixed)) == 0, "$localName", pstate); - | if (pstate->error) return;""".stripMargin + s"""$indent1$indent2 uint8_t $fixed[] = {$array}; + |$indent1$indent2 parse_validate_fixed(memcmp($field.array, $fixed, sizeof($fixed)) == 0, "$localName", pstate); + |$indent1$indent2 if (pstate->error) return;""".stripMargin val unparseStatement = - s""" uint8_t $fixed[] = {$array}; - | unparse_validate_fixed(memcmp($field.array, $fixed, sizeof($fixed)) == 0, "$localName", ustate); - | if (ustate->error) return;""".stripMargin - cgState.addSimpleTypeStatements(initERDStatement, initSelfStatement, parseStatement, unparseStatement) + s"""$indent1$indent2 uint8_t $fixed[] = {$array}; + |$indent1$indent2 unparse_validate_fixed(memcmp($field.array, $fixed, sizeof($fixed)) == 0, "$localName", ustate); + |$indent1$indent2 if (ustate->error) return;""".stripMargin Review Comment: I haven't implemented stripMargin(indent) yet since it doesn't have to be done right now. ########## daffodil-runtime2/src/main/scala/org/apache/daffodil/runtime2/generators/CodeGeneratorState.scala: ########## @@ -58,38 +56,209 @@ class CodeGeneratorState(private val root: ElementBase) { sb } + // Returns a name for the given element's element resource descriptor + private def erdName(context: ElementBase): String = { + val sb = buildName(context, new StringBuilder) ++= "ERD" + val name = sb.toString + name + } + // Returns a name for the given element's C struct identifier private def cStructName(context: ElementBase): String = { val sb = buildName(context, new StringBuilder) val name = sb.toString name } - // Returns a name for the given element's element resource descriptor - private def erdName(context: ElementBase): String = { - val sb = buildName(context, new StringBuilder) ++= "ERD" - val name = sb.toString - name + // Returns the notation needed to access a C struct field. We make some simplifying + // assumptions to make generating the field access easier: + // - the expression contains only a relative or absolute path, nothing else (e.g., + // the expression doesn't call any functions or perform any computation) + // - we can convert an absolute path to a rootElement()-> indirection + // - we can convert a relative path beginning with up dirs to a parents-> indirection + // - we can convert a relative path without any up dirs to an instance-> indirection + // - we can convert slashes in the path to dots in a C struct field access notation + private def cStructFieldAccess(expr: String): String = { + // Strip any namespace prefixes from expr and the root element's local name since + // C code uses only struct fields' names. + val rootName = root.namedQName.local + val exprPath = expr.replaceAll("/[^/:]+:", "/").stripPrefix(s"/$rootName") + val fieldAccess = if (exprPath.startsWith("/")) { + // Convert exprPath to a rootElement()-> indirection + val C = cStructName(root) + s"""(($C *)rootElement())->${exprPath.stripPrefix("/")}""" + } else if (exprPath.startsWith("../")) { + // Split exprPath into the up dirs and after the up dirs + val afterUpDirs = exprPath.split("\\.\\./").mkString + val upDirs = exprPath.stripSuffix(afterUpDirs) + // Count how many up dirs there are + val nUpDirs = upDirs.split('/').length + // Go up the stack that many times to get that struct's C type + val C = structs(nUpDirs).C + // Convert the up dirs to parents + val parents = upDirs.replaceAllLiterally("../", "parent->").stripSuffix("->") + // Convert exprPath to a parents-> indirection + s"""(($C *)instance->_base.$parents)->$afterUpDirs""" + } else { + // Convert exprPath to an instance-> indirection + s"""instance->$exprPath""" + } + // Finally, convert the field access to C struct dot notation + val notation = fieldAccess.replace('/', '.') + notation } Review Comment: Yes, I would be happy to replace this part of runtime2 with a walker for expressions in a subsequent PR. Furthermore, we also need a walker for grammar objects that both the runtime1 and the C code generator backend can use to convert grammar objects to runtime1 objects and generated C code, respectively. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
