Parse Error on Out of Range Binary Integers

[olabusayoT]

• add checks for signed/unsigned in BinaryIntegerBaseParser and BinaryIntegerBaseUnparser
• add tests for parse and unparse
• update failing tests

DAFFODIL-2297

[stevedlawrence]

This is going to call getBitLength twice, once here and once when super is called. We should probably avoid that.

Also, it looks like we don't have any checks on the max size for numbers like we do in parse. Maybe we should just do all the min/max length checks in the main unparse function like we do with parse? Or maybe in BinaryIntegerBaseUnparser.putNumber?

[olabusayoT]

Hmm, the issue is putNumber doesn't have access to state, which we need for Unparse error and BinaryNumberBaseUnparser is used by non Binary Integer classes

[stevedlawrence]

the issue is putNumber doesn't have access to state

When we call putNumber we actually do pass in the state, it is just upcast to a FormatInfo so putNumber doesn't realize it's actually a UState. But we can change the FormatInfo parameter to UState and it should all just work. I don't think there's a reason putNumber must take a FormatInfo instead of a UState.

BinaryNumberBaseUnparser is used by non Binary Integer classes

Could we put all the width logic in BinaryIntegerBaseUnparser.putNumber(). Then it will only apply to integer types, which I think is exactly what we want?

[mbeckerle]

When there is no access to state, I think the technique is to throw a thin-exception to someplace that catches it that has the state needed to report the error in context properly.

[stevedlawrence]

the issue is putNumber doesn't have access to state

When we call putNumber we actually do pass in the state, it is just upcast to a FormatInfo so putNumber doesn't realize it's actually a UState. But we can change the FormatInfo parameter to UState and it should all just work. I don't think there's a reason putNumber must take a FormatInfo instead of a UState.

BinaryNumberBaseUnparser is used by non Binary Integer classes

Could we put all the width logic in BinaryIntegerBaseUnparser.putNumber(). Then it will only apply to integer types, which I think is exactly what we want?

[stevedlawrence]

Thoughts on renaming width to maxWidth and adding a new minWidth member in NodeInfo. Avoids some of the special logic around figuring out if stuff is signed or not. And then checking maxWidth and minWidth is very similar logic, just something like

if (primNumeric.minWdith.isDefined) {
   val minWidth = primNumeric.minWidth.get
   if(nBits < minWidth) {
     SDE(...)
   }
}

And then the minWidth can be reused in the parser/unparser logic.

Maybe we also add a signed member so we don't have to have the isSigned/isUnsigned stuff? And then parser/unparsers, which should already have access to the primType can just reference the signed member? We pass around isSigned in a bunch of places, wondering if maybe that can all be simplified.

[stevedlawrence]

isSigned and signedStr aren't used unless we fall into the nBits < minWidth block. Suggest we move this val into that block.

[stevedlawrence]

Does this just want to be a lazy val now that it isn't parameterized?

[stevedlawrence]

Seems this function is only use for numerics, this probably wants to be an assert.impossible with code coverage disabed, or just do e.primType.asInstanceOf[PrimNumeric].isSigned

[stevedlawrence]

Same as above, suggest moving vals into the block where they are used. Avoids unnecessary work (though in this case the work is pretty minimal).

[stevedlawrence]

Is this return false correct? If we are only warning then should we still unparse something? For example, if nBits is 1 and we are signed but allowSignedIntegerLength1Bit, then we should output 1 bit. Seems like we want to warn but then drop through and still put a number.

[olabusayoT]

Hmm, so I have a test that exercises it, and we go from an infoset containing -1 to unparsing 1, since it is only 1 bit. Is that ok?

[stevedlawrence]

I think that's probably correct? If we think about the min/max values of a 2's complement value, the min value is given as -(2^n-1) and the max value is (2^n-1)-1, where n is the number of bits. That gives us a range of -1 to 0. So -1 unparsing to a single 1 bit seems reasonable. Hopefully our parse logic is consistent with that when this tunabled is enabled.

[olabusayoT]

So it looks like we parse data that is -1 to an infoset containing 0, which I think we should be unparsing to 0 instead of 1 here since the max value is 0?

[stevedlawrence]

(previous comment removed, it was wrong)

So it looks like, for parse
0x0 -> 0
0x1 -> 1

And for unparse:
0 -> 0x0
1 -> 0x1
-1 -> 0x1

Other infoset values do not throw an error, so I guess we just aren't doing proper range checking that the value fits in the number of bits?

I'm not sure 2's complement really makes sense with just a single bit, so I guess always parsing/unparse to 0 or 1 is just as logical as something else, and at least it's symmetric.

[mbeckerle]

Truncation of a number because it doesn't fit in the available space is supposed to be an unparse error.

[stevedlawrence]

Just below we have this line:

if (nBits == 0) return // zero length is used for outputValueCalc often.

The purpose of this bug was to make it so zero length numbers is an error. I don't understand why outputValueCalc would have zero length, but based on the bug it sounds like that shouldn't be allowed any more. Should that change to an unparse error?

[olabusayoT]

Above this we have PackedDecimal that also returns if nBits is 0, should we also make that a PE? Also PackedDecimal doesn't pass along decimalSigned to the base, is it logical to do something like the below? If so, they I think we need to update the PackedDecimal classes to pass along decimalSigned

<xs:element name="num" type="xs:decimal"  dfdl:representation="binary" dfdl:binaryNumberRep="ibm4690Packed" dfdl:decimalSigned="yes" dfdl:binaryDecimalVirtualPoint="0"/>

[mbeckerle]

Yeah, I think you need to know if the decimal representation is expected to be signed or not. The error case of dfdl:decimalSigned="no" but the packed representation has a negative sign needs to be detected somehow.
One could choose different parse primitives based on dfdl:decimalSigned yes/no or pass a boolean isSigned to a common primitive.

[stevedlawrence]

+1, nice improvements. I think my only comments is do reduce a bunch of copy/pasting, and I think we need different behavior/error messages for packed numbers (which we might already have?)

[stevedlawrence]

Are there any concerns about creating SDW's at parse time? Diagnostic overhead should be small, but imagine an array of a bunch of 1-bit length elements--that could create a lot of diagnostics. Also consider that this same warning was already output during compilation unless nbits was calculated at runtime. Maybe the compile time warning is sufficient and we should just be silent at runtime? Or maybe we need a flag so we only warn here only if we're in the BinaryIntegerKnownLengthUnparser?

[mbeckerle]

I'm in favor of silent success at runtime if the flag is set to allow 1-bit signed (for compatibility), and parse-error if the flag is not set.

[stevedlawrence]

I think this same chunk of code with very minor variations appears in multiple places. Can we add trait to avoid all this duplication?

[stevedlawrence]

I'm not sure this error message is correct. I believe packed numbers have different rules about minimum lengths. It's not the same as 2's complement.

Also, this message references "binary decimal", which I think implies 2's complement. Any error messages here should probably reference packed decimals, or leave it up to individual packed parsers to detect bitLength errors and give specific errors.

I think what was intended with the packed implementation is we just call toBigDecimal/toBigInteger below and the different packed implementations do their parsing/validation and throw an exception if it's not a valid length/value, and that exception is converted into a PE.

So I think all this 1-bit signed stuff done elsewhere doesn't even apply to packed decimals. Packed decimals should already be doing bit length checking specific to the packed format. If they handle the zero case correctly, we could probably even remove this check/PE. If not, we probably still need it, but should have an error specific to zero length packed decimals not allowed.

It might also be worth adding a test for some zero length packed things so we get coverage of this edge case.

[mbeckerle]

"binary decimal" means type xs:decimal dfdl:binaryNumberRep="binary" i.e., twos complement.

A fraction part is specified using dfdl:binaryDecimalVirtualPoint="3" for example to get 999.999 (3 digits after the decimal point)

So "binary decimal" behaves like a twos-complement signed integer vis a vis this 1-bit compatibility mode.

The packed representations are entirely different, and 1-bit stuff does not apply. They must be aligned 4-bit and a multiple of 4-bits length.

[olabusayoT]

It doesn't look like the indvidual packed parsers/unparser handle the nBits == 0 case, Before we were just returning when nBits was 0, should we revert the code, or just update the error to remove the suggestion of what could be used instead, and update the messaging to state nBits == 0 is not allowed.

[mbeckerle]

Well, we may be depending on this zero-length behavior. See the comment in the code about zero length is used by outputValueCalc. But this doesn't really make sense to me. Some things, like strings and hexBinary, can be zero length, but many data types zero length makes no sense. There is defaulting, e.g., when we are trying to parse an integer and we get zero-length back we may provide a default value for the integer. But in the absence of defaulting, most simple types require at least 1 bit.

[olabusayoT]

I'm not sure about that comment. Isn't OVC only used on unparsing? Also, none of our OVC tests failed when we changed the Return to a PE

[mbeckerle]

Yes, OVC is only for unparsing. If no test fail when making this a PE, then I would also delete the comment about OVC, which makes no sense if this is parser code anyway.

[stevedlawrence]

Same question as above, I'm not sure this error makes sense for packed numbers and can possibly be removed?

[mbeckerle]

It may be possible to remove the check for nbits == 0 for packed decimal representations (including IBM4690Packed) because a check for 4-bit multiple has to be elsewhere and that presumably will also rule out the zero case. At least one of these in the code does not have test coverage.

I suggest removing the code that does check for nbits == 0 for the packed numbers and re-test.

I also think removing all the passing of dfdl:decimalSigned as a boolean to the packed/ibm4690 primitives means you cannot detect a negative representation being parsed into an unsigned number.

[mbeckerle]

Note that this change and tunable requires a release note in the next release suggesting that people update schemas to not depend on 1-bit signed binary integers.

[mbeckerle]

Somewhere else we must be checking for packed length not a multiple of 4 bits, and having room for the packed sign nibble, etc. So I suspect we don't need to check for zero bits as a special case here.

[olabusayoT]

I can't find where we check the packed length is a multiple of 4 bits. Unless you're referring to what we do in the function below?

def bcdFromBigIntegerLength(absBigIntAsString: String, minLengthInBits: Int): (Int, Int) = {
    val numDigits = absBigIntAsString.length
    // Need to have an even number of digits to fill out a complete byte
    val requiredBitLen = if (numDigits % 2 == 0) (numDigits * 4) else ((numDigits + 1) * 4)
    val bitLen = scala.math.max(minLengthInBits, requiredBitLen)
    val numBytes = (bitLen / 8)
    val leadingZeros = bitLen / 4 - numDigits
    (numBytes, leadingZeros)
  }

[olabusayoT]

Op, nvm. I found it in org.apache.daffodil.core.grammar.ElementBaseGrammarMixin

 if ((binaryNumberKnownLengthInBits != -1) && (binaryNumberKnownLengthInBits % 4) != 0)

[olabusayoT]

This logic doesn't preclude nBits == 0, and it only checks for when nBits is known. I think we should leave in the 0 bits check as it is now, because it covers when the nBits is known and when it is calculated. Thoughts?

[stevedlawrence]

Yeah, you're right, we can't rely entirely compile time validation since lengths could be runtime calculate.

In fact, in addition to checking it's not zero length, we also need to check it's a multiple for 4 at runtime. Right now during runtime, we just ask for a byte array that is nbits long and the packed logic assumes the array is completely full, even though it could have partial fragment bytes.

So I think we do need to keep this 0 check, but also need to add a % 4 == 0 check.

[mbeckerle]

This does need test coverage. It may be possible to remove this check entirely. It may not be reachable in that something else is checking for the zero case before this is called?

[olabusayoT]

It is actually reachable. I've added the test. I don't think we ought to remove this check as we don't check for nBits == 0 anywhere else

[mbeckerle]

Yeah, I think you need to know if the decimal representation is expected to be signed or not. The error case of dfdl:decimalSigned="no" but the packed representation has a negative sign needs to be detected somehow.
One could choose different parse primitives based on dfdl:decimalSigned yes/no or pass a boolean isSigned to a common primitive.

[mbeckerle]

I think you need to pass the signed boolean.

[stevedlawrence]

Does using the signedness of the primitive type work? I think the only time xs:decimalSign should matter is with the primitive type is xs:decimal, which uses the PackedDecimal* parsers, not these PackedInteger* parsers. So if feels like we do need a signed boolean for PackedDecimal* parsers, but I think that's just an existing bug that could maybe be added in a separate PR, since this one is more about ranges of things.

[olabusayoT]

So this signed boolean was replaced by the primitives signedness...as we were just passing in true/false depending on whether the primitive was signed or unsigned...the PAckedDecimalParsers never passed in decimalSigned, so that wasn't remove...as @stevedlawrence said I think that's a separate bug

[olabusayoT]

Created DAFFODIL-2963 to track separate bug

[stevedlawrence]

Also, #1391 modifies packed integers to use PrimType.fromNumber, which validates signedness according to the primitive, and the signed check in this parser goes away.

[jadams-tresys]

+1

I think all comments have been addressed and I confirmed that with the comibnation of these changes and the ones for PR-1391 that we can remove all the isSigned parameters being passed around and rely on the PrimitiveNumeric.fromNumber to do appropriate sign/type checking.

[mbeckerle]

+1

[olabusayoT]

Note this PR causes regressions in dfdl-nitf, dfdl-vmf and vmf-pmmc-dfdl