You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@daffodil.apache.org by Mike Beckerle <mb...@tresys.com> on 2018/02/05 16:08:20 UTC
Re: bitOrder Regression - DAFFODIL-1884
Taking this discussion to the dev@daffodil list.
So whenever there is a suspension, we do create a new buffered data output stream, and a UState (actually a sub-class called UStateForSuspension, there are two subclasses of abstract UState, the other is UStateMain) that provides the additional things needed to say, compute expressions, should they be needed to complete the unparsing later. The UStateForSuspension object also caches the bitOrder, encoders/decoders, etc. - but those caches should be empty given that it is a new object.
But that's not sufficient.
Consider that we are unparsing LSBF data, and we end in the middle of a byte. So there is a fragment of a byte.
Then next suppose we're on an element or model group (a Term generally) that has alignment 1 byte, and bit order MSBF.
So we go to move over to the byte boundary, filling the unused bits with fill byte.
Ooops. which bits. Not the MSBF bits the current term would tell us is the proper bit order. It should be the LSBF bits where the byte was left off.
So the bit order of the fragment byte must be tracked.
Looks like it is. There's a maybeFragmentBitOrder var, and a setter setFragmentBitOrder on the DirectOrBufferedDataOutputStream class.
But, it is never called. Oops. Looks like I put the mechanism in place to do this, but didn't finish the implementation.
...mikeb
________________________________
From: Stephen Lawrence
Sent: Monday, February 5, 2018 10:29 AM
To: Mike Beckerle
Subject: Re: bitOrder Regression
Ok. Let's talk/email through this and make sure we fully understand the
problem/what the solution would entail before decided if the fix can be
punted or not.
This just popped into my head: Perhaps the solution is everytime there
is a suspension, we just need to create a new buffered DOS. This is
because that suspension could potentially change the bitorder. This
buffered DOS could then have a prior bitOrder of "unknown", or has a
flag about what its initial bit order is, or something along those
lines. We write to that DOS assuming the prior bit order is acceptable
(i.e. the same as the first thing written to the buffered DOS).
Then anytime we collapse a buffered DOS into the direct DOS, we just
need to ensure that that either 1) the direct DOS ends on a byte
boundary in which case we don't care about the bit order of the buffered
DOS or 2) the final bitOrder of the direct DOS must match the initial
bitOrder of the buffered DOS.
I feel like we already have that logic or somethign close, so maybe the
current logic is just slightly off.
- Steve
On 02/05/2018 10:20 AM, Mike Beckerle wrote:
> I did not make any progress. I spend enough time to determine I couldn't easily
> fix it.
>
>
> The problem originated in Link16 because it uses bigEndian MSBF envelopes (NACT)
> surrounding little endian LSBF data (Link16), however, that is working
> currently. The bug fix is perhaps something we can live without for now. VMF
> doesn't run into this because the envelopes (MIL-STD-2045) are also little
> endian LSBF.
>
>
> Fixing it, I'd really like to spend some time talking through the design issues
> with you because it is subtle. it's not just a matter of finding the one spot we
> forgot to put something in. (Well, perhaps it is, but I'd like to capture the
> design intent and articulate it in comments in the code.)
>
>
> At this point I am not sure we are keeping track of bitOrder in the right way in
> the unparser. We depend on all unparsers running so that the underlying bit
> stream is either marked with one bitOrder or the other because the boundary is
> known, or it is split at the transition boundary, and checked later once the
> actual boundary position is known. There are situations where we're not
> recording the bit order, due to IVC elements, and perhaps due to other things.
>
> --------------------------------------------------------------------------------
> *From:* Stephen Lawrence
> *Sent:* Monday, February 5, 2018 8:51:36 AM
> *To:* Mike Beckerle
> *Subject:* Re: bitOrder Regression
> Did you make any progress with this regression? I assume this test came
> from VMF so I suspect we want it fixed for 2.1.0, but maybe it can be
> punted to the next release? I'm not sure of its severity and what this
> bug affects.
>
> - Steve
>
>
> On 01/31/2018 01:14 PM, Steve Lawrence wrote:
>> I think there might be another bug somewhere. I added a hack so that
>> when a suspension was created it would check the bitOrder and cause a
>> new buffered DOS if it changed (basically, I just call state.bitOrder at
>> the end of the suspension code and that handles all the logic).
>>
>> However, with this hack, I found that when the test switches back to BE
>> MSBF for the last element, the bitOrder is never checked. This means
>> this element ends up written to a buffered DOS that had LE LSBF and
>> things get go off the rails. I confirmed this but adding a
>> state.bitOrder in the unparse() method to ensure that bitOrder is
>> checked and that fixed it. However, I don't think this is the right
>> place for it. And places that feel right to me end up with invariants.
>> Not sure the right place for this logic.
>>
>> So I think there are two issues here:
>>
>> 1) Suspensions aren't setting/checking prior bit order
>> 2) Bit order isn't checked when switching from LE LSBF to BE MSBF
>>
>> You're more familiar with this code so probably have a better idea of
>> the right fix.
>>
>> - Steve
>>
>> On 01/31/2018 08:14 AM, Mike Beckerle wrote:
>>> My quick tests last night concur.
>>>
>>> We need to insure that priorBitOrder is set and propagated correctly in the
>>> unparser even when suspensions are created.
>>>
>>>
>>> -------- Original message --------
>>> From: Stephen Lawrence <sl...@tresys.com>
>>> Date: 1/30/18 9:10 PM (GMT-05:00)
>>> To: Mike Beckerle <mb...@tresys.com>
>>> Subject: Re: bitOrder Regression
>>>
>>> Had some time, took at alookt at the test a little bit. Here's what I
>>> think is going on:
>>>
>>> There is an element "a" that is BE MSBF. The bitOrder is correctly set
>>> to MSBF when this is unparsed.
>>>
>>> The following siblings of this element are LE LSBF. However, the first
>>> couple of siblings are OVC. The OVC must be suspended, but ends up
>>> moving the bit position even though nothing is written. Since nothing is
>>> written, bitOrder is never accessed and priorBitOrder is never set. So
>>> it is still MSBF.
>>>
>>> Eventually we get to a sibling "padBit" that doesn't need to suspend. It
>>> tries to write a padding bit at bitPosition 28 (1b). The priorBitOrder
>>> is still MSBF but padBit is LSBF, so it complains about changing
>>> bitOrrder on a non-byte boundary.
>>>
>>> The issue here is that had the first sibling not suspended it would have
>>> changed the bitOrder to LSBF on a byte boundary and by the time we go to
>>> "padBit" it's not on a byte boundary, but it wouldn't be changing the
>>> bitOrder.
>>>
>>> So I think the solution is somehow related to having suspensions ensure
>>> that the priorBitOrder is set correctly, even though they haven't
>>> written anything yet. I'm not sure exactly how to do this, but this
>>> should help narrow things down a bit.
>>>
>>> - Steve
>>>
>>> On 01/30/2018 06:36 PM, Mike Beckerle wrote:
>>>> Grrg. Probably a regression. I will take a look Wednesday afternoon.
>>>>
>>>>
>>>> -------- Original message --------
>>>> From: Stephen Lawrence <sl...@tresys.com>
>>>> Date: 1/30/18 5:13 PM (GMT-05:00)
>>>> To: Mike Beckerle <mb...@tresys.com>
>>>> Subject: bitOrder Regression
>>>>
>>>> Mike,
>>>>
>>>> I rebased the Apache changes and noticed there was a test still in
>>>> scala-new, which are currently ignored. The test now fails when I move
>>>> it to scala, I suspect due to some of the recent bit order changes. Not
>>>> sure if this is a known issue and needs to be moved to scala-debug or if
>>>> it's a regression. The test is:
>>>>
>>>> edu.illinois.ncsa.daffodil.section00.general.TestUnparserGeneral2.test_bitOrderOVC1
>>>>
>>>> - Steve
>>>>
>>>
>>
>
Re: bitOrder Regression - DAFFODIL-1884
Posted by Mike Beckerle <mb...@tresys.com>.
From code inspection it seems the data output stream code always uses finfo.bitOrder, and never uses the prior bit order, so that's one bug. Then there's the maintaining of priorBitOrder setting, which it also isn't doing.
So basically, the priorBitOrder fix, which was put in for parsing, wasn't finished for unparsing.
I know exactly why this happened. It's because on a rebase when the tests in scala-new were no longer being run, this test slipped through the cracks as I rebased onto that change, but my copy of this tests stayed in scala-new, but the new code, well nothing was running it anymore, so it falsely looked to be fixed. So I simply never finished the priorBitOrder change in the unparser code.
________________________________
From: Mike Beckerle
Sent: Monday, February 5, 2018 11:48:10 AM
To: Steve Lawrence; dev@daffodil.apache.org
Subject: Re: bitOrder Regression - DAFFODIL-1884
My error. The priorBitOrder is not on the P/UState. It's on the DataStreamCommonState where it is part of both data input and output stream implementations.
Makes me think the fragment bitOrder is clearly just a duplicate mechanism, as it appears on DataOutputStreamImplMixin, which extends DataStreamCommonState.
________________________________
From: Steve Lawrence <sl...@apache.org>
Sent: Monday, February 5, 2018 11:16:38 AM
To: Mike Beckerle; dev@daffodil.apache.org
Subject: Re: bitOrder Regression - DAFFODIL-1884
Does fragmentBitOrder differ from priorBitOrder? priorBitOrder always
represents the bitOrder of the last thing that was written. Seems to me
the only difference is that fragmentBitOrder will be Nope when there
isn't a fragment, and will be the same as priorBitOrder when there is a
fragment. In which case it seems the maybeFragmentBitOrder stuff can go
away?
I imagine the align issue goes like this:
1) Write less than a byte of LSBF data. priorBitOrder = LSBF
2) We want to write some data that is MBSF, write 8 - N bits of fillByte
using priorBitOrder (LSBF)
3) Write MSBF data ensuring that it starts on a byte boundary doe to the
bit order change.
So the thing here that I think might be unique (and might solve other
alignment issues? I think there's a bug for it), is to use the
priorBitOrder for writing alignment bits rather than the
FormatInfo.bitOrder object?
On 02/05/2018 11:08 AM, Mike Beckerle wrote:
>
> Taking this discussion to the dev@daffodil list.
>
> So whenever there is a suspension, we do create a new buffered data output
> stream, and a UState (actually a sub-class called UStateForSuspension, there are
> two subclasses of abstract UState, the other is UStateMain) that provides the
> additional things needed to say, compute expressions, should they be needed to
> complete the unparsing later. The UStateForSuspension object also caches the
> bitOrder, encoders/decoders, etc. - but those caches should be empty given that
> it is a new object.
>
> But that's not sufficient.
>
> Consider that we are unparsing LSBF data, and we end in the middle of a byte. So
> there is a fragment of a byte.
> Then next suppose we're on an element or model group (a Term generally) that has
> alignment 1 byte, and bit order MSBF.
> So we go to move over to the byte boundary, filling the unused bits with fill byte.
>
> Ooops. which bits. Not the MSBF bits the current term would tell us is the
> proper bit order. It should be the LSBF bits where the byte was left off.
>
> So the bit order of the fragment byte must be tracked.
>
> Looks like it is. There's a maybeFragmentBitOrder var, and a setter
> setFragmentBitOrder on the DirectOrBufferedDataOutputStream class.
>
> But, it is never called. Oops. Looks like I put the mechanism in place to do
> this, but didn't finish the implementation.
>
> ...mikeb
>
>
>
>
>
> --------------------------------------------------------------------------------
> *From:* Stephen Lawrence
> *Sent:* Monday, February 5, 2018 10:29 AM
> *To:* Mike Beckerle
> *Subject:* Re: bitOrder Regression
> Ok. Let's talk/email through this and make sure we fully understand the
> problem/what the solution would entail before decided if the fix can be
> punted or not.
>
> This just popped into my head: Perhaps the solution is everytime there
> is a suspension, we just need to create a new buffered DOS. This is
> because that suspension could potentially change the bitorder. This
> buffered DOS could then have a prior bitOrder of "unknown", or has a
> flag about what its initial bit order is, or something along those
> lines. We write to that DOS assuming the prior bit order is acceptable
> (i.e. the same as the first thing written to the buffered DOS).
>
> Then anytime we collapse a buffered DOS into the direct DOS, we just
> need to ensure that that either 1) the direct DOS ends on a byte
> boundary in which case we don't care about the bit order of the buffered
> DOS or 2) the final bitOrder of the direct DOS must match the initial
> bitOrder of the buffered DOS.
>
> I feel like we already have that logic or somethign close, so maybe the
> current logic is just slightly off.
>
> - Steve
>
> On 02/05/2018 10:20 AM, Mike Beckerle wrote:
>> I did not make any progress. I spend enough time to determine I couldn't easily
>> fix it.
>>
>>
>> The problem originated in Link16 because it uses bigEndian MSBF envelopes (NACT)
>> surrounding little endian LSBF data (Link16), however, that is working
>> currently. The bug fix is perhaps something we can live without for now. VMF
>> doesn't run into this because the envelopes (MIL-STD-2045) are also little
>> endian LSBF.
>>
>>
>> Fixing it, I'd really like to spend some time talking through the design issues
>> with you because it is subtle. it's not just a matter of finding the one spot we
>> forgot to put something in. (Well, perhaps it is, but I'd like to capture the
>> design intent and articulate it in comments in the code.)
>>
>>
>> At this point I am not sure we are keeping track of bitOrder in the right way in
>> the unparser. We depend on all unparsers running so that the underlying bit
>> stream is either marked with one bitOrder or the other because the boundary is
>> known, or it is split at the transition boundary, and checked later once the
>> actual boundary position is known. There are situations where we're not
>> recording the bit order, due to IVC elements, and perhaps due to other things.
>>
>> --------------------------------------------------------------------------------
>> *From:* Stephen Lawrence
>> *Sent:* Monday, February 5, 2018 8:51:36 AM
>> *To:* Mike Beckerle
>> *Subject:* Re: bitOrder Regression
>> Did you make any progress with this regression? I assume this test came
>> from VMF so I suspect we want it fixed for 2.1.0, but maybe it can be
>> punted to the next release? I'm not sure of its severity and what this
>> bug affects.
>>
>> - Steve
>>
>>
>> On 01/31/2018 01:14 PM, Steve Lawrence wrote:
>>> I think there might be another bug somewhere. I added a hack so that
>>> when a suspension was created it would check the bitOrder and cause a
>>> new buffered DOS if it changed (basically, I just call state.bitOrder at
>>> the end of the suspension code and that handles all the logic).
>>>
>>> However, with this hack, I found that when the test switches back to BE
>>> MSBF for the last element, the bitOrder is never checked. This means
>>> this element ends up written to a buffered DOS that had LE LSBF and
>>> things get go off the rails. I confirmed this but adding a
>>> state.bitOrder in the unparse() method to ensure that bitOrder is
>>> checked and that fixed it. However, I don't think this is the right
>>> place for it. And places that feel right to me end up with invariants.
>>> Not sure the right place for this logic.
>>>
>>> So I think there are two issues here:
>>>
>>> 1) Suspensions aren't setting/checking prior bit order
>>> 2) Bit order isn't checked when switching from LE LSBF to BE MSBF
>>>
>>> You're more familiar with this code so probably have a better idea of
>>> the right fix.
>>>
>>> - Steve
>>>
>>> On 01/31/2018 08:14 AM, Mike Beckerle wrote:
>>>> My quick tests last night concur.
>>>>
>>>> We need to insure that priorBitOrder is set and propagated correctly in the
>>>> unparser even when suspensions are created.
>>>>
>>>>
>>>> -------- Original message --------
>>>> From: Stephen Lawrence <sl...@tresys.com>
>>>> Date: 1/30/18 9:10 PM (GMT-05:00)
>>>> To: Mike Beckerle <mb...@tresys.com>
>>>> Subject: Re: bitOrder Regression
>>>>
>>>> Had some time, took at alookt at the test a little bit. Here's what I
>>>> think is going on:
>>>>
>>>> There is an element "a" that is BE MSBF. The bitOrder is correctly set
>>>> to MSBF when this is unparsed.
>>>>
>>>> The following siblings of this element are LE LSBF. However, the first
>>>> couple of siblings are OVC. The OVC must be suspended, but ends up
>>>> moving the bit position even though nothing is written. Since nothing is
>>>> written, bitOrder is never accessed and priorBitOrder is never set. So
>>>> it is still MSBF.
>>>>
>>>> Eventually we get to a sibling "padBit" that doesn't need to suspend. It
>>>> tries to write a padding bit at bitPosition 28 (1b). The priorBitOrder
>>>> is still MSBF but padBit is LSBF, so it complains about changing
>>>> bitOrrder on a non-byte boundary.
>>>>
>>>> The issue here is that had the first sibling not suspended it would have
>>>> changed the bitOrder to LSBF on a byte boundary and by the time we go to
>>>> "padBit" it's not on a byte boundary, but it wouldn't be changing the
>>>> bitOrder.
>>>>
>>>> So I think the solution is somehow related to having suspensions ensure
>>>> that the priorBitOrder is set correctly, even though they haven't
>>>> written anything yet. I'm not sure exactly how to do this, but this
>>>> should help narrow things down a bit.
>>>>
>>>> - Steve
>>>>
>>>> On 01/30/2018 06:36 PM, Mike Beckerle wrote:
>>>>> Grrg. Probably a regression. I will take a look Wednesday afternoon.
>>>>>
>>>>>
>>>>> -------- Original message --------
>>>>> From: Stephen Lawrence <sl...@tresys.com>
>>>>> Date: 1/30/18 5:13 PM (GMT-05:00)
>>>>> To: Mike Beckerle <mb...@tresys.com>
>>>>> Subject: bitOrder Regression
>>>>>
>>>>> Mike,
>>>>>
>>>>> I rebased the Apache changes and noticed there was a test still in
>>>>> scala-new, which are currently ignored. The test now fails when I move
>>>>> it to scala, I suspect due to some of the recent bit order changes. Not
>>>>> sure if this is a known issue and needs to be moved to scala-debug or if
>>>>> it's a regression. The test is:
>>>>>
>>>>> edu.illinois.ncsa.daffodil.section00.general.TestUnparserGeneral2.test_bitOrderOVC1
>>>>>
>>>>> - Steve
>>>>>
>>>>
>>>
>>
>
Re: bitOrder Regression - DAFFODIL-1884
Posted by Mike Beckerle <mb...@tresys.com>.
My error. The priorBitOrder is not on the P/UState. It's on the DataStreamCommonState where it is part of both data input and output stream implementations.
Makes me think the fragment bitOrder is clearly just a duplicate mechanism, as it appears on DataOutputStreamImplMixin, which extends DataStreamCommonState.
________________________________
From: Steve Lawrence <sl...@apache.org>
Sent: Monday, February 5, 2018 11:16:38 AM
To: Mike Beckerle; dev@daffodil.apache.org
Subject: Re: bitOrder Regression - DAFFODIL-1884
Does fragmentBitOrder differ from priorBitOrder? priorBitOrder always
represents the bitOrder of the last thing that was written. Seems to me
the only difference is that fragmentBitOrder will be Nope when there
isn't a fragment, and will be the same as priorBitOrder when there is a
fragment. In which case it seems the maybeFragmentBitOrder stuff can go
away?
I imagine the align issue goes like this:
1) Write less than a byte of LSBF data. priorBitOrder = LSBF
2) We want to write some data that is MBSF, write 8 - N bits of fillByte
using priorBitOrder (LSBF)
3) Write MSBF data ensuring that it starts on a byte boundary doe to the
bit order change.
So the thing here that I think might be unique (and might solve other
alignment issues? I think there's a bug for it), is to use the
priorBitOrder for writing alignment bits rather than the
FormatInfo.bitOrder object?
On 02/05/2018 11:08 AM, Mike Beckerle wrote:
>
> Taking this discussion to the dev@daffodil list.
>
> So whenever there is a suspension, we do create a new buffered data output
> stream, and a UState (actually a sub-class called UStateForSuspension, there are
> two subclasses of abstract UState, the other is UStateMain) that provides the
> additional things needed to say, compute expressions, should they be needed to
> complete the unparsing later. The UStateForSuspension object also caches the
> bitOrder, encoders/decoders, etc. - but those caches should be empty given that
> it is a new object.
>
> But that's not sufficient.
>
> Consider that we are unparsing LSBF data, and we end in the middle of a byte. So
> there is a fragment of a byte.
> Then next suppose we're on an element or model group (a Term generally) that has
> alignment 1 byte, and bit order MSBF.
> So we go to move over to the byte boundary, filling the unused bits with fill byte.
>
> Ooops. which bits. Not the MSBF bits the current term would tell us is the
> proper bit order. It should be the LSBF bits where the byte was left off.
>
> So the bit order of the fragment byte must be tracked.
>
> Looks like it is. There's a maybeFragmentBitOrder var, and a setter
> setFragmentBitOrder on the DirectOrBufferedDataOutputStream class.
>
> But, it is never called. Oops. Looks like I put the mechanism in place to do
> this, but didn't finish the implementation.
>
> ...mikeb
>
>
>
>
>
> --------------------------------------------------------------------------------
> *From:* Stephen Lawrence
> *Sent:* Monday, February 5, 2018 10:29 AM
> *To:* Mike Beckerle
> *Subject:* Re: bitOrder Regression
> Ok. Let's talk/email through this and make sure we fully understand the
> problem/what the solution would entail before decided if the fix can be
> punted or not.
>
> This just popped into my head: Perhaps the solution is everytime there
> is a suspension, we just need to create a new buffered DOS. This is
> because that suspension could potentially change the bitorder. This
> buffered DOS could then have a prior bitOrder of "unknown", or has a
> flag about what its initial bit order is, or something along those
> lines. We write to that DOS assuming the prior bit order is acceptable
> (i.e. the same as the first thing written to the buffered DOS).
>
> Then anytime we collapse a buffered DOS into the direct DOS, we just
> need to ensure that that either 1) the direct DOS ends on a byte
> boundary in which case we don't care about the bit order of the buffered
> DOS or 2) the final bitOrder of the direct DOS must match the initial
> bitOrder of the buffered DOS.
>
> I feel like we already have that logic or somethign close, so maybe the
> current logic is just slightly off.
>
> - Steve
>
> On 02/05/2018 10:20 AM, Mike Beckerle wrote:
>> I did not make any progress. I spend enough time to determine I couldn't easily
>> fix it.
>>
>>
>> The problem originated in Link16 because it uses bigEndian MSBF envelopes (NACT)
>> surrounding little endian LSBF data (Link16), however, that is working
>> currently. The bug fix is perhaps something we can live without for now. VMF
>> doesn't run into this because the envelopes (MIL-STD-2045) are also little
>> endian LSBF.
>>
>>
>> Fixing it, I'd really like to spend some time talking through the design issues
>> with you because it is subtle. it's not just a matter of finding the one spot we
>> forgot to put something in. (Well, perhaps it is, but I'd like to capture the
>> design intent and articulate it in comments in the code.)
>>
>>
>> At this point I am not sure we are keeping track of bitOrder in the right way in
>> the unparser. We depend on all unparsers running so that the underlying bit
>> stream is either marked with one bitOrder or the other because the boundary is
>> known, or it is split at the transition boundary, and checked later once the
>> actual boundary position is known. There are situations where we're not
>> recording the bit order, due to IVC elements, and perhaps due to other things.
>>
>> --------------------------------------------------------------------------------
>> *From:* Stephen Lawrence
>> *Sent:* Monday, February 5, 2018 8:51:36 AM
>> *To:* Mike Beckerle
>> *Subject:* Re: bitOrder Regression
>> Did you make any progress with this regression? I assume this test came
>> from VMF so I suspect we want it fixed for 2.1.0, but maybe it can be
>> punted to the next release? I'm not sure of its severity and what this
>> bug affects.
>>
>> - Steve
>>
>>
>> On 01/31/2018 01:14 PM, Steve Lawrence wrote:
>>> I think there might be another bug somewhere. I added a hack so that
>>> when a suspension was created it would check the bitOrder and cause a
>>> new buffered DOS if it changed (basically, I just call state.bitOrder at
>>> the end of the suspension code and that handles all the logic).
>>>
>>> However, with this hack, I found that when the test switches back to BE
>>> MSBF for the last element, the bitOrder is never checked. This means
>>> this element ends up written to a buffered DOS that had LE LSBF and
>>> things get go off the rails. I confirmed this but adding a
>>> state.bitOrder in the unparse() method to ensure that bitOrder is
>>> checked and that fixed it. However, I don't think this is the right
>>> place for it. And places that feel right to me end up with invariants.
>>> Not sure the right place for this logic.
>>>
>>> So I think there are two issues here:
>>>
>>> 1) Suspensions aren't setting/checking prior bit order
>>> 2) Bit order isn't checked when switching from LE LSBF to BE MSBF
>>>
>>> You're more familiar with this code so probably have a better idea of
>>> the right fix.
>>>
>>> - Steve
>>>
>>> On 01/31/2018 08:14 AM, Mike Beckerle wrote:
>>>> My quick tests last night concur.
>>>>
>>>> We need to insure that priorBitOrder is set and propagated correctly in the
>>>> unparser even when suspensions are created.
>>>>
>>>>
>>>> -------- Original message --------
>>>> From: Stephen Lawrence <sl...@tresys.com>
>>>> Date: 1/30/18 9:10 PM (GMT-05:00)
>>>> To: Mike Beckerle <mb...@tresys.com>
>>>> Subject: Re: bitOrder Regression
>>>>
>>>> Had some time, took at alookt at the test a little bit. Here's what I
>>>> think is going on:
>>>>
>>>> There is an element "a" that is BE MSBF. The bitOrder is correctly set
>>>> to MSBF when this is unparsed.
>>>>
>>>> The following siblings of this element are LE LSBF. However, the first
>>>> couple of siblings are OVC. The OVC must be suspended, but ends up
>>>> moving the bit position even though nothing is written. Since nothing is
>>>> written, bitOrder is never accessed and priorBitOrder is never set. So
>>>> it is still MSBF.
>>>>
>>>> Eventually we get to a sibling "padBit" that doesn't need to suspend. It
>>>> tries to write a padding bit at bitPosition 28 (1b). The priorBitOrder
>>>> is still MSBF but padBit is LSBF, so it complains about changing
>>>> bitOrrder on a non-byte boundary.
>>>>
>>>> The issue here is that had the first sibling not suspended it would have
>>>> changed the bitOrder to LSBF on a byte boundary and by the time we go to
>>>> "padBit" it's not on a byte boundary, but it wouldn't be changing the
>>>> bitOrder.
>>>>
>>>> So I think the solution is somehow related to having suspensions ensure
>>>> that the priorBitOrder is set correctly, even though they haven't
>>>> written anything yet. I'm not sure exactly how to do this, but this
>>>> should help narrow things down a bit.
>>>>
>>>> - Steve
>>>>
>>>> On 01/30/2018 06:36 PM, Mike Beckerle wrote:
>>>>> Grrg. Probably a regression. I will take a look Wednesday afternoon.
>>>>>
>>>>>
>>>>> -------- Original message --------
>>>>> From: Stephen Lawrence <sl...@tresys.com>
>>>>> Date: 1/30/18 5:13 PM (GMT-05:00)
>>>>> To: Mike Beckerle <mb...@tresys.com>
>>>>> Subject: bitOrder Regression
>>>>>
>>>>> Mike,
>>>>>
>>>>> I rebased the Apache changes and noticed there was a test still in
>>>>> scala-new, which are currently ignored. The test now fails when I move
>>>>> it to scala, I suspect due to some of the recent bit order changes. Not
>>>>> sure if this is a known issue and needs to be moved to scala-debug or if
>>>>> it's a regression. The test is:
>>>>>
>>>>> edu.illinois.ncsa.daffodil.section00.general.TestUnparserGeneral2.test_bitOrderOVC1
>>>>>
>>>>> - Steve
>>>>>
>>>>
>>>
>>
>
Re: bitOrder Regression - DAFFODIL-1884
Posted by Steve Lawrence <sl...@apache.org>.
Does fragmentBitOrder differ from priorBitOrder? priorBitOrder always
represents the bitOrder of the last thing that was written. Seems to me
the only difference is that fragmentBitOrder will be Nope when there
isn't a fragment, and will be the same as priorBitOrder when there is a
fragment. In which case it seems the maybeFragmentBitOrder stuff can go
away?
I imagine the align issue goes like this:
1) Write less than a byte of LSBF data. priorBitOrder = LSBF
2) We want to write some data that is MBSF, write 8 - N bits of fillByte
using priorBitOrder (LSBF)
3) Write MSBF data ensuring that it starts on a byte boundary doe to the
bit order change.
So the thing here that I think might be unique (and might solve other
alignment issues? I think there's a bug for it), is to use the
priorBitOrder for writing alignment bits rather than the
FormatInfo.bitOrder object?
On 02/05/2018 11:08 AM, Mike Beckerle wrote:
>
> Taking this discussion to the dev@daffodil list.
>
> So whenever there is a suspension, we do create a new buffered data output
> stream, and a UState (actually a sub-class called UStateForSuspension, there are
> two subclasses of abstract UState, the other is UStateMain) that provides the
> additional things needed to say, compute expressions, should they be needed to
> complete the unparsing later. The UStateForSuspension object also caches the
> bitOrder, encoders/decoders, etc. - but those caches should be empty given that
> it is a new object.
>
> But that's not sufficient.
>
> Consider that we are unparsing LSBF data, and we end in the middle of a byte. So
> there is a fragment of a byte.
> Then next suppose we're on an element or model group (a Term generally) that has
> alignment 1 byte, and bit order MSBF.
> So we go to move over to the byte boundary, filling the unused bits with fill byte.
>
> Ooops. which bits. Not the MSBF bits the current term would tell us is the
> proper bit order. It should be the LSBF bits where the byte was left off.
>
> So the bit order of the fragment byte must be tracked.
>
> Looks like it is. There's a maybeFragmentBitOrder var, and a setter
> setFragmentBitOrder on the DirectOrBufferedDataOutputStream class.
>
> But, it is never called. Oops. Looks like I put the mechanism in place to do
> this, but didn't finish the implementation.
>
> ...mikeb
>
>
>
>
>
> --------------------------------------------------------------------------------
> *From:* Stephen Lawrence
> *Sent:* Monday, February 5, 2018 10:29 AM
> *To:* Mike Beckerle
> *Subject:* Re: bitOrder Regression
> Ok. Let's talk/email through this and make sure we fully understand the
> problem/what the solution would entail before decided if the fix can be
> punted or not.
>
> This just popped into my head: Perhaps the solution is everytime there
> is a suspension, we just need to create a new buffered DOS. This is
> because that suspension could potentially change the bitorder. This
> buffered DOS could then have a prior bitOrder of "unknown", or has a
> flag about what its initial bit order is, or something along those
> lines. We write to that DOS assuming the prior bit order is acceptable
> (i.e. the same as the first thing written to the buffered DOS).
>
> Then anytime we collapse a buffered DOS into the direct DOS, we just
> need to ensure that that either 1) the direct DOS ends on a byte
> boundary in which case we don't care about the bit order of the buffered
> DOS or 2) the final bitOrder of the direct DOS must match the initial
> bitOrder of the buffered DOS.
>
> I feel like we already have that logic or somethign close, so maybe the
> current logic is just slightly off.
>
> - Steve
>
> On 02/05/2018 10:20 AM, Mike Beckerle wrote:
>> I did not make any progress. I spend enough time to determine I couldn't easily
>> fix it.
>>
>>
>> The problem originated in Link16 because it uses bigEndian MSBF envelopes (NACT)
>> surrounding little endian LSBF data (Link16), however, that is working
>> currently. The bug fix is perhaps something we can live without for now. VMF
>> doesn't run into this because the envelopes (MIL-STD-2045) are also little
>> endian LSBF.
>>
>>
>> Fixing it, I'd really like to spend some time talking through the design issues
>> with you because it is subtle. it's not just a matter of finding the one spot we
>> forgot to put something in. (Well, perhaps it is, but I'd like to capture the
>> design intent and articulate it in comments in the code.)
>>
>>
>> At this point I am not sure we are keeping track of bitOrder in the right way in
>> the unparser. We depend on all unparsers running so that the underlying bit
>> stream is either marked with one bitOrder or the other because the boundary is
>> known, or it is split at the transition boundary, and checked later once the
>> actual boundary position is known. There are situations where we're not
>> recording the bit order, due to IVC elements, and perhaps due to other things.
>>
>> --------------------------------------------------------------------------------
>> *From:* Stephen Lawrence
>> *Sent:* Monday, February 5, 2018 8:51:36 AM
>> *To:* Mike Beckerle
>> *Subject:* Re: bitOrder Regression
>> Did you make any progress with this regression? I assume this test came
>> from VMF so I suspect we want it fixed for 2.1.0, but maybe it can be
>> punted to the next release? I'm not sure of its severity and what this
>> bug affects.
>>
>> - Steve
>>
>>
>> On 01/31/2018 01:14 PM, Steve Lawrence wrote:
>>> I think there might be another bug somewhere. I added a hack so that
>>> when a suspension was created it would check the bitOrder and cause a
>>> new buffered DOS if it changed (basically, I just call state.bitOrder at
>>> the end of the suspension code and that handles all the logic).
>>>
>>> However, with this hack, I found that when the test switches back to BE
>>> MSBF for the last element, the bitOrder is never checked. This means
>>> this element ends up written to a buffered DOS that had LE LSBF and
>>> things get go off the rails. I confirmed this but adding a
>>> state.bitOrder in the unparse() method to ensure that bitOrder is
>>> checked and that fixed it. However, I don't think this is the right
>>> place for it. And places that feel right to me end up with invariants.
>>> Not sure the right place for this logic.
>>>
>>> So I think there are two issues here:
>>>
>>> 1) Suspensions aren't setting/checking prior bit order
>>> 2) Bit order isn't checked when switching from LE LSBF to BE MSBF
>>>
>>> You're more familiar with this code so probably have a better idea of
>>> the right fix.
>>>
>>> - Steve
>>>
>>> On 01/31/2018 08:14 AM, Mike Beckerle wrote:
>>>> My quick tests last night concur.
>>>>
>>>> We need to insure that priorBitOrder is set and propagated correctly in the
>>>> unparser even when suspensions are created.
>>>>
>>>>
>>>> -------- Original message --------
>>>> From: Stephen Lawrence <sl...@tresys.com>
>>>> Date: 1/30/18 9:10 PM (GMT-05:00)
>>>> To: Mike Beckerle <mb...@tresys.com>
>>>> Subject: Re: bitOrder Regression
>>>>
>>>> Had some time, took at alookt at the test a little bit. Here's what I
>>>> think is going on:
>>>>
>>>> There is an element "a" that is BE MSBF. The bitOrder is correctly set
>>>> to MSBF when this is unparsed.
>>>>
>>>> The following siblings of this element are LE LSBF. However, the first
>>>> couple of siblings are OVC. The OVC must be suspended, but ends up
>>>> moving the bit position even though nothing is written. Since nothing is
>>>> written, bitOrder is never accessed and priorBitOrder is never set. So
>>>> it is still MSBF.
>>>>
>>>> Eventually we get to a sibling "padBit" that doesn't need to suspend. It
>>>> tries to write a padding bit at bitPosition 28 (1b). The priorBitOrder
>>>> is still MSBF but padBit is LSBF, so it complains about changing
>>>> bitOrrder on a non-byte boundary.
>>>>
>>>> The issue here is that had the first sibling not suspended it would have
>>>> changed the bitOrder to LSBF on a byte boundary and by the time we go to
>>>> "padBit" it's not on a byte boundary, but it wouldn't be changing the
>>>> bitOrder.
>>>>
>>>> So I think the solution is somehow related to having suspensions ensure
>>>> that the priorBitOrder is set correctly, even though they haven't
>>>> written anything yet. I'm not sure exactly how to do this, but this
>>>> should help narrow things down a bit.
>>>>
>>>> - Steve
>>>>
>>>> On 01/30/2018 06:36 PM, Mike Beckerle wrote:
>>>>> Grrg. Probably a regression. I will take a look Wednesday afternoon.
>>>>>
>>>>>
>>>>> -------- Original message --------
>>>>> From: Stephen Lawrence <sl...@tresys.com>
>>>>> Date: 1/30/18 5:13 PM (GMT-05:00)
>>>>> To: Mike Beckerle <mb...@tresys.com>
>>>>> Subject: bitOrder Regression
>>>>>
>>>>> Mike,
>>>>>
>>>>> I rebased the Apache changes and noticed there was a test still in
>>>>> scala-new, which are currently ignored. The test now fails when I move
>>>>> it to scala, I suspect due to some of the recent bit order changes. Not
>>>>> sure if this is a known issue and needs to be moved to scala-debug or if
>>>>> it's a regression. The test is:
>>>>>
>>>>> edu.illinois.ncsa.daffodil.section00.general.TestUnparserGeneral2.test_bitOrderOVC1
>>>>>
>>>>> - Steve
>>>>>
>>>>
>>>
>>
>