Skip to content

[codegen] Special-case CFF INDEX struct size #1728

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

Open
behdad wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[codegen] Special-case CFF INDEX struct size #1728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 34 additions & 1 deletion font-codegen/src/table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,7 +998,40 @@ impl Table {
}

fn iter_field_validation_stmts(&self) -> impl Iterator<Item = TokenStream> + '_ {
self.fields.iter().map(Field::field_parse_validation_stmts)
let table_name = self.raw_name().to_string();
let is_index = table_name == "Index1" || table_name == "Index2";

@cmyr cmyr Feb 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a funny little case hm? In general we don't like to have this kind of code be actually defined in codegen. Currently we have special labels for the different special patterns for computing a count (such as the add_multiply, used elsewhere in Index1 and Index2 and so following that pattern we would have some annotation like, #[count(cff_index_data($offsets)], and maybe we would also introduce a type for the offsets themselves that would abstract the variable length so we could use a ComputedArray, which would simplify getting the last element... let me try putting that patch together as a first step.

@behdad behdad Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@cmyr cmyr Feb 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking about this more, is this patch motivated by a specific concern? it is true that the raw data array here will potentially include extra bytes belonging to another table, but given that we know the start/end offsets of the actual objects, is there any risk of us misinterpreting the data?

@behdad behdad Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well one issue that current code is incorrect is that if count=0, we shouldn't expect offSize at all.

As for consuming the rest of the bytes, I noticed it, because I'm working on VARC table rendering, which uses Index2 in a couple of different ways. So I wasn't sure if the current code will work for me.

@dfrg dfrg Feb 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just mark the count field as a version and then set the others to min version of 1. Hacky but should work?

@cmyr cmyr Feb 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a general concept of a field being conditional on something, so that's fine: the annoying bit is just that it means off_size would become Option<u8>.

self.fields.iter().map(move |field| {
if is_index && field.name == "data" {
let len_field_name = field.shape_byte_len_field_name();
quote! {
let #len_field_name = {
let count: usize = count as usize;
if count == 0 {
0usize
} else {
let off_size: usize = off_size as usize;
let offsets_end = cursor.position()?;
let last_offset_pos = offsets_end
.checked_sub(off_size)
.ok_or(ReadError::OutOfBounds)?;
let last_offset = match off_size {
1 => data.read_at::<u8>(last_offset_pos)? as usize,
2 => data.read_at::<u16>(last_offset_pos)? as usize,
3 => data.read_at::<Uint24>(last_offset_pos)?.to_u32() as usize,
4 => data.read_at::<u32>(last_offset_pos)? as usize,
_ => return Err(ReadError::OutOfBounds),
};
last_offset
.checked_sub(1)
.ok_or(ReadError::OutOfBounds)?
}
};
cursor.advance_by(#len_field_name);
}
} else {
field.field_parse_validation_stmts()
}
})
}

fn iter_table_ref_getters(&self) -> impl Iterator<Item = TokenStream> + '_ {
Expand Down
42 changes: 40 additions & 2 deletions read-fonts/generated/generated_postscript.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,26 @@ impl<'a> FontRead<'a> for Index1<'a> {
.checked_mul(u8::RAW_BYTE_LEN)
.ok_or(ReadError::OutOfBounds)?;
cursor.advance_by(offsets_byte_len);
let data_byte_len = cursor.remaining_bytes() / u8::RAW_BYTE_LEN * u8::RAW_BYTE_LEN;
let data_byte_len = {
let count: usize = count as usize;
if count == 0 {
0usize
} else {
let off_size: usize = off_size as usize;
let offsets_end = cursor.position()?;
let last_offset_pos = offsets_end
.checked_sub(off_size)
.ok_or(ReadError::OutOfBounds)?;
let last_offset = match off_size {
1 => data.read_at::<u8>(last_offset_pos)? as usize,
2 => data.read_at::<u16>(last_offset_pos)? as usize,
3 => data.read_at::<Uint24>(last_offset_pos)?.to_u32() as usize,
4 => data.read_at::<u32>(last_offset_pos)? as usize,
_ => return Err(ReadError::OutOfBounds),
};
last_offset.checked_sub(1).ok_or(ReadError::OutOfBounds)?
}
};
cursor.advance_by(data_byte_len);
cursor.finish(Index1Marker {
offsets_byte_len,
Expand Down Expand Up @@ -158,7 +177,26 @@ impl<'a> FontRead<'a> for Index2<'a> {
.checked_mul(u8::RAW_BYTE_LEN)
.ok_or(ReadError::OutOfBounds)?;
cursor.advance_by(offsets_byte_len);
let data_byte_len = cursor.remaining_bytes() / u8::RAW_BYTE_LEN * u8::RAW_BYTE_LEN;
let data_byte_len = {
let count: usize = count as usize;
if count == 0 {
0usize
} else {
let off_size: usize = off_size as usize;
let offsets_end = cursor.position()?;
let last_offset_pos = offsets_end
.checked_sub(off_size)
.ok_or(ReadError::OutOfBounds)?;
let last_offset = match off_size {
1 => data.read_at::<u8>(last_offset_pos)? as usize,
2 => data.read_at::<u16>(last_offset_pos)? as usize,
3 => data.read_at::<Uint24>(last_offset_pos)?.to_u32() as usize,
4 => data.read_at::<u32>(last_offset_pos)? as usize,
_ => return Err(ReadError::OutOfBounds),
};
last_offset.checked_sub(1).ok_or(ReadError::OutOfBounds)?
}
};
cursor.advance_by(data_byte_len);
cursor.finish(Index2Marker {
offsets_byte_len,
Expand Down
Loading