[assembler] bugfix: allow forward references to origin names.

youngman-g · youngman-g · commit 4a7420aff0eb · 2025-08-22T15:39:44.000+01:00
diff --git a/assembler/src/asmlib/ast.rs b/assembler/src/asmlib/ast.rs
@@ -1281,27 +1281,6 @@ impl Origin {
         Address::new(u18!(0o200_000))
     }
 
-    pub(crate) fn has_same_specification(&self, that: &Origin) -> bool {
-        use Origin::*;
-        if self.span() == that.span() {
-            match (self, that) {
-                (Literal(_, a1), Literal(_, a2)) if a1 == a2 => true,
-                (Symbolic(_, n1), Symbolic(_, n2)) if n1 == n2 => true,
-                (Deduced(_, n1, a1), Deduced(_, n2, a2)) if n1 == n2 && a1 == a2 => true,
-                (Deduced(_, n1, _), Symbolic(_, n2)) | (Symbolic(_, n1), Deduced(_, n2, _))
-                    if n1 == n2 =>
-                {
-                    // Only one of the two origins has an address, so
-                    // that does not feature in the comparison.
-                    true
-                }
-                _ => false,
-            }
-        } else {
-            false
-        }
-    }
-
     pub(super) fn symbol_uses(
         &self,
         block_id: BlockIdentifier,
diff --git a/assembler/src/asmlib/eval.rs b/assembler/src/asmlib/eval.rs
@@ -9,6 +9,8 @@ use base::{
     prelude::{Address, IndexBy, Unsigned18Bit, Unsigned36Bit},
     subword,
 };
+#[cfg(test)]
+use base::{u18, u36};
 
 use super::ast::*;
 use super::collections::OneOrMore;
@@ -17,6 +19,8 @@ use super::memorymap::{
 };
 use super::source::Source;
 use super::span::*;
+#[cfg(test)]
+use super::symbol::AddressUse;
 use super::symbol::{ConfigUse, IndexUse, OriginUse, SymbolName};
 
 use super::types::{AssemblerFailure, BlockIdentifier, MachineLimitExceededFailure, ProgramError};
@@ -212,6 +216,7 @@ impl<R: RcUpdater> EvaluationContext<'_, R> {
         match existing_def {
             ImplicitDefinition::DefaultAssigned(value, _) => Ok(*value),
             ImplicitDefinition::Undefined(_) => {
+                let saved_def = existing_def.clone();
                 let context: SymbolContext = existing_def.context().clone();
                 let span: Span = *context.any_span();
 
@@ -243,8 +248,9 @@ impl<R: RcUpdater> EvaluationContext<'_, R> {
                                 ) {
                                     Ok(()) => Ok(value),
                                     Err(e) => {
+                                        dbg!(saved_def);
                                         panic!(
-                                            "got a bad symbol definition error ({e:?}) on a previously undefined origin symbol"
+                                            "got a bad symbol definition error ({e:?}) on a previously undefined origin symbol {name}"
                                         );
                                     }
                                 }
@@ -274,6 +280,113 @@ impl<R: RcUpdater> EvaluationContext<'_, R> {
     }
 }
 
+#[test]
+fn can_deduce_address_of_origin_with_previous_forward_reference() {
+    // GIVEN a program containing two blocks, in whicht the second block has a
+    // symbolic origin and where there is a forward reference within the first
+    // block to the address of the second block
+    let origin_name = SymbolName::from("INPUT");
+    let origin_def = Origin::Symbolic(span(2455..2461), origin_name.clone());
+    const BLOCK0_SIZE: Unsigned18Bit = u18!(4);
+    const BLOCK1_SIZE: Unsigned18Bit = u18!(3);
+    let block0_pos = BlockPosition {
+        block_identifier: BlockIdentifier::from(0),
+        // The first reference to block 1 is inside block 1, so the
+        // span of block1 includes the location of the first reference
+        // to origin_name.  This is not a necessary setup for our test
+        // scenario, but it is realistic.
+        span: span(0..2400),
+        origin: None,
+        block_address: None,
+        block_size: BLOCK0_SIZE,
+    };
+    let block1_pos = BlockPosition {
+        block_identifier: BlockIdentifier::from(1),
+        span: span(2455..2800),
+        origin: None,
+        block_address: None,
+        block_size: BLOCK1_SIZE,
+    };
+
+    let mut implicit_symtab = ImplicitSymbolTable::default();
+    implicit_symtab.load_test_definitions([(
+        origin_name.clone(),
+        ImplicitDefinition::Undefined(SymbolContext {
+            address: AddressUse::IncludesAddress,
+            origin: OriginUse::IncludesOrigin(block1_pos.block_identifier, origin_def.clone()),
+            uses: [
+                // The first reference is inside block 0.
+                OrderableSpan(span(1188..1193)),
+                // The second reference is in the origin definition of
+                // block 1 (which is the thing we're trying to assign
+                // a default value to).
+                OrderableSpan(span(block1_pos.span.start..(block1_pos.span.start + 6))),
+                // There is a third reference (but this is just from
+                // our motivating example, it is not necessary to the
+                // test).
+                OrderableSpan(span(3059..3064)),
+            ]
+            .into_iter()
+            .collect(),
+        }),
+    )]);
+
+    let explicit_symtab = ExplicitSymbolTable::default();
+    let mut register_assigner = IndexRegisterAssigner::default();
+    let mut memory_map = MemoryMap::new([
+        (span(0..10), None, BLOCK0_SIZE),
+        (
+            span(2455..2461),
+            Some(Origin::Symbolic(span(2455..2461), origin_name.clone())),
+            BLOCK1_SIZE,
+        ),
+    ]);
+    let mut rc_block = RcBlock::default();
+    let mut context = EvaluationContext {
+        here: HereValue::Address(Address::from(u18!(0o100))),
+        explicit_symtab: &explicit_symtab,
+        implicit_symtab: &mut implicit_symtab,
+        index_register_assigner: &mut register_assigner,
+        memory_map: &memory_map,
+        rc_updater: &mut rc_block,
+        lookup_operation: LookupOperation::default(),
+    };
+
+    // Assign an address for block 0 as part of the scenario setup.
+    let block0_addr = block0_pos
+        .evaluate(&mut context)
+        .expect("should be able to assign an address to the first block");
+    drop(context); // no longer want an immutable borrow on memory_map.
+    memory_map.set_block_position(
+        block0_pos.block_identifier,
+        subword::right_half(block0_addr).into(),
+    );
+
+    // WHEN we try to assign an address to the second block (block 1).
+    let mut context = EvaluationContext {
+        here: HereValue::Address(Address::from(u18!(0o100))),
+        explicit_symtab: &explicit_symtab,
+        implicit_symtab: &mut implicit_symtab,
+        index_register_assigner: &mut register_assigner,
+        memory_map: &memory_map,
+        rc_updater: &mut rc_block,
+        lookup_operation: LookupOperation::default(),
+    };
+    let eval_result = context.fetch_or_assign_default(&origin_name);
+
+    // THEN we should assign an address without failing.
+    match eval_result {
+        Err(e) => {
+            panic!("should not fail to assign a start address to a block: {e}");
+        }
+        Ok(default_value) => {
+            let expected: Unsigned36Bit =
+                u36!(0o200000).wrapping_add(Unsigned36Bit::from(BLOCK0_SIZE));
+            assert_eq!(default_value, expected);
+        }
+    }
+}
+
 impl<R: RcUpdater> EvaluationContext<'_, R> {
     pub(super) fn for_target_address<F, T>(&mut self, new_here: HereValue, mut closure: F) -> T
     where
diff --git a/assembler/src/asmlib/symbol.rs b/assembler/src/asmlib/symbol.rs
@@ -161,10 +161,8 @@ pub(crate) enum AddressUse {
 impl AddressUse {
     fn or(&self, other: &AddressUse) -> AddressUse {
         match (self, other) {
-            (AddressUse::IncludesAddress, _) | (_, AddressUse::IncludesAddress) => {
-                AddressUse::IncludesAddress
-            }
-            _ => AddressUse::NotAddress,
+            (AddressUse::NotAddress, AddressUse::NotAddress) => AddressUse::NotAddress,
+            _ => AddressUse::IncludesAddress,
         }
     }
 }
@@ -249,7 +247,9 @@ impl SymbolContext {
     pub(crate) fn origin(block_id: BlockIdentifier, origin: Origin) -> SymbolContext {
         let span = origin.span();
         SymbolContext {
-            address: AddressUse::NotAddress,
+            // All origins are (implicitly) uses of the symbol as an
+            // address.
+            address: AddressUse::IncludesAddress,
             origin: OriginUse::IncludesOrigin(block_id, origin),
             uses: SymbolContext::uses(span),
         }
@@ -301,7 +301,7 @@ impl SymbolContext {
                 OriginUse::IncludesOrigin(my_block, my_origin),
                 OriginUse::IncludesOrigin(their_block, their_origin),
             ) => {
-                if my_block == their_block && my_origin.has_same_specification(their_origin) {
+                if my_block == their_block {
                     // If one of the origins is a deduced origin, that
                     // has more information, so retain that one.
                     //
@@ -366,7 +366,24 @@ impl SymbolContext {
     }
 
     pub(super) fn requires_rc_word_allocation(&self) -> bool {
-        self.address == AddressUse::IncludesAddress
+        if self.address == AddressUse::IncludesAddress {
+            if matches!(&self.origin, &OriginUse::IncludesOrigin(_, _)) {
+                // This symbol is used in address contexts, but it is
+                // the name used for an origin.  Therefore we do not
+                // need to allocate an RC-word for it.
+                false
+            } else {
+                // This name is used in an address context but it is
+                // not the name of an origin, so when there is no tag
+                // definition for it, we will need to allocate an
+                // RC-word for it.
+                true
+            }
+        } else {
+            // Since nothing expects this symbol to refer to an
+            // address, there is no need to allocate an RC-word.
+            false
+        }
     }
 
     pub(super) fn any_span(&self) -> &Span {
@@ -511,17 +528,103 @@ fn test_origin_cannot_be_used_as_a_configuration_value() {
 
 #[test]
 fn test_deduced_origin_merge() {
-    use super::span::span;
-    use base::prelude::Address;
-    use base::u18;
-    let span = span(0..4);
-    let block = BlockIdentifier::from(0);
-    let name = SymbolName::from("OGN");
-    let address = Address::from(u18!(0o200_000));
-    let mut current = SymbolContext::origin(block, Origin::Symbolic(span, name.clone()));
-    let new_use = SymbolContext::origin(block, Origin::Deduced(span, name.clone(), address));
-    assert_eq!(current.merge(&name, new_use.clone()), Ok(()));
-    assert_eq!(current, new_use);
+    struct Contexts {
+        reference: SymbolContext,
+        origin_definition: SymbolContext,
+        expected_merge: SymbolContext,
+    }
+    // Convenience function for creating the test input and expected output.
+    fn make_symbolic_and_deduced_origin_contexts(
+        name: SymbolName,
+        is_forward_reference: bool,
+    ) -> Contexts {
+        let block = BlockIdentifier::from(0);
+
+        let deduced_origin_span: Span = span(1000..1010);
+        let symbol_span = if is_forward_reference {
+            // The reference appears before the origin specification
+            span(10..20)
+        } else {
+            // The reference appears after the origin specification
+            span(2000..2020)
+        };
+
+        // In our examples, the deduced origin value is a
+        // symbolically-defined origin to which we would deduced
+        // address (from the locations and sizes of the blocks
+        // preceding it).
+        let deduced_origin_context = SymbolContext {
+            address: AddressUse::IncludesAddress,
+            origin: OriginUse::IncludesOrigin(
+                block,
+                Origin::Symbolic(deduced_origin_span, name.clone()),
+            ),
+            uses: SymbolContext::uses(deduced_origin_span),
+        };
+
+        // The symbolic context is is simply a reference to the
+        // origin, either preceding (is_forward_reference) or
+        // following (!is_forward_reference) the definition of the
+        // origin.
+        let reference_context = SymbolContext {
+            address: AddressUse::IncludesAddress,
+            // Although this use of the symbol will turn out to be a
+            // reference to an origin, we cannot tell that at the
+            // reference site, and so the context in which this symbol
+            // is used at that point is not an origin context.
+            origin: OriginUse::NotOrigin {
+                config: ConfigUse::NotConfig,
+                index: IndexUse::NotIndex,
+            },
+            uses: SymbolContext::uses(symbol_span),
+        };
+
+        Contexts {
+            reference: reference_context,
+            origin_definition: deduced_origin_context,
+            expected_merge: SymbolContext {
+                address: AddressUse::IncludesAddress,
+                origin: OriginUse::IncludesOrigin(
+                    BlockIdentifier::from(0),
+                    Origin::Symbolic(deduced_origin_span, name.clone()),
+                ),
+                uses: [
+                    OrderableSpan(deduced_origin_span),
+                    OrderableSpan(symbol_span),
+                ]
+                .into_iter()
+                .collect(),
+            },
+        }
+    }
+    let name = SymbolName::from("OGNX");
+    // Set up contexts for the forward-reference and the backward-reference cases.
+    let contexts_backward_ref = make_symbolic_and_deduced_origin_contexts(name.clone(), false);
+    let contexts_forward_ref = make_symbolic_and_deduced_origin_contexts(name.clone(), true);
+    // The value in expected_merge is not the same in each case, since
+    // although the span of the origin definition is fixed, the span
+    // of the reference to it is different for the forward-reference
+    // and backward-reference cases.
+
+    // Merge for the defined-then-used direction (where we encouter
+    // the origin defintiion and later a reference to it).
+    let mut current = contexts_backward_ref.origin_definition.clone();
+    assert_eq!(
+        current.merge(&name, contexts_backward_ref.reference.clone()),
+        Ok(())
+    );
+    assert_eq!(current, contexts_backward_ref.expected_merge);
+
+    // Merge in forward-reference direction (where we find a reference
+    // to the origin address of a block before we have seen the origin
+    // definition of that block).
+    //
+    let mut current = contexts_forward_ref.reference.clone(); // we find the fwd ref first
+    assert_eq!(
+        current.merge(&name, contexts_forward_ref.origin_definition.clone()),
+        Ok(())
+    );
+    assert_eq!(current, contexts_forward_ref.expected_merge);
 }
 
 impl From<(&Script, Span)> for SymbolContext {
diff --git a/assembler/src/asmlib/symtab.rs b/assembler/src/asmlib/symtab.rs
@@ -85,6 +85,18 @@ impl ImplicitSymbolTable {
             .or_insert_with(|| ImplicitDefinition::DefaultAssigned(value.into(), context.clone()))
             .merge_context(&name, context)
     }
+
+    #[cfg(test)]
+    pub(super) fn load_test_definitions<
+        I: IntoIterator<Item = (SymbolName, ImplicitDefinition)>,
+    >(
+        &mut self,
+        defs: I,
+    ) {
+        for (name, definition) in defs.into_iter() {
+            self.definitions.insert(name, definition);
+        }
+    }
 }
 
 #[derive(Debug, Default, Clone, PartialEq, Eq)]
