Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve size checks #114

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Improve size checks #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9c846d
Remove support for partial parent derivation
hchataing Nov 8, 2024
68e6165
Implement analyzer checks for field size and offsets
hchataing Nov 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
235 changes: 235 additions & 0 deletions pdl-compiler/src/analyzer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ pub enum ErrorCode {
InvalidConditionValue = 48,
E49 = 49,
ReusedConditionIdentifier = 50,
InvalidFieldOffset = 51,
InvalidFieldSize = 52,
InvalidPacketSize = 53,
}

impl From<ErrorCode> for String {
Expand Down Expand Up @@ -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",
hchataing marked this conversation as resolved.
Show resolved Hide resolved
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(),
hchataing marked this conversation as resolved.
Show resolved Hide resolved
)
.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<File, Diagnostics> {
fn inline_fields<'a>(
Expand Down Expand Up @@ -1768,6 +1863,9 @@ pub fn analyze(file: &File) -> Result<File, Diagnostics> {
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)
}

Expand Down Expand Up @@ -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!(
Expand Down
44 changes: 0 additions & 44 deletions pdl-compiler/tests/canonical/le_test_file.pdl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,18 +54,6 @@ packet EmptyParent : ScalarParent {
_payload_
}

// Start: little_endian_only
hchataing marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading