From 306d141761516fb7646ec33481d4326b7b01f1d7 Mon Sep 17 00:00:00 2001 From: Benjamin Bannier Date: Mon, 5 Feb 2024 16:36:45 +0100 Subject: [PATCH] Fix incorrect data consumption for `&max-size`. We would previously handle `&size` and `&max-size` almost identical with the only difference that `&max-size` sets up a slightly larger view to accomodate a sentinel. In particular, we also used identical code to set up the position where parsing should resume after such a field. This was incorrect as it is in general impossible to tell where parsing continues after a field with `&max-size` since it does not signify a fixed view like `&size`. In this patch we now compute the next position for a `&max-size` field by inspecting the limited view to detect how much data was extracted. Closes #1668. --- .../src/compiler/codegen/parser-builder.cc | 16 +++++++++- .../spicy.types.unit.field-max-size/output | 4 +-- .../spicy.types.unit.field-max-size/output2 | 11 +++++++ tests/spicy/types/unit/field-max-size.spicy | 31 ++++++++++++++----- 4 files changed, 52 insertions(+), 10 deletions(-) create mode 100644 tests/Baseline/spicy.types.unit.field-max-size/output2 diff --git a/spicy/toolchain/src/compiler/codegen/parser-builder.cc b/spicy/toolchain/src/compiler/codegen/parser-builder.cc index 6e80ade1d..b6b49f8e0 100644 --- a/spicy/toolchain/src/compiler/codegen/parser-builder.cc +++ b/spicy/toolchain/src/compiler/codegen/parser-builder.cc @@ -778,6 +778,11 @@ struct ProductionVisitor pb->parseError("parsing not done within &max-size bytes", a->meta()); }); + + // For `&max-size` we use `ncur` to store a view limited and slightly larger than the _permissible_ range, + // not the _consumed_ range. Update the value with where we ended up during parsing so we can inspect it + // below when setting the position after parsing. + ncur = state().cur; } else if ( auto a = AttributeSet::find(field->attributes(), "&size") ) { @@ -814,8 +819,17 @@ struct ProductionVisitor pb->saveParsePosition(); } - if ( ncur ) + if ( ncur ) { + // For `&max-size` we use `ncur` to store a view limited and slightly larger than the _permissible_ range, + // not the _consumed_ range. We need to manually compute the actually consumed range from where we ended + // parsing in the limited view. We update the value stored in `ncur` so it can be updated below. + if ( AttributeSet::find(field->attributes(), "&max-size") ) + ncur = builder::memberCall(state().cur, "advance", + {builder::difference(builder::memberCall(*ncur, "offset", {}), + builder::memberCall(state().cur, "offset", {}))}); + builder()->addAssign(state().cur, *ncur); + } if ( ! meta.container() ) { if ( pb->isEnabledDefaultNewValueForField() && state().literal_mode == LiteralMode::Default ) diff --git a/tests/Baseline/spicy.types.unit.field-max-size/output b/tests/Baseline/spicy.types.unit.field-max-size/output index fd7a8711c..86d9db73f 100644 --- a/tests/Baseline/spicy.types.unit.field-max-size/output +++ b/tests/Baseline/spicy.types.unit.field-max-size/output @@ -2,6 +2,6 @@ done, [$xs=b"\x00"] done, [$xs=b"\x01\x00"] error, [$xs=(not set)] -[error] terminating with uncaught exception of type spicy::rt::ParseError: parsing not done within &max-size bytes (<...>/field-max-size.spicy:15:40-15:56) +[error] terminating with uncaught exception of type spicy::rt::ParseError: parsing not done within &max-size bytes (<...>/field-max-size.spicy:16:40-16:56) error, [$xs=b"\x01\x01\x01"] -[error] terminating with uncaught exception of type spicy::rt::ParseError: end-of-data reached before &until-including expression found (<...>/field-max-size.spicy:15:32-15:38) +[error] terminating with uncaught exception of type spicy::rt::ParseError: end-of-data reached before &until-including expression found (<...>/field-max-size.spicy:16:32-16:38) diff --git a/tests/Baseline/spicy.types.unit.field-max-size/output2 b/tests/Baseline/spicy.types.unit.field-max-size/output2 new file mode 100644 index 000000000..e04538d51 --- /dev/null +++ b/tests/Baseline/spicy.types.unit.field-max-size/output2 @@ -0,0 +1,11 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +Mini::X { + use_max_size: True + xs: + rest: BC +} +Mini::X { + use_max_size: False + xs: + rest: BC +} diff --git a/tests/spicy/types/unit/field-max-size.spicy b/tests/spicy/types/unit/field-max-size.spicy index 0b1a557f9..c5252de3f 100644 --- a/tests/spicy/types/unit/field-max-size.spicy +++ b/tests/spicy/types/unit/field-max-size.spicy @@ -1,14 +1,15 @@ -# @TEST-EXEC: spicyc -j -d %INPUT -o test.hlto -# @TEST-EXEC: printf '\000' | spicy-driver -d test.hlto >>output 2>&1 -# @TEST-EXEC: printf '\001\000' | spicy-driver -d test.hlto >>output 2>&1 -# @TEST-EXEC-FAIL: printf '\001\001\000' | spicy-driver -d test.hlto >>output 2>&1 -# @TEST-EXEC-FAIL: printf '\001\001\001\000' | spicy-driver -d test.hlto >>output 2>&1 -# @TEST-EXEC: btest-diff output -# # @TEST-DOC: Validate effects of field-level `&max-size` attribute +# +# @TEST-EXEC: spicyc -j -d %INPUT -o test.hlto module Mini; +# @TEST-EXEC: printf '\000' | spicy-driver -d test.hlto -p Mini::Test >>output 2>&1 +# @TEST-EXEC: printf '\001\000' | spicy-driver -d test.hlto -p Mini::Test >>output 2>&1 +# @TEST-EXEC-FAIL: printf '\001\001\000' | spicy-driver -d test.hlto -p Mini::Test >>output 2>&1 +# @TEST-EXEC-FAIL: printf '\001\001\001\000' | spicy-driver -d test.hlto -p Mini::Test >>output 2>&1 +# @TEST-EXEC: btest-diff output + const MaxSize = 2; public type Test = unit { @@ -17,3 +18,19 @@ public type Test = unit { on %done { print "done", self; } on %error { print "error", self; } }; + +# Check that `&max-size` has no effect on how much data is consumed. This is a regression test for #1668. +# @TEST-EXEC: printf '\001_BC' | spicy-dump -d test.hlto -p Mini::X >>output2 2>&1 +# @TEST-EXEC: printf '\000_BC' | spicy-dump -d test.hlto -p Mini::X >>output2 2>&1 +# @TEST-EXEC: btest-diff output2 + +public type X = unit { + use_max_size: uint8 &convert=cast($$); + + switch (self.use_max_size) { + True -> xs: bytes &until=b"_" &max-size=1; + False -> xs: bytes &until=b"_"; + }; + + rest: bytes &eod; +};