mbeckerle closed pull request #34: Daffodil 1884 bit order
URL: https://github.com/apache/incubator-daffodil/pull/34
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/daffodil-io/src/main/scala/edu/illinois/ncsa/daffodil/io/DataOutputStreamImplMixin.scala
b/daffodil-io/src/main/scala/edu/illinois/ncsa/daffodil/io/DataOutputStreamImplMixin.scala
index 44784cbf1..33f9c3eee 100644
---
a/daffodil-io/src/main/scala/edu/illinois/ncsa/daffodil/io/DataOutputStreamImplMixin.scala
+++
b/daffodil-io/src/main/scala/edu/illinois/ncsa/daffodil/io/DataOutputStreamImplMixin.scala
@@ -247,15 +247,6 @@ trait DataOutputStreamImplMixin extends
DataStreamCommonState
private var fragmentLastByteLimit_ : Int = 0
def fragmentLastByteLimit = fragmentLastByteLimit_
- private var maybeFragmentBitOrder: Maybe[BitOrder] = Nope
- def fragmentBitOrder = {
- Assert.usage(maybeFragmentBitOrder.isDefined)
- this.maybeFragmentBitOrder.value
- }
- def setFragmentBitOrder(bo: BitOrder) {
- maybeFragmentBitOrder = One(bo)
- }
-
def setFragmentLastByte(newFragmentByte: Int, nBitsInUse: Int) {
Assert.usage(nBitsInUse >= 0 && nBitsInUse <= 7)
Assert.usage(newFragmentByte >= 0 && newFragmentByte <= 255) // no bits
above first byte are in use.
diff --git
a/daffodil-io/src/main/scala/edu/illinois/ncsa/daffodil/io/DirectOrBufferedDataOutputStream.scala
b/daffodil-io/src/main/scala/edu/illinois/ncsa/daffodil/io/DirectOrBufferedDataOutputStream.scala
index 7a9cfd27d..9ebbb51d0 100644
---
a/daffodil-io/src/main/scala/edu/illinois/ncsa/daffodil/io/DirectOrBufferedDataOutputStream.scala
+++
b/daffodil-io/src/main/scala/edu/illinois/ncsa/daffodil/io/DirectOrBufferedDataOutputStream.scala
@@ -179,7 +179,7 @@ final class DirectOrBufferedDataOutputStream private[io]
(var splitFrom: DirectO
*/
private var _javaOutputStream: java.io.OutputStream = bufferingJOS
- private[io] final def isBuffering: Boolean = {
+ final def isBuffering: Boolean = {
val res = getJavaOutputStream() _eq_ bufferingJOS
res
}
diff --git
a/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/ProcessorStateBases.scala
b/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/ProcessorStateBases.scala
index d46485215..e69f8b201 100644
---
a/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/ProcessorStateBases.scala
+++
b/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/ProcessorStateBases.scala
@@ -71,9 +71,6 @@ import
edu.illinois.ncsa.daffodil.schema.annotation.props.gen.ByteOrder
import edu.illinois.ncsa.daffodil.processors.charset.BitsCharsetDecoder
import edu.illinois.ncsa.daffodil.processors.charset.BitsCharsetEncoder
import edu.illinois.ncsa.daffodil.processors.unparsers.UState
-import edu.illinois.ncsa.daffodil.processors.parsers.ParserBitOrderChecks
-import edu.illinois.ncsa.daffodil.processors.parsers.PState
-import edu.illinois.ncsa.daffodil.processors.unparsers.UnparserBitOrderChecks
/**
* Trait mixed into the PState.Mark object class and the ParseOrUnparseState
@@ -200,6 +197,23 @@ abstract class ParseOrUnparseState protected (
private def runtimeData = processor.context
private def termRuntimeData = runtimeData.asInstanceOf[TermRuntimeData]
+ /**
+ * Checks if the bit order change is legal.
+ *
+ * For parsing we know the bitPos, so we can determine if we're at a byte
boundary.
+ *
+ * For unparsing we may not know the absolute bitPos, so we cannot
necessarily
+ * determine if the boundary is legal or not.
+ *
+ * If we know the absoluteBitPos we do the check (as for parsing).
+ *
+ * If we do not know the absoluteBitPos, then in that case, we split the
+ * DataOutputStream into original and buffered. The "check" then occurs
+ * when these DataOutputStreams are collapsed back together.
+ *
+ * So this "check" call, can have an important side effect when unparsing
that
+ * queues up the check to be done in the future.
+ */
protected def checkBitOrder(): Unit
/**
@@ -306,7 +320,7 @@ abstract class ParseOrUnparseState protected (
if (this.processor.isPrimitive)
if (decoderCacheEntry_.encodingMandatoryAlignmentInBitsArg != 1)
if (this.bitPos1b % 8 != 1)
- ParserBitOrderChecks.checkParseBitOrder(this.asInstanceOf[PState])
+ checkBitOrder()
}
decoderCacheEntry_
}
@@ -318,7 +332,7 @@ abstract class ParseOrUnparseState protected (
if (this.processor.isPrimitive)
if (encoderCacheEntry_.encodingMandatoryAlignmentInBitsArg != 1)
if (this.bitPos1b % 8 != 1)
-
UnparserBitOrderChecks.checkUnparseBitOrder(this.asInstanceOf[UState])
+ checkBitOrder()
}
encoderCacheEntry_
}
diff --git
a/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/Suspension.scala
b/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/Suspension.scala
index a839de085..125823657 100644
---
a/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/Suspension.scala
+++
b/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/Suspension.scala
@@ -167,6 +167,7 @@ trait Suspension
val mkl = maybeKnownLengthInBits.getULong
buffered.setAbsStartingBitPos0b(originalAbsBitPos0b + mkl)
+ buffered.setPriorBitOrder(ustate.bitOrder)
}
} else {
diff --git
a/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/parsers/PState.scala
b/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/parsers/PState.scala
index 9c625bf71..1a455d1fd 100644
---
a/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/parsers/PState.scala
+++
b/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/parsers/PState.scala
@@ -284,8 +284,53 @@ final class PState private (
dataInputStream.setDebugging(flag)
}
+ /**
+ * Checks for bit order change. If the bit order is changing, checks if we're
+ * on a proper byte boundary.
+ */
final override protected def checkBitOrder(): Unit = {
- ParserBitOrderChecks.checkParseBitOrder(this)
+
+ //
+ // TODO: This looks like a lot of overhead for every single parse call.
+ //
+ // We need to check for bitOrder change. If it is changing, we
+ // need to know if it is on a proper byte boundary.
+ //
+ val dis = this.dataInputStream
+ val isChanging = isParseBitOrderChanging(dis)
+ if (isChanging) {
+ //
+ // the bit order is changing. Let's be sure
+ // that it's legal to do so w.r.t. other properties
+ // These checks will have been evaluated at compile time if
+ // all the properties are static, so this is really just
+ // in case the charset or byteOrder are runtime-valued.
+ //
+ this.processor.context match {
+ case trd: TermRuntimeData => {
+ val mcboc = trd.maybeCheckBitOrderAndCharsetEv
+ val mcbbo = trd.maybeCheckByteAndBitOrderEv
+ if (mcboc.isDefined) mcboc.get.evaluate(this) // Expressions must be
evaluated on the element, not before it is created.
+ if (mcbbo.isDefined) mcbbo.get.evaluate(this)
+ }
+ case _ => // ok
+ }
+
+ dis.st.setPriorBitOrder(this.bitOrder)
+ if (!dis.isAligned(8))
+ SDE("Can only change dfdl:bitOrder on a byte boundary. Bit pos (1b)
was %s.", dis.bitPos1b)
+ }
+ }
+
+ private def isParseBitOrderChanging(dis: ByteBufferDataInputStream): Boolean
= {
+ this.processor.context match {
+ case ntrd: NonTermRuntimeData => false
+ case _ => {
+ val priorBitOrder = dis.st.priorBitOrder
+ val newBitOrder = this.bitOrder
+ priorBitOrder ne newBitOrder
+ }
+ }
}
}
@@ -426,53 +471,3 @@ object PState {
createInitialPState(root, dis, output, dataProc)
}
}
-
-object ParserBitOrderChecks {
- /**
- * Checks for bit order change. If the bit order is changing, checks if we're
- * on a proper byte boundary.
- */
- final def checkParseBitOrder(pstate: PState) = {
- //
- // TODO: This looks like a lot of overhead for every single parse call.
- //
- // We need to check for bitOrder change. If it is changing, we
- // need to know if it is on a proper byte boundary.
- //
- val dis = pstate.dataInputStream
- val isChanging = isParseBitOrderChanging(dis, pstate)
- if (isChanging) {
- //
- // the bit order is changing. Let's be sure
- // that it's legal to do so w.r.t. other properties
- // These checks will have been evaluated at compile time if
- // all the properties are static, so this is really just
- // in case the charset or byteOrder are runtime-valued.
- //
- pstate.processor.context match {
- case trd: TermRuntimeData => {
- val mcboc = trd.maybeCheckBitOrderAndCharsetEv
- val mcbbo = trd.maybeCheckByteAndBitOrderEv
- if (mcboc.isDefined) mcboc.get.evaluate(pstate) // Expressions must
be evaluated on the element, not before it is created.
- if (mcbbo.isDefined) mcbbo.get.evaluate(pstate)
- }
- case _ => // ok
- }
-
- dis.st.setPriorBitOrder(pstate.bitOrder)
- if (!dis.isAligned(8))
- pstate.SDE("Can only change dfdl:bitOrder on a byte boundary. Bit pos
(1b) was %s.", dis.bitPos1b)
- }
- }
-
- private def isParseBitOrderChanging(dis: ByteBufferDataInputStream, pstate:
PState): Boolean = {
- pstate.processor.context match {
- case ntrd: NonTermRuntimeData => false
- case _ => {
- val priorBitOrder = dis.st.priorBitOrder
- val newBitOrder = pstate.bitOrder
- priorBitOrder ne newBitOrder
- }
- }
- }
-}
diff --git
a/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/unparsers/UState.scala
b/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/unparsers/UState.scala
index c4222617a..e8bde2e17 100644
---
a/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/unparsers/UState.scala
+++
b/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/unparsers/UState.scala
@@ -183,8 +183,152 @@ abstract class UState(
_processorStatus = new Failure(ue)
}
+ /**
+ * Checks for legal bitOrder change (byte boundary required), or splits the
+ * DOS so that the check will occur later when they are collapsed back
together.
+ *
+ * If you think about it, the only way we could not have the absoluteBitPos
is
+ * because something variable-length preceded us, and couldn't be computed
due
+ * to suspended computation (forward referencing expression somewhere prior).
+ *
+ * At some point, that suspension will get resolved, and forward collapsing
of
+ * the DataOutputStreams will occur. When it encounters a split created here,
+ * we already know that the bit orders are different (or we wouldn't have
put in
+ * the split), so we just have to see if we're on a byte boundary. That
could happen
+ * if the original DOS ended in a frag byte, but previous to it, was
something
+ * that was variable bits wide (all bits shift such that original DOS's frag
byte
+ * becomes a whole byte.)
+ *
+ * The invariant here is that the original DOS will get collapsed together
with
+ * DOS preceding it. After that collapsing, it has to end at a byte boundary
(no
+ * frag byte). If it doesn't then it's a bit order-change error. Otherwise
+ * we're ok.
+ *
+ * This is why we can always proceed with a new buffered DOS, knowing we're
+ * going to be on a byte boundary with the bit order needed.
+ */
final override protected def checkBitOrder(): Unit = {
- UnparserBitOrderChecks.checkUnparseBitOrder(this)
+ //
+ // Check for bitOrder change. If yes, then unless we know we're byte
aligned
+ // we must split the DOS until we find out. That way the new buffered DOS
+ // can be assumed to be byte aligned (which will be checked on combining),
+ // and the bytes in it will actually start out byte aligned.
+ //
+ val dos = this.dataOutputStream
+ val isChanging = isUnparseBitOrderChanging(dos)
+ if (isChanging) {
+ //
+ // the bit order is changing. Let's be sure
+ // that it's legal to do so w.r.t. other properties
+ // These checks will have been evaluated at compile time if
+ // all the properties are static, so this is really just
+ // in case the charset or byteOrder are runtime-valued.
+ //
+ this.processor.context match {
+ case trd: TermRuntimeData => {
+ val mcboc = trd.maybeCheckBitOrderAndCharsetEv
+ val mcbbo = trd.maybeCheckByteAndBitOrderEv
+ if (mcboc.isDefined) mcboc.get.evaluate(this)
+ if (mcbbo.isDefined) mcbbo.get.evaluate(this)
+ }
+ case _ => // ok
+ }
+
+ // TODO: Figure out why this setPriorBitOrder is needed here.
+ // If we remove it, then test_ep2 (an envelope-payload test with
+ // bigEndian MSBF envelope and littleEndian LSBF payload)
+ // fails with Assert.invariant(isWritable)
+ // when writing a long. The buffered DOS it is writing to is finished.
+ //
+ // It's unclear why setting the prior bit order here affects whether
+ // a DOS is active or finished elsewhere, but it does.
+ //
+ val bo = this.bitOrder // will NOT recurse back to here. It *will* hit
cache.
+ dos.setPriorBitOrder(bo)
+
+ // If we can't check right now because we don't have absolute bit
position
+ // then split the DOS so it gets checked later.
+ //
+ splitOnUknownByteAlignmentBitOrderChange(dos)
+ }
+ }
+
+ private def isUnparseBitOrderChanging(dos:
DirectOrBufferedDataOutputStream): Boolean = {
+ val ctxt = this.processor.context
+ ctxt match {
+ case ntrd: NonTermRuntimeData => false
+ case _ => {
+ val priorBitOrder = dos.priorBitOrder
+ val newBitOrder = this.bitOrder
+ priorBitOrder ne newBitOrder
+ }
+ }
+ }
+
+ /**
+ * If necessary, split DOS so bitOrder proper byte boundary is checked
later.
+ *
+ * If we can't check because of unknown absolute bit position,
+ * then we split the DOS, start a new buffering one (assumed to be
+ * byte aligned, with the new bitOrder).
+ *
+ * The bit order would not be unknown except that something of
+ * variable length precedes us and is suspended.
+ * When that eventually is resolved, then the DOS will collapse forward
+ * and the boundary between the original (dos here), and the buffered
+ * one will be checked as part of the collapsing logic.
+ *
+ * That is, this split does NOT queue a suspension object, it
+ * Just inserts a split in the DOS. This gets put together later when
+ * the DOS are collapsed together, and the check for byte boundary occurs
+ * at that time.
+ */
+ private def splitOnUknownByteAlignmentBitOrderChange(dos:
DirectOrBufferedDataOutputStream): Unit = {
+ val mabp = dos.maybeAbsBitPos0b
+ val mabpDefined = mabp.isDefined
+ val isSplitNeeded: Boolean = {
+ if (mabpDefined && dos.isAligned(8)) {
+ //
+ // Not only do we have to be logically aligned, we also have
+ // to be physically aligned in the buffered stream, otherwise we
+ // cannot switch bit orders, and we have to split off a new
+ // stream to start the accumulation of the new bit-order material.
+ //
+ // fragmentLastByteLimit == 0 means there is no fragment byte,
+ // which only happens if we're on a byte boundary in the
implementation.
+ //
+ if (dos.fragmentLastByteLimit == 0) false
+ else true
+ } else if (!mabpDefined) true
+ else {
+ // mabp is defined, and we're not on a byte boundary
+ // and the bit order is changing.
+ // Error: bit order change on non-byte boundary
+ val bp1b = mabp.get + 1
+ SDE("Can only change dfdl:bitOrder on a byte boundary. Bit pos (1b)
was %s. Should be 1 mod 8, was %s (mod 8)", bp1b, bp1b % 8)
+ }
+ }
+ if (isSplitNeeded) {
+ Assert.invariant(dos.isBuffering) // Direct DOS always has absolute
position, so has to be buffering.
+ val newDOS = dos.addBuffered
+ dataOutputStream = newDOS
+ //
+ // Just splitting to start a new bitOrder on a byte boundary in a new
+ // buffered DOS
+ // So the prior DOS can be finished. Nothing else will be added to it.
+ //
+ // Note: unlike a suspension, in this case, we're not going to write
anything
+ // more to the end of that DOS. A bitOrder change occurs before we get to
+ // any such content being unparsed, or suspended. So after a bitOrder
change,
+ // the unparsing occurs, possibly buffered, and works as if the
+ // bitOrder change was legal and happened, even though we cannot know yet
+ // if that is the case, and it will get checked later.
+ //
+ // Finished means you won't add data to the end of it any more.
+ // It does NOT prevent information like the absoluteBitPos to
+ // propagate.
+ dos.setFinished(this)
+ }
}
}
@@ -319,7 +463,7 @@ final class UStateMain private (
suspendedDOS,
variableBox,
currentInfosetNodeStack.top.get, // only need the to of the stack, not
the whole thing
- arrayIndexStack.top, // only need the to of the stack, not the whole
thing
+ arrayIndexStack.top, // only need the top of the stack, not the whole
thing
es,
ds,
prior,
@@ -494,86 +638,3 @@ object UState {
}
}
-object UnparserBitOrderChecks {
-
- final def checkUnparseBitOrder(ustate: UState) = {
- //
- // Check for bitOrder change. If yes, then unless we know we're byte
aligned
- // we must split the DOS until we find out. That way the new buffered DOS
- // can be assumed to be byte aligned (which will be checked on combining),
- // and the bytes in it will actually start out byte aligned.
- //
- val dos = ustate.dataOutputStream
- val isChanging = isUnparseBitOrderChanging(dos, ustate)
- if (isChanging) {
- //
- // the bit order is changing. Let's be sure
- // that it's legal to do so w.r.t. other properties
- // These checks will have been evaluated at compile time if
- // all the properties are static, so this is really just
- // in case the charset or byteOrder are runtime-valued.
- //
- ustate.processor.context match {
- case trd: TermRuntimeData => {
- val mcboc = trd.maybeCheckBitOrderAndCharsetEv
- val mcbbo = trd.maybeCheckByteAndBitOrderEv
- if (mcboc.isDefined) mcboc.get.evaluate(ustate)
- if (mcbbo.isDefined) mcbbo.get.evaluate(ustate)
- }
- case _ => // ok
- }
-
- dos.setPriorBitOrder(ustate.bitOrder)
- splitOnUnalignedBitOrderChange(dos, ustate)
- }
- }
-
- private def isUnparseBitOrderChanging(dos: DirectOrBufferedDataOutputStream,
ustate: UState): Boolean = {
- val ctxt = ustate.processor.context
- ctxt match {
- case ntrd: NonTermRuntimeData => false
- case _ => {
- val priorBitOrder = dos.priorBitOrder
- val newBitOrder = ustate.bitOrder
- priorBitOrder ne newBitOrder
- }
- }
- }
-
- private def splitOnUnalignedBitOrderChange(dos:
DirectOrBufferedDataOutputStream, ustate: UState): Unit = {
- val mabp = dos.maybeAbsBitPos0b
- val mabpDefined = mabp.isDefined
- val isSplitNeeded: Boolean = {
- if (mabpDefined && dos.isAligned(8)) {
- //
- // Not only do we have to be logically aligned, we also have
- // to be physically aligned in the buffered stream, otherwise we
- // cannot switch bit orders, and we have to split off a new
- // stream to start the accumulation of the new bit-order material.
- //
- // fragmentLastByteLimit == 0 means there is no fragment byte,
- // which only happens if we're on a byte boundary in the
implementation.
- //
- if (dos.fragmentLastByteLimit == 0) false
- else true
- } else if (!mabpDefined) true
- else {
- // mabp is defined, and we're not on a byte boundary
- // and the bit order is changing.
- // Error: bit order change on non-byte boundary
- val bp1b = mabp.get + 1
- ustate.SDE("Can only change dfdl:bitOrder on a byte boundary. Bit pos
(1b) was %s.", bp1b)
- }
- }
- if (isSplitNeeded) {
- val newDOS = dos.addBuffered
- ustate.dataOutputStream = newDOS
- //
- // Just splitting to start a new bitOrder on a byte boundary in a new
- // buffered DOS
- // So the prior DOS can be finished. Nothing else will be added to it.
- //
- dos.setFinished(ustate)
- }
- }
-}
diff --git
a/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/unparsers/Unparser.scala
b/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/unparsers/Unparser.scala
index 0f43eb221..63be8445e 100644
---
a/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/unparsers/Unparser.scala
+++
b/daffodil-runtime1/src/main/scala/edu/illinois/ncsa/daffodil/processors/unparsers/Unparser.scala
@@ -64,6 +64,24 @@ sealed trait Unparser
if (ustate.dataProc.isDefined) ustate.dataProc.get.before(ustate, this)
try {
unparse(ustate)
+
+ // TODO: Remove this call to ustate.bitOrder below.
+ // Figure out where this is needed elsewhere in unparser code.
+ //
+ // Clearly calling this here is overkilling the problem.
+ //
+ // In theory some places in the unparser code dealing with
splitting/suspending
+ // or outputValueCalc elemetns are missing proper checking of bitOrder,
or
+ // keeping track of prior bit order. Finding those has been problematic.
+ //
+ // So this is a temporary fix, until we can figure out where else to do
this.
+ //
+ this.context match {
+ case trd: TermRuntimeData =>
+ ustate.bitOrder // asking for bitOrder checks bit order changes.
+ // this splits DOS on bitOrder changes if absoluteBitPos not known
+ case _ => //ok
+ }
} finally {
ustate.resetFormatInfoCaches()
}
@@ -116,6 +134,8 @@ trait SuspendableUnparser
protected def suspendableOperation: SuspendableOperation
override final def unparse(state: UState): Unit = {
+ state.bitOrder // force checking of bitOrder changes on non-byte boundaries
+ // also forces capture of bitOrder value in case of suspension.
suspendableOperation.run(state)
}
}
diff --git
a/daffodil-test/src/test/scala-new/edu/illinois/ncsa/daffodil/section00/general/TestUnparserGeneral2.scala
b/daffodil-test/src/test/scala/edu/illinois/ncsa/daffodil/section00/general/TestUnparserGeneral2.scala
similarity index 100%
rename from
daffodil-test/src/test/scala-new/edu/illinois/ncsa/daffodil/section00/general/TestUnparserGeneral2.scala
rename to
daffodil-test/src/test/scala/edu/illinois/ncsa/daffodil/section00/general/TestUnparserGeneral2.scala
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
