From 693bc159196d4f44ff402980da8c463b307003c5 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Wed, 27 May 2020 17:04:43 +0200 Subject: [PATCH] avoid some RVO bugs --- beacon_chain/ssz.nim | 11 +++-- beacon_chain/ssz/bytes_reader.nim | 75 ++++++++++++++++--------------- beacon_chain/ssz/navigator.nim | 2 +- tests/official/fixtures_utils.nim | 2 +- 4 files changed, 48 insertions(+), 42 deletions(-) diff --git a/beacon_chain/ssz.nim b/beacon_chain/ssz.nim index 9562da1a1..bdc818065 100644 --- a/beacon_chain/ssz.nim +++ b/beacon_chain/ssz.nim @@ -12,6 +12,10 @@ # nim-beacon-chain/beacon_chain/ssz.nim(212, 18) Error: can raise an unlisted exception: IOError #{.push raises: [Defect].} +# TODO Many RVO bugs, careful +# https://github.com/nim-lang/Nim/issues/14470 +# https://github.com/nim-lang/Nim/issues/14126 + import options, algorithm, options, strformat, typetraits, stew/[bitops2, bitseqs, endians2, objects, varints, ptrops], @@ -65,6 +69,7 @@ serializationFormat SSZ, template decode*(Format: type SSZ, input: openarray[byte], RecordType: distinct type): auto = + # TODO how badly is this affected by RVO bugs? serialization.decode(SSZ, input, RecordType, maxObjectSize = input.len) template loadFile*(Format: type SSZ, @@ -291,20 +296,20 @@ proc readValue*[T](r: var SszReader, val: var T) {.raises: [Defect, MalformedSsz when isFixedSize(T): const minimalSize = fixedPortionSize(T) if r.stream.readable(minimalSize): - val = readSszValue(r.stream.read(minimalSize), T) + readSszValue(r.stream.read(minimalSize), val) else: raise newException(MalformedSszError, "SSZ input of insufficient size") else: # TODO Read the fixed portion first and precisely measure the size of # the dynamic portion to consume the right number of bytes. - val = readSszValue(r.stream.read(r.stream.len.get), T) + readSszValue(r.stream.read(r.stream.len.get), val) proc readValue*[T](r: var SszReader, val: var SizePrefixed[T]) {.raises: [Defect].} = let length = r.stream.readVarint(uint64) if length > r.maxObjectSize: raise newException(SszMaxSizeExceeded, "Maximum SSZ object size exceeded: " & $length) - val = readSszValue(r.stream.read(length), T) + readSszValue(r.stream.read(length), T(val)) const zeroChunk = default array[32, byte] diff --git a/beacon_chain/ssz/bytes_reader.nim b/beacon_chain/ssz/bytes_reader.nim index 6a08bad88..49ebdc57a 100644 --- a/beacon_chain/ssz/bytes_reader.nim +++ b/beacon_chain/ssz/bytes_reader.nim @@ -84,11 +84,9 @@ template checkForForbiddenBits(ResulType: type, if (input[^1] and forbiddenBitsMask) != 0: raiseIncorrectSize ResulType -func readSszValue*(input: openarray[byte], T: type): T {.raisesssz.} = +func readSszValue*(input: openarray[byte], val: var auto) {.raisesssz.} = mixin fromSszBytes, toSszType - type T {.used.} = type(result) - template readOffsetUnchecked(n: int): int {.used.}= int fromSszBytes(uint32, input.toOpenArray(n, n + offsetSize - 1)) @@ -106,42 +104,45 @@ func readSszValue*(input: openarray[byte], T: type): T {.raisesssz.} = # result.checkOutputSize input.len # readOpenArray(result, input) - when result is BitList: + when val is BitList: if input.len == 0: raise newException(MalformedSszError, "Invalid empty SSZ BitList value") - const maxExpectedSize = (result.maxLen div 8) + 1 - result = T readSszValue(input, List[byte, maxExpectedSize]) + const maxExpectedSize = (val.maxLen div 8) + 1 + # TODO can't cast here.. + var v: List[byte, maxExpectedSize] + readSszValue(input, v) + val = (type val)(v) - let resultBytesCount = len bytes(result) + let resultBytesCount = len bytes(val) - if bytes(result)[resultBytesCount - 1] == 0: + if bytes(val)[resultBytesCount - 1] == 0: raise newException(MalformedSszError, "SSZ BitList is not properly terminated") if resultBytesCount == maxExpectedSize: - checkForForbiddenBits(T, input, result.maxLen + 1) + checkForForbiddenBits(type val, input, val.maxLen + 1) - elif result is List|array: - type E = type result[0] + elif val is List|array: + type E = type val[0] when E is byte: - result.setOutputSize input.len + val.setOutputSize input.len if input.len > 0: - copyMem(addr result[0], unsafeAddr input[0], input.len) + copyMem(addr val[0], unsafeAddr input[0], input.len) elif isFixedSize(E): const elemSize = fixedPortionSize(E) if input.len mod elemSize != 0: var ex = new SszSizeMismatchError - ex.deserializedType = cstring typetraits.name(T) + ex.deserializedType = cstring typetraits.name(type val) ex.actualSszSize = input.len ex.elementSize = elemSize raise ex - result.setOutputSize input.len div elemSize - trs "READING LIST WITH LEN ", result.len - for i in 0 ..< result.len: + val.setOutputSize input.len div elemSize + trs "READING LIST WITH LEN ", val.len + for i in 0 ..< val.len: trs "TRYING TO READ LIST ELEM ", i let offset = i * elemSize - result[i] = readSszValue(input.toOpenArray(offset, offset + elemSize - 1), E) + readSszValue(input.toOpenArray(offset, offset + elemSize - 1), val[i]) trs "LIST READING COMPLETE" else: @@ -164,36 +165,36 @@ func readSszValue*(input: openarray[byte], T: type): T {.raisesssz.} = # not matching up with its nextOffset properly) raise newException(MalformedSszError, "SSZ list incorrectly encoded of zero length") - result.setOutputSize resultLen + val.setOutputSize resultLen for i in 1 ..< resultLen: let nextOffset = readOffset(i * offsetSize) if nextOffset <= offset: raise newException(MalformedSszError, "SSZ list element offsets are not monotonically increasing") else: - result[i - 1] = readSszValue(input.toOpenArray(offset, nextOffset - 1), E) + readSszValue(input.toOpenArray(offset, nextOffset - 1), val[i - 1]) offset = nextOffset - result[resultLen - 1] = readSszValue(input.toOpenArray(offset, input.len - 1), E) + readSszValue(input.toOpenArray(offset, input.len - 1), val[resultLen - 1]) # TODO: Should be possible to remove BitArray from here - elif result is UintN|bool|enum: - trs "READING BASIC TYPE ", type(result).name, " input=", input.len - result = fromSszBytes(type(result), input) - trs "RESULT WAS ", repr(result) + elif val is UintN|bool|enum: + trs "READING BASIC TYPE ", type(val).name, " input=", input.len + val = fromSszBytes(type(val), input) + trs "RESULT WAS ", repr(val) - elif result is BitArray: - if sizeof(result) != input.len: - raiseIncorrectSize T - checkForForbiddenBits(T, input, result.bits) - copyMem(addr result.bytes[0], unsafeAddr input[0], input.len) + elif val is BitArray: + if sizeof(val) != input.len: + raiseIncorrectSize(type val) + checkForForbiddenBits(type val, input, val.bits) + copyMem(addr val.bytes[0], unsafeAddr input[0], input.len) - elif result is object|tuple: - const minimallyExpectedSize = fixedPortionSize(T) + elif val is object|tuple: + const minimallyExpectedSize = fixedPortionSize(type val) if input.len < minimallyExpectedSize: raise newException(MalformedSszError, "SSZ input of insufficient size") - enumInstanceSerializedFields(result, fieldName, field): - const boundingOffsets = T.getFieldBoundingOffsets(fieldName) + enumInstanceSerializedFields(val, fieldName, field): + const boundingOffsets = getFieldBoundingOffsets(type val, fieldName) trs "BOUNDING OFFSET FOR FIELD ", fieldName, " = ", boundingOffsets # type FieldType = type field # buggy @@ -229,9 +230,9 @@ func readSszValue*(input: openarray[byte], T: type): T {.raisesssz.} = # TODO passing in `FieldType` instead of `type(field)` triggers a # bug in the compiler - field = readSszValue( + readSszValue( input.toOpenArray(startOffset, endOffset - 1), - type(field)) + field) trs "READING COMPLETE ", fieldName else: @@ -241,4 +242,4 @@ func readSszValue*(input: openarray[byte], T: type): T {.raisesssz.} = input.toOpenArray(startOffset, endOffset - 1)) else: - unsupported T + unsupported (type val) diff --git a/beacon_chain/ssz/navigator.nim b/beacon_chain/ssz/navigator.nim index 46fdbb469..6ebad498e 100644 --- a/beacon_chain/ssz/navigator.nim +++ b/beacon_chain/ssz/navigator.nim @@ -121,7 +121,7 @@ func `[]`*[T](n: SszNavigator[T]): T {.raisesssz.} = mixin toSszType, fromSszBytes type SszRepr = type toSszType(declval T) when type(SszRepr) is type(T) or T is List: - readSszValue(toOpenArray(n.m), T) + readSszValue(toOpenArray(n.m), result) else: fromSszBytes(T, toOpenArray(n.m)) diff --git a/tests/official/fixtures_utils.nim b/tests/official/fixtures_utils.nim index 661d96b42..6a3737c32 100644 --- a/tests/official/fixtures_utils.nim +++ b/tests/official/fixtures_utils.nim @@ -58,7 +58,7 @@ template readFileBytes*(path: string): seq[byte] = proc sszDecodeEntireInput*(input: openarray[byte], Decoded: type): Decoded = var stream = unsafeMemoryInput(input) var reader = init(SszReader, stream, input.len) - result = reader.readValue(Decoded) + reader.readValue(result) if stream.readable: raise newException(UnconsumedInput, "Remaining bytes in the input")