Skip to content

Commit 3764732

Browse files
committed
fix(emacs-core): type byte-code structural keys
GNU Emacs compares PVEC_CLOSURE objects element-wise and requires structurally equal closures to produce the same sxhash. The PR's shared structural view established that contract, but its equal-table projection encoded an absent field as the ordinary text key #<absent>. A byte-code object with a present Lisp string of that value therefore aliased an unequal object whose environment was absent. Represent byte-code table keys as a dedicated HashKey::ByteCode containing domain-tagged ByteCodeKeyPart values. Preserve observable shape, Lisp values, retained bytes, decoded operations, constant vectors, GNU string contents, and absence as distinct enum variants, and teach the pdump conversion and object codec to round-trip that typed representation. Replace the allocating structural Vec with an ExactSizeIterator over a fixed array plus extra slots. Derive Eq and Hash for decoded Op values so sxhash no longer allocates a debug-formatted string, and keep observable closure shape in the same structural evidence stream consumed by equality, sxhash, and equal-table keys. Add a red-first regression proving absent state cannot collide with Lisp-reachable sentinel text, correct the tests to honor the one-way sxhash contract instead of demanding distinct hashes for unequal values, and add a GNU 31.0.90 differential oracle covering byte-code objects, make-closure, equal tables, and delete. Verified with release-mode neovm-core checks, seven focused release tests including pdump codec round-trip, and the live release differential oracle against the pinned GNU Emacs 31.0.90 binary.
1 parent dab1bf0 commit 3764732

10 files changed

Lines changed: 480 additions & 118 deletions

File tree

‎crates/neovm-core/src/emacs_core/runtime/bytecode/chunk.rs‎

Lines changed: 77 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -122,9 +122,18 @@ fn arglist_value_from_params(params: &LambdaParams) -> Value {
122122
Value::list(elements)
123123
}
124124

125-
/// One element of [`ByteCodeFunction::structural_slots`].
125+
/// One piece of evidence used for structural byte-code equality and hashing.
126+
///
127+
/// This is deliberately named a "part", not a GNU closure "slot": Neomacs
128+
/// keeps some execution state (notably the captured environment) beside the
129+
/// GNU-compatible slot projection. Consumers must compare that state too,
130+
/// because it affects execution even when it has no independently observable
131+
/// GNU slot.
126132
#[derive(Clone, Copy, Debug)]
127-
pub enum ByteCodeSlot<'a> {
133+
pub enum ByteCodeStructuralPart<'a> {
134+
/// GNU's observable pseudovector size. Keeping shape in the sequence
135+
/// makes it impossible for a consumer to forget the `ASIZE` comparison.
136+
ObservableSlotCount(usize),
128137
/// A Lisp value compared, hashed and keyed as any other.
129138
Value(Value),
130139
/// The GNU bytecode string, byte-exact.
@@ -135,10 +144,37 @@ pub enum ByteCodeSlot<'a> {
135144
Values(&'a [Value]),
136145
/// A docstring, under GNU string equality: characters, bytes, contents.
137146
Text(&'a LispString),
138-
/// An optional slot that is not there.
147+
/// An optional structural part that is not present.
139148
Absent,
140149
}
141150

151+
/// Allocation-free iterator over a byte-code function's structural evidence.
152+
pub(crate) struct ByteCodeStructuralParts<'a> {
153+
fixed: std::array::IntoIter<ByteCodeStructuralPart<'a>, 8>,
154+
extras: std::slice::Iter<'a, Value>,
155+
}
156+
157+
impl<'a> Iterator for ByteCodeStructuralParts<'a> {
158+
type Item = ByteCodeStructuralPart<'a>;
159+
160+
fn next(&mut self) -> Option<Self::Item> {
161+
self.fixed.next().or_else(|| {
162+
self.extras
163+
.next()
164+
.copied()
165+
.map(ByteCodeStructuralPart::Value)
166+
})
167+
}
168+
169+
fn size_hint(&self) -> (usize, Option<usize>) {
170+
let remaining = self.fixed.len() + self.extras.len();
171+
(remaining, Some(remaining))
172+
}
173+
}
174+
175+
impl ExactSizeIterator for ByteCodeStructuralParts<'_> {}
176+
impl std::iter::FusedIterator for ByteCodeStructuralParts<'_> {}
177+
142178
/// A compiled bytecode function.
143179
#[derive(Debug)]
144180
pub struct ByteCodeFunction {
@@ -634,9 +670,9 @@ impl ByteCodeFunction {
634670
}
635671
}
636672

637-
/// The structural view of this function: GNU's closure slots, in GNU's
638-
/// order, plus the one field GNU keeps inside a slot and this port keeps
639-
/// beside it.
673+
/// The structural evidence for this function: GNU's observable shape and
674+
/// closure slots, in GNU's order, plus the environment this port keeps
675+
/// beside the constants vector.
640676
///
641677
/// GNU's `internal_equal_1` compares a `PVEC_CLOSURE` element-wise
642678
/// (src/fns.c:2984-2998 in emacs-31.1) and `sxhash_obj` hashes it through
@@ -645,46 +681,48 @@ impl ByteCodeFunction {
645681
/// `equal`, its fallible twin, `sxhash-equal` and the `equal`-table key
646682
/// all read THIS sequence rather than each re-deriving the schema:
647683
///
648-
/// | index | GNU slot | here |
649-
/// |-------|---------------------|----------------------------------------|
650-
/// | 0 | arglist | `Value` |
651-
/// | 1 | bytecode string | `Bytes`, or `Ops` when none is retained|
652-
/// | 2 | constants vector | `Values` |
653-
/// | 3 | (inside slot 2) | captured `env`: `Value`, else `Absent` |
654-
/// | 4 | max stack depth | `Value` (a fixnum) |
655-
/// | 5 | docstring / form | `Text`, `Value`, or `Absent` |
656-
/// | 6 | interactive spec | `Value` or `Absent` |
657-
/// | 7.. | `&rest ELEMENTS` | `Value` each |
684+
/// | part | GNU representation | Neomacs structural evidence |
685+
/// |------|---------------------|------------------------------------------|
686+
/// | 0 | pseudovector ASIZE | `ObservableSlotCount` |
687+
/// | 1 | arglist | `Value` |
688+
/// | 2 | bytecode string | `Bytes`, or `Ops` when none is retained |
689+
/// | 3 | constants vector | `Values` |
690+
/// | 4 | inside constants | captured `env`: `Value`, else `Absent` |
691+
/// | 5 | max stack depth | `Value` (a fixnum) |
692+
/// | 6 | docstring / form | `Text`, `Value`, or `Absent` |
693+
/// | 7 | interactive spec | `Value` or `Absent` |
694+
/// | 8.. | `&rest ELEMENTS` | `Value` each |
658695
///
659696
/// `Absent` is distinct from a present `nil`: `Op::MakeClosure` stores
660697
/// `Some(lexenv)`, and a closure over no variables has a present, `nil`
661698
/// environment that `(aref FN 2)` reports as `nil` rather than as the
662699
/// constants vector. A function with no retained GNU bytes compares its
663700
/// decoded instructions instead, so two functions with different code are
664701
/// never equal even though GNU's slot 1 would be `nil` for both.
665-
pub(crate) fn structural_slots(&self) -> Vec<ByteCodeSlot<'_>> {
666-
let mut slots = Vec::with_capacity(7 + self.extra_slots.len());
667-
slots.push(ByteCodeSlot::Value(self.arglist));
668-
slots.push(match &self.gnu_bytecode_bytes {
669-
Some(bytes) => ByteCodeSlot::Bytes(bytes.as_slice()),
670-
None => ByteCodeSlot::Ops(&self.ops),
671-
});
672-
slots.push(ByteCodeSlot::Values(self.constants.as_slice()));
673-
slots.push(self.env.map_or(ByteCodeSlot::Absent, ByteCodeSlot::Value));
674-
slots.push(ByteCodeSlot::Value(Value::fixnum(i64::from(
675-
self.max_stack,
676-
))));
677-
slots.push(match (self.doc_form, &self.docstring) {
678-
(Some(form), _) => ByteCodeSlot::Value(form),
679-
(None, Some(doc)) => ByteCodeSlot::Text(doc),
680-
(None, None) => ByteCodeSlot::Absent,
681-
});
682-
slots.push(
683-
self.interactive
684-
.map_or(ByteCodeSlot::Absent, ByteCodeSlot::Value),
685-
);
686-
slots.extend(self.extra_slots.iter().copied().map(ByteCodeSlot::Value));
687-
slots
702+
pub(crate) fn structural_parts(&self) -> ByteCodeStructuralParts<'_> {
703+
use ByteCodeStructuralPart as Part;
704+
705+
ByteCodeStructuralParts {
706+
fixed: [
707+
Part::ObservableSlotCount(self.observable_closure_slot_count()),
708+
Part::Value(self.arglist),
709+
match &self.gnu_bytecode_bytes {
710+
Some(bytes) => Part::Bytes(bytes.as_slice()),
711+
None => Part::Ops(&self.ops),
712+
},
713+
Part::Values(self.constants.as_slice()),
714+
self.env.map_or(Part::Absent, Part::Value),
715+
Part::Value(Value::fixnum(i64::from(self.max_stack))),
716+
match (self.doc_form, &self.docstring) {
717+
(Some(form), _) => Part::Value(form),
718+
(None, Some(doc)) => Part::Text(doc),
719+
(None, None) => Part::Absent,
720+
},
721+
self.interactive.map_or(Part::Absent, Part::Value),
722+
]
723+
.into_iter(),
724+
extras: self.extra_slots.iter(),
725+
}
688726
}
689727

690728
pub fn observable_closure_slot_count(&self) -> usize {

‎crates/neovm-core/src/emacs_core/runtime/bytecode/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,6 @@ pub mod vm;
1313

1414
// Re-export main types
1515
pub(crate) use chunk::fresh_bytecode_source_id;
16-
pub use chunk::{ByteCodeFunction, ByteCodeSlot};
16+
pub use chunk::{ByteCodeFunction, ByteCodeStructuralPart};
1717
pub use opcode::Op;
1818
pub use vm::Vm;

‎crates/neovm-core/src/emacs_core/runtime/bytecode/opcode.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
use serde::{Deserialize, Serialize};
88

99
/// A single bytecode instruction.
10-
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
10+
#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
1111
pub enum Op {
1212
// -- Constants and stack --------------------------------------------------
1313
/// Push a constant from the constant pool.

‎crates/neovm-core/src/emacs_core/runtime/hashtab/mod.rs‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ fn hash_key_to_value(key: &HashKey) -> Value {
7373
let vals: Vec<Value> = items.iter().map(hash_key_to_value).collect();
7474
Value::vector(vals)
7575
}
76+
HashKey::ByteCode(_) => Value::NIL,
7677
HashKey::Marker(_) | HashKey::Overlay(_) => Value::NIL,
7778
HashKey::BoolVec(parts) => {
7879
let (len, bits) = **parts;
@@ -251,21 +252,32 @@ fn emacs_sxhash_overlay(value: &Value, depth: usize) -> Option<u64> {
251252
/// GNU `sxhash_obj` (src/fns.c:5525-5536) sends a `PVEC_CLOSURE` through
252253
/// `sxhash_vector` like any pseudovector `internal_equal` walks, so two
253254
/// `equal` byte-code objects hash alike and share an `equal` hash-table
254-
/// bucket. The elements are [`ByteCodeFunction::structural_slots`] -- the
255+
/// bucket. The elements are [`ByteCodeFunction::structural_parts`] -- the
255256
/// same sequence `equal` compares, so everything `equal` distinguishes,
256257
/// including a captured environment beside the constants, moves the hash --
257258
/// folded like `sxhash_vector`: seeded with the slot count, at most
258259
/// `SXHASH_MAX_LEN` elements.
259260
fn emacs_sxhash_bytecode(value: &Value, depth: usize) -> Option<u64> {
260-
use crate::emacs_core::bytecode::ByteCodeSlot;
261+
use crate::emacs_core::bytecode::ByteCodeStructuralPart as Part;
261262
let bc = value.get_bytecode_data()?;
262-
let mut hash = bc.observable_closure_slot_count() as u64;
263-
for slot in bc.structural_slots().into_iter().take(SXHASH_MAX_LEN) {
264-
let slot_hash = match slot {
265-
ByteCodeSlot::Value(value) => emacs_sxhash_obj_with_fallback(&value, depth + 1),
266-
ByteCodeSlot::Bytes(bytes) => emacs_hash_char_array(bytes),
267-
ByteCodeSlot::Ops(ops) => emacs_hash_char_array(format!("{ops:?}").as_bytes()),
268-
ByteCodeSlot::Values(values) => {
263+
let mut parts = bc.structural_parts();
264+
let Part::ObservableSlotCount(slot_count) = parts.next()? else {
265+
unreachable!("byte-code structure starts with its observable shape")
266+
};
267+
let mut hash = slot_count as u64;
268+
for part in parts.take(SXHASH_MAX_LEN) {
269+
let part_hash = match part {
270+
Part::ObservableSlotCount(_) => {
271+
unreachable!("byte-code structure contains one leading shape")
272+
}
273+
Part::Value(value) => emacs_sxhash_obj_with_fallback(&value, depth + 1),
274+
Part::Bytes(bytes) => emacs_hash_char_array(bytes),
275+
Part::Ops(ops) => {
276+
let mut hasher = DefaultHasher::new();
277+
ops.hash(&mut hasher);
278+
hasher.finish() & SXHASH_FALLBACK_NONNEG_MASK
279+
}
280+
Part::Values(values) => {
269281
let mut vector_hash = values.len() as u64;
270282
for value in values.iter().take(SXHASH_MAX_LEN) {
271283
vector_hash = sxhash_combine(
@@ -277,10 +289,10 @@ fn emacs_sxhash_bytecode(value: &Value, depth: usize) -> Option<u64> {
277289
}
278290
// GNU `sxhash_string` hashes the bytes; equal strings have equal
279291
// bytes whatever their representation.
280-
ByteCodeSlot::Text(text) => emacs_hash_char_array(text.as_bytes()),
281-
ByteCodeSlot::Absent => 42,
292+
Part::Text(text) => emacs_hash_char_array(text.as_bytes()),
293+
Part::Absent => 42,
282294
};
283-
hash = sxhash_combine(hash, slot_hash);
295+
hash = sxhash_combine(hash, part_hash);
284296
}
285297
Some(hash)
286298
}

‎crates/neovm-core/src/emacs_core/runtime/pdump/convert.rs‎

Lines changed: 103 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,8 @@ use crate::emacs_core::rect::RectangleState;
5151
use crate::emacs_core::register::{RegisterContent, RegisterManager};
5252
use crate::emacs_core::symbol::{LispSymbol, Obarray, SymbolTrappedWrite};
5353
use crate::emacs_core::value::{
54-
HashKey, HashTableTest, HashTableWeakness, LambdaParams, LispHashTable, RuntimeBindingValue,
55-
StringTextPropertyRun, Value,
54+
ByteCodeKeyPart, HashKey, HashTableTest, HashTableWeakness, LambdaParams, LispHashTable,
55+
RuntimeBindingValue, StringTextPropertyRun, Value,
5656
};
5757
use crate::emacs_core::value::{ValueKind, VecLikeType};
5858
use crate::emacs_core::value::{
@@ -3046,6 +3046,12 @@ pub(crate) fn dump_hash_key(encoder: &mut DumpEncoder, k: &HashKey) -> DumpHashK
30463046
HashKey::EqualVec(v) => {
30473047
DumpHashKey::EqualVec(v.iter().map(|key| dump_hash_key(encoder, key)).collect())
30483048
}
3049+
HashKey::ByteCode(parts) => DumpHashKey::ByteCode(
3050+
parts
3051+
.iter()
3052+
.map(|part| dump_byte_code_key_part(encoder, part))
3053+
.collect(),
3054+
),
30493055
HashKey::Marker(parts) => DumpHashKey::Marker(parts.0, parts.1.get()),
30503056
HashKey::Overlay(parts) => DumpHashKey::Overlay {
30513057
buffer: parts.0,
@@ -3066,6 +3072,31 @@ pub(crate) fn dump_hash_key(encoder: &mut DumpEncoder, k: &HashKey) -> DumpHashK
30663072
}
30673073
}
30683074

3075+
fn dump_byte_code_key_part(
3076+
encoder: &mut DumpEncoder,
3077+
part: &ByteCodeKeyPart,
3078+
) -> DumpByteCodeKeyPart {
3079+
match part {
3080+
ByteCodeKeyPart::ObservableSlotCount(count) => {
3081+
DumpByteCodeKeyPart::ObservableSlotCount(*count)
3082+
}
3083+
ByteCodeKeyPart::Value(value) => DumpByteCodeKeyPart::Value(dump_hash_key(encoder, value)),
3084+
ByteCodeKeyPart::Bytes(bytes) => DumpByteCodeKeyPart::Bytes(bytes.to_vec()),
3085+
ByteCodeKeyPart::Ops(ops) => DumpByteCodeKeyPart::Ops(ops.to_vec()),
3086+
ByteCodeKeyPart::Values(values) => DumpByteCodeKeyPart::Values(
3087+
values
3088+
.iter()
3089+
.map(|value| dump_hash_key(encoder, value))
3090+
.collect(),
3091+
),
3092+
ByteCodeKeyPart::Text { char_count, bytes } => DumpByteCodeKeyPart::Text {
3093+
char_count: *char_count,
3094+
bytes: bytes.to_vec(),
3095+
},
3096+
ByteCodeKeyPart::Absent => DumpByteCodeKeyPart::Absent,
3097+
}
3098+
}
3099+
30693100
pub(crate) fn dump_hash_table_test(t: &HashTableTest) -> DumpHashTableTest {
30703101
match t {
30713102
HashTableTest::Eq => DumpHashTableTest::Eq,
@@ -4701,6 +4732,13 @@ pub(crate) fn load_hash_key(decoder: &mut LoadDecoder, k: &DumpHashKey) -> HashK
47014732
.collect::<Vec<_>>()
47024733
.into_boxed_slice(),
47034734
),
4735+
DumpHashKey::ByteCode(parts) => HashKey::ByteCode(
4736+
parts
4737+
.iter()
4738+
.map(|part| load_byte_code_key_part(decoder, part))
4739+
.collect::<Vec<_>>()
4740+
.into_boxed_slice(),
4741+
),
47044742
DumpHashKey::Marker(buffer, bytepos) => {
47054743
HashKey::Marker(Box::new((*buffer, EmacsBytePos::new(*bytepos))))
47064744
}
@@ -4725,6 +4763,34 @@ pub(crate) fn load_hash_key(decoder: &mut LoadDecoder, k: &DumpHashKey) -> HashK
47254763
}
47264764
}
47274765

4766+
fn load_byte_code_key_part(
4767+
decoder: &mut LoadDecoder,
4768+
part: &DumpByteCodeKeyPart,
4769+
) -> ByteCodeKeyPart {
4770+
match part {
4771+
DumpByteCodeKeyPart::ObservableSlotCount(count) => {
4772+
ByteCodeKeyPart::ObservableSlotCount(*count)
4773+
}
4774+
DumpByteCodeKeyPart::Value(value) => ByteCodeKeyPart::Value(load_hash_key(decoder, value)),
4775+
DumpByteCodeKeyPart::Bytes(bytes) => {
4776+
ByteCodeKeyPart::Bytes(bytes.clone().into_boxed_slice())
4777+
}
4778+
DumpByteCodeKeyPart::Ops(ops) => ByteCodeKeyPart::Ops(ops.clone().into_boxed_slice()),
4779+
DumpByteCodeKeyPart::Values(values) => ByteCodeKeyPart::Values(
4780+
values
4781+
.iter()
4782+
.map(|value| load_hash_key(decoder, value))
4783+
.collect::<Vec<_>>()
4784+
.into_boxed_slice(),
4785+
),
4786+
DumpByteCodeKeyPart::Text { char_count, bytes } => ByteCodeKeyPart::Text {
4787+
char_count: *char_count,
4788+
bytes: bytes.clone().into_boxed_slice(),
4789+
},
4790+
DumpByteCodeKeyPart::Absent => ByteCodeKeyPart::Absent,
4791+
}
4792+
}
4793+
47284794
fn load_hash_key_owned(decoder: &mut LoadDecoder, k: DumpHashKey) -> HashKey {
47294795
match k {
47304796
DumpHashKey::Nil => HashKey::Nil,
@@ -4755,6 +4821,13 @@ fn load_hash_key_owned(decoder: &mut LoadDecoder, k: DumpHashKey) -> HashKey {
47554821
.collect::<Vec<_>>()
47564822
.into_boxed_slice(),
47574823
),
4824+
DumpHashKey::ByteCode(parts) => HashKey::ByteCode(
4825+
parts
4826+
.into_iter()
4827+
.map(|part| load_byte_code_key_part_owned(decoder, part))
4828+
.collect::<Vec<_>>()
4829+
.into_boxed_slice(),
4830+
),
47584831
DumpHashKey::Marker(buffer, bytepos) => {
47594832
HashKey::Marker(Box::new((buffer, EmacsBytePos::new(bytepos))))
47604833
}
@@ -4779,6 +4852,34 @@ fn load_hash_key_owned(decoder: &mut LoadDecoder, k: DumpHashKey) -> HashKey {
47794852
}
47804853
}
47814854

4855+
fn load_byte_code_key_part_owned(
4856+
decoder: &mut LoadDecoder,
4857+
part: DumpByteCodeKeyPart,
4858+
) -> ByteCodeKeyPart {
4859+
match part {
4860+
DumpByteCodeKeyPart::ObservableSlotCount(count) => {
4861+
ByteCodeKeyPart::ObservableSlotCount(count)
4862+
}
4863+
DumpByteCodeKeyPart::Value(value) => {
4864+
ByteCodeKeyPart::Value(load_hash_key_owned(decoder, value))
4865+
}
4866+
DumpByteCodeKeyPart::Bytes(bytes) => ByteCodeKeyPart::Bytes(bytes.into_boxed_slice()),
4867+
DumpByteCodeKeyPart::Ops(ops) => ByteCodeKeyPart::Ops(ops.into_boxed_slice()),
4868+
DumpByteCodeKeyPart::Values(values) => ByteCodeKeyPart::Values(
4869+
values
4870+
.into_iter()
4871+
.map(|value| load_hash_key_owned(decoder, value))
4872+
.collect::<Vec<_>>()
4873+
.into_boxed_slice(),
4874+
),
4875+
DumpByteCodeKeyPart::Text { char_count, bytes } => ByteCodeKeyPart::Text {
4876+
char_count,
4877+
bytes: bytes.into_boxed_slice(),
4878+
},
4879+
DumpByteCodeKeyPart::Absent => ByteCodeKeyPart::Absent,
4880+
}
4881+
}
4882+
47824883
pub(crate) fn load_hash_table_test(t: &DumpHashTableTest) -> HashTableTest {
47834884
match t {
47844885
DumpHashTableTest::Eq => HashTableTest::Eq,

0 commit comments

Comments
 (0)