From e9c846d1dc7ca81e8645937020f49e9d6dbb00bd Mon Sep 17 00:00:00 2001 From: Henri Chataing Date: Thu, 7 Nov 2024 18:19:26 -0800 Subject: [PATCH 1/2] Remove support for partial parent derivation The following pattern is currently only supported by the Python generator, and will no longer be officially supported going forward ``` struct Parent { a : 1, // no an octet size _payload_, } struct Child : Parent (a = 0) { b : 7, // other fields } ``` --- pdl-compiler/tests/canonical/le_test_file.pdl | 44 -------- .../tests/canonical/le_test_vectors.json | 106 ------------------ 2 files changed, 150 deletions(-) diff --git a/pdl-compiler/tests/canonical/le_test_file.pdl b/pdl-compiler/tests/canonical/le_test_file.pdl index 86f9c86..89ec89c 100644 --- a/pdl-compiler/tests/canonical/le_test_file.pdl +++ b/pdl-compiler/tests/canonical/le_test_file.pdl @@ -54,18 +54,6 @@ packet EmptyParent : ScalarParent { _payload_ } -// Start: little_endian_only -packet PartialParent5 { - a: 5, - _payload_ -} - -packet PartialParent12 { - a: 12, - _payload_ -} -// End: little_endian_only - // Packet bit fields // The parser must be able to handle bit fields with scalar values @@ -458,38 +446,6 @@ packet AliasedChild_B : EmptyParent (a = 3) { c: 16, } -// Start: little_endian_only - -// The parser must handle inheritance of packets with payloads starting -// on a shifted byte boundary, as long as the first fields of the child -// complete the bit fields. -packet PartialChild5_A : PartialParent5 (a = 0) { - b: 11, -} - -// The parser must handle inheritance of packets with payloads starting -// on a shifted byte boundary, as long as the first fields of the child -// complete the bit fields. -packet PartialChild5_B : PartialParent5 (a = 1) { - c: 27, -} - -// The parser must handle inheritance of packets with payloads starting -// on a shifted byte boundary, as long as the first fields of the child -// complete the bit fields. -packet PartialChild12_A : PartialParent12 (a = 2) { - d: 4, -} - -// The parser must handle inheritance of packets with payloads starting -// on a shifted byte boundary, as long as the first fields of the child -// complete the bit fields. -packet PartialChild12_B : PartialParent12 (a = 3) { - e: 20, -} - -// End: little_endian_only - // Struct bit fields // The parser must be able to handle bit fields with scalar values diff --git a/pdl-compiler/tests/canonical/le_test_vectors.json b/pdl-compiler/tests/canonical/le_test_vectors.json index 287c664..89e8b89 100644 --- a/pdl-compiler/tests/canonical/le_test_vectors.json +++ b/pdl-compiler/tests/canonical/le_test_vectors.json @@ -2790,112 +2790,6 @@ } ] }, - { - "packet": "PartialParent5", - "tests": [ - { - "packed": "0000", - "unpacked": { - "a": 0, - "b": 0 - }, - "packet": "PartialChild5_A" - }, - { - "packed": "e0ff", - "unpacked": { - "a": 0, - "b": 2047 - }, - "packet": "PartialChild5_A" - }, - { - "packed": "0081", - "unpacked": { - "a": 0, - "b": 1032 - }, - "packet": "PartialChild5_A" - }, - { - "packed": "01000000", - "unpacked": { - "a": 1, - "c": 0 - }, - "packet": "PartialChild5_B" - }, - { - "packed": "e1ffffff", - "unpacked": { - "a": 1, - "c": 134217727 - }, - "packet": "PartialChild5_B" - }, - { - "packed": "c1a262a2", - "unpacked": { - "a": 1, - "c": 85136662 - }, - "packet": "PartialChild5_B" - } - ] - }, - { - "packet": "PartialParent12", - "tests": [ - { - "packed": "0200", - "unpacked": { - "a": 2, - "d": 0 - }, - "packet": "PartialChild12_A" - }, - { - "packed": "02f0", - "unpacked": { - "a": 2, - "d": 15 - }, - "packet": "PartialChild12_A" - }, - { - "packed": "0260", - "unpacked": { - "a": 2, - "d": 6 - }, - "packet": "PartialChild12_A" - }, - { - "packed": "03000000", - "unpacked": { - "a": 3, - "e": 0 - }, - "packet": "PartialChild12_B" - }, - { - "packed": "03f0ffff", - "unpacked": { - "a": 3, - "e": 1048575 - }, - "packet": "PartialChild12_B" - }, - { - "packed": "03d0b191", - "unpacked": { - "a": 3, - "e": 596765 - }, - "packet": "PartialChild12_B" - } - ] - }, { "packet": "Struct_Enum_Field", "tests": [ From 68e61651b0b47ba2d3f71c160ab09bc23ca28425 Mon Sep 17 00:00:00 2001 From: Henri Chataing Date: Fri, 10 Nov 2023 17:16:10 -0800 Subject: [PATCH 2/2] Implement analyzer checks for field size and offsets Validate that non-bitfield fields do start on an octet boundary, and that packet and structs have a size that is an integral number of octets. Fixes #33 --- pdl-compiler/src/analyzer.rs | 235 +++++++++++++++++++++++++++++++++++ 1 file changed, 235 insertions(+) diff --git a/pdl-compiler/src/analyzer.rs b/pdl-compiler/src/analyzer.rs index 90ed239..9f61453 100644 --- a/pdl-compiler/src/analyzer.rs +++ b/pdl-compiler/src/analyzer.rs @@ -138,6 +138,9 @@ pub enum ErrorCode { InvalidConditionValue = 48, E49 = 49, ReusedConditionIdentifier = 50, + InvalidFieldOffset = 51, + InvalidFieldSize = 52, + InvalidPacketSize = 53, } impl From for String { @@ -1618,6 +1621,98 @@ fn check_optional_fields(file: &File) -> Result<(), Diagnostics> { diagnostics.err_or(()) } +/// Check field offsets. +/// Raises error diagnostics for the following cases: +/// - non bit-field field not aligned to a octet boundary +fn check_field_offsets(file: &File, scope: &Scope, schema: &Schema) -> Result<(), Diagnostics> { + let mut diagnostics: Diagnostics = Default::default(); + for decl in &file.declarations { + let mut offset = 0; + + for field in decl.fields() { + match &field.desc { + FieldDesc::Typedef { type_id, .. } + if matches!( + scope.typedef.get(type_id), + Some(Decl { desc: DeclDesc::Enum { .. }, .. }) + ) => {} + FieldDesc::Payload { .. } + | FieldDesc::Body { .. } + | FieldDesc::Typedef { .. } + | FieldDesc::Array { .. } + | FieldDesc::Padding { .. } + | FieldDesc::Checksum { .. } => { + if offset % 8 != 0 { + diagnostics.push( + Diagnostic::error() + .with_code(ErrorCode::InvalidFieldOffset) + .with_message(format!( + "{} field is not aligned to an octet boundary", + field.kind() + )) + .with_labels(vec![field.loc.primary()]), + ) + } + } + FieldDesc::Size { .. } + | FieldDesc::Count { .. } + | FieldDesc::ElementSize { .. } + | FieldDesc::FixedEnum { .. } + | FieldDesc::FixedScalar { .. } + | FieldDesc::Group { .. } + | FieldDesc::Flag { .. } + | FieldDesc::Reserved { .. } + | FieldDesc::Scalar { .. } => (), + } + offset = match schema.field_size[&field.key] { + Size::Static(size) => offset + size, + Size::Dynamic | Size::Unknown => 0, + }; + } + } + diagnostics.err_or(()) +} + +/// Check field sizes. +/// Raises error diagnostics for the following cases: +/// - struct size is not an integral number of octets +/// - packet size is not an integral number of octets +/// - scalar array element size is not an integral number of octets +fn check_decl_sizes(file: &File, schema: &Schema) -> Result<(), Diagnostics> { + let mut diagnostics: Diagnostics = Default::default(); + for decl in &file.declarations { + let mut static_size = 0; + + for field in decl.fields() { + match &field.desc { + FieldDesc::Array { width: Some(width), .. } if width % 8 != 0 => diagnostics.push( + Diagnostic::error() + .with_code(ErrorCode::InvalidFieldSize) + .with_message( + "array element size is not an integral number of octets".to_owned(), + ) + .with_labels(vec![field.loc.primary()]), + ), + _ => (), + } + static_size += schema.field_size[&field.key].static_().unwrap_or(0); + } + + if static_size % 8 != 0 { + diagnostics.push( + Diagnostic::error() + .with_code(ErrorCode::InvalidPacketSize) + .with_message(format!( + "{} size is not an integral number of octets", + decl.kind() + )) + .with_labels(vec![decl.loc.primary()]), + ) + } + } + diagnostics.err_or(()) +} + /// Inline group fields and remove group declarations. fn inline_groups(file: &File) -> Result { fn inline_fields<'a>( @@ -1768,6 +1863,9 @@ pub fn analyze(file: &File) -> Result { desugar_flags(&mut file); let scope = Scope::new(&file)?; check_decl_constraints(&file, &scope)?; + let schema = Schema::new(&file); + check_field_offsets(&file, &scope, &schema)?; + check_decl_sizes(&file, &schema)?; Ok(file) } @@ -3002,6 +3100,143 @@ mod test { ); } + #[test] + fn test_e51() { + raises!( + InvalidFieldOffset, + r#" + little_endian_packets + struct S { a: 8 } + packet A { + a : 1, + s : S, + c : 7, + } + "# + ); + + raises!( + InvalidFieldOffset, + r#" + little_endian_packets + packet A { + a : 1, + b : 8[], + c : 7, + } + "# + ); + + raises!( + InvalidFieldOffset, + r#" + big_endian_packets + packet A { + a : 1, + _payload_, + b : 7, + } + "# + ); + + raises!( + InvalidFieldOffset, + r#" + big_endian_packets + packet A { + a : 1, + _body_, + b : 7, + } + "# + ); + + raises!( + InvalidFieldOffset, + r#" + little_endian_packets + custom_field F : 8 "f" + packet A { + a : 1, + f : F, + } + "# + ); + } + + #[test] + fn test_e52() { + raises!( + InvalidPacketSize, + r#" + little_endian_packets + packet A { + a : 1, + } + "# + ); + + raises!( + InvalidPacketSize, + r#" + little_endian_packets + packet A { + a : 8[], + b : 1, + } + "# + ); + + raises!( + InvalidPacketSize, + r#" + little_endian_packets + packet A { + a : 8[], + b : 1, + } + "# + ); + + raises!( + InvalidPacketSize, + r#" + little_endian_packets + struct S { + _size_(_payload_) : 8, + _payload_, + } + packet A { + a : S, + b : 1, + } + "# + ); + + raises!( + InvalidPacketSize, + r#" + little_endian_packets + struct A { + a : 1, + } + "# + ); + } + + #[test] + fn test_e53() { + raises!( + InvalidFieldSize, + r#" + little_endian_packets + packet A { + a : 12[], + } + "# + ); + } + #[test] fn test_enum_declaration() { valid!(