[cpu] Inclue a bug-reporting URL in the BUGAL alarm message.

We arrange for the URL to provide some reasonable values for bug title
and labels.

Author: youngman-g

cpu/Cargo.toml

@@ -11,10 +11,16 @@ publish = false
 
 [dependencies]
 base = { path = "../base" }
-conv = "0.3"			# MIT license
+conv = "0.3"                    # MIT license
 serde = { version = "1", features = ["derive"] }
-tracing = "0.1"			# MIT license
+tracing = "0.1"                 # MIT license
 wasm-bindgen = "0.2"
+# Using version 1.0 of idna_adapter removes punycode URL support from
+# the url crate.  In general this is a bad idea, but we know that the
+# only URLs we will generate are for a domain name that does not use
+# punycode.
+idna_adapter = "1.0"
+url = "2.5.7"
 
 [lib]
 name = "cpu"

cpu/src/alarm.rs

@@ -7,6 +7,9 @@ use serde::Serialize;
 use base::instruction::Instruction;
 use base::prelude::*;
 
+use super::bugreport::{IssueType, bug_report_url};
+use super::diagnostics::{CurrentInstructionDiagnostics, DiagnosticFetcher};
+
 #[derive(Debug, Clone)]
 pub enum BadMemOp {
     Read(Unsigned36Bit),
@@ -125,6 +128,14 @@ fn test_alarm_kind_round_trip() {
     assert!(AlarmKind::try_from("this is not an alarm name").is_err());
 }
 
+/// Indicates what the emulator was doing when a bug was discovered.
+#[derive(Debug, Clone)]
+pub enum BugActivity {
+    Io,
+    Opcode,
+    AlarmHandling,
+}
+
 // Alarms from User's Handbook section 5-2.2; full names are taken
 // from section 10-2.5.1 (vol 2) of the Technical Manual.
 // These acrronyms are upper case to follow the names in the TX-2 documentation.
@@ -198,7 +209,11 @@ pub enum AlarmDetails {
 
     /// There is a bug in the simulator.
     BUGAL {
-        instr: Option<Instruction>,
+        /// What were we doing?
+        activity: BugActivity,
+        /// What instruction was executing?
+        diagnostics: CurrentInstructionDiagnostics,
+        /// What went wrong?
         message: String,
     },
 }
@@ -222,18 +237,11 @@ impl AlarmDetails {
             AlarmDetails::PSAL(_, _) => AlarmKind::PSAL,
             AlarmDetails::OCSAL(_, _) => AlarmKind::OCSAL,
             AlarmDetails::QSAL(_, _, _) => AlarmKind::QSAL,
-            AlarmDetails::IOSAL {
-                unit: _,
-                operand: _,
-                message: _,
-            } => AlarmKind::IOSAL,
-            AlarmDetails::MISAL { affected_unit: _ } => AlarmKind::MISAL,
+            AlarmDetails::IOSAL { .. } => AlarmKind::IOSAL,
+            AlarmDetails::MISAL { .. } => AlarmKind::MISAL,
             AlarmDetails::ROUNDTUITAL { .. } => AlarmKind::ROUNDTUITAL,
-            AlarmDetails::DEFERLOOPAL { address: _ } => AlarmKind::DEFERLOOPAL,
-            AlarmDetails::BUGAL {
-                instr: _,
-                message: _,
-            } => AlarmKind::BUGAL,
+            AlarmDetails::DEFERLOOPAL { .. } => AlarmKind::DEFERLOOPAL,
+            AlarmDetails::BUGAL { .. } => AlarmKind::BUGAL,
         }
     }
 }
@@ -299,24 +307,22 @@ impl Display for AlarmDetails {
                 "MISAL: program too slow; missed data for unit {affected_unit:o}"
             ),
 
-            BUGAL { instr, message } => {
-                if let Some(instruction) = instr.as_ref() {
-                    if let Ok(symbolic) = SymbolicInstruction::try_from(instruction) {
-                        write!(
-                            f,
-                            "BUGAL: encountered simulator bug during execution of instruction {symbolic}: {message}"
-                        )
-                    } else {
-                        write!(
-                            f,
-                            "BUGAL: encountered simulator bug during execution of instruction {:o}: {}",
-                            instruction.bits(),
-                            message,
-                        )
-                    }
-                } else {
-                    write!(f, "BUGAL: encountered simulator bug: {message}")
-                }
+            BUGAL {
+                diagnostics,
+                message,
+                activity,
+            } => {
+                let (issue_type, activity_desc): (Option<IssueType>, &'static str) = match activity
+                {
+                    BugActivity::AlarmHandling => (None, "alarm handling"),
+                    BugActivity::Io => (Some(IssueType::Io), "I/O"),
+                    BugActivity::Opcode => (Some(IssueType::Opcode), "instruction execution"),
+                };
+                let report_url = bug_report_url(message, issue_type);
+                write!(
+                    f,
+                    "BUGAL: encountered a bug in enumator {activity_desc} during execution of {diagnostics}: {message}; please report this as a bug at {report_url}",
+                )
             }
             DEFERLOOPAL { address } => {
                 write!(
@@ -349,6 +355,10 @@ impl Display for UnmaskedAlarm {
 }
 
 pub trait Alarmer {
-    fn fire_if_not_masked(&mut self, alarm_instance: Alarm) -> Result<(), Alarm>;
-    fn always_fire(&mut self, alarm_instance: Alarm) -> Alarm;
+    fn fire_if_not_masked<F: DiagnosticFetcher>(
+        &mut self,
+        alarm_instance: Alarm,
+        get_diags: F,
+    ) -> Result<(), Alarm>;
+    fn always_fire<F: DiagnosticFetcher>(&mut self, alarm_instance: Alarm, get_diags: F) -> Alarm;
 }

cpu/src/alarmunit.rs

@@ -5,8 +5,10 @@ use std::collections::{BTreeMap, BTreeSet};
 use serde::Serialize;
 use tracing::{Level, event};
 
-use super::alarm::{Alarm, AlarmDetails, AlarmKind, AlarmMaskability, Alarmer};
+use super::alarm::{Alarm, AlarmDetails, AlarmKind, AlarmMaskability, Alarmer, BugActivity};
 use super::changelog::ChangeIndex;
+use super::diagnostics::DiagnosticFetcher;
+use crate::diagnostics::CurrentInstructionDiagnostics;
 
 #[cfg(test)]
 use base::Unsigned6Bit;
@@ -80,17 +82,22 @@ impl AlarmUnit {
         }
     }
 
-    pub fn mask(&mut self, kind: AlarmKind) -> Result<(), Alarm> {
+    pub fn mask(
+        &mut self,
+        kind: AlarmKind,
+        diags: &CurrentInstructionDiagnostics,
+    ) -> Result<(), Alarm> {
         match kind.maskable() {
             AlarmMaskability::Unmaskable => {
                 let bug = Alarm {
                     sequence: None,
                     details: AlarmDetails::BUGAL {
-                        instr: None,
+                        activity: BugActivity::AlarmHandling,
+                        diagnostics: diags.clone(),
                         message: format!("attempt to mask unmaskable alarm {kind}"),
                     },
                 };
-                Err(self.always_fire(bug))
+                Err(self.always_fire(bug, diags))
             }
             AlarmMaskability::Maskable => {
                 self.changes.add(kind);
@@ -161,12 +168,20 @@ impl AlarmUnit {
 }
 
 impl Alarmer for AlarmUnit {
-    fn fire_if_not_masked(&mut self, alarm_instance: Alarm) -> Result<(), Alarm> {
+    fn fire_if_not_masked<F: DiagnosticFetcher>(
+        &mut self,
+        alarm_instance: Alarm,
+        _get_diags: F,
+    ) -> Result<(), Alarm> {
         self.changes.add(alarm_instance.kind());
         self.set_active(alarm_instance)
     }
 
-    fn always_fire(&mut self, alarm_instance: Alarm) -> Alarm {
+    fn always_fire<F: DiagnosticFetcher>(
+        &mut self,
+        alarm_instance: Alarm,
+        get_diagnostics: F,
+    ) -> Alarm {
         let kind = alarm_instance.kind();
         self.changes.add(kind);
         let sequence = alarm_instance.sequence;
@@ -176,7 +191,8 @@ impl Alarmer for AlarmUnit {
                 let bug = Alarm {
                     sequence,
                     details: AlarmDetails::BUGAL {
-                        instr: None,
+                        activity: BugActivity::AlarmHandling,
+                        diagnostics: get_diagnostics.diagnostics(),
                         message: format!(
                             "alarm {kind} is masked, but the caller assumed it could not be"
                         ),
@@ -193,19 +209,32 @@ impl Alarmer for AlarmUnit {
 
 #[test]
 fn unmaskable_alarms_are_not_maskable() {
+    use base::prelude::{Address, Instruction};
+
     let mut alarm_unit = AlarmUnit::new_with_panic(false);
     assert!(!alarm_unit.unmasked_alarm_active());
+    let diagnostics = CurrentInstructionDiagnostics {
+        current_instruction: Instruction::invalid(),
+        instruction_address: Address::ZERO,
+    };
     // Any attempt to mask an unmaskable alarm should itself result in an error.
-    assert!(alarm_unit.mask(AlarmKind::ROUNDTUITAL).is_err());
+    assert!(
+        alarm_unit
+            .mask(AlarmKind::ROUNDTUITAL, &diagnostics)
+            .is_err()
+    );
     // Now we raise some non-maskable alarm.
     assert!(matches!(
-        alarm_unit.fire_if_not_masked(Alarm {
-            sequence: Some(Unsigned6Bit::ZERO),
-            details: AlarmDetails::ROUNDTUITAL {
-                explanation: "The ROUNDTUITAL alarm is not maskable!".to_string(),
-                bug_report_url: "https://github.com/TX-2/TX-2-simulator/issues/144",
+        alarm_unit.fire_if_not_masked(
+            Alarm {
+                sequence: Some(Unsigned6Bit::ZERO),
+                details: AlarmDetails::ROUNDTUITAL {
+                    explanation: "The ROUNDTUITAL alarm is not maskable!".to_string(),
+                    bug_report_url: "https://github.com/TX-2/TX-2-simulator/issues/144",
+                },
             },
-        }),
+            &diagnostics
+        ),
         Err(Alarm {
             sequence: Some(_),
             details: AlarmDetails::ROUNDTUITAL { .. },
@@ -218,20 +247,26 @@ fn unmaskable_alarms_are_not_maskable() {
 
 #[test]
 fn maskable_alarms_are_not_masked_by_default() {
+    use base::prelude::{Address, Instruction};
+
     let mut alarm_unit = AlarmUnit::new_with_panic(false);
     assert!(!alarm_unit.unmasked_alarm_active());
+    let diagnostics = CurrentInstructionDiagnostics {
+        current_instruction: Instruction::invalid(),
+        instruction_address: Address::ZERO,
+    };
     // Now we raise some maskable, but not masked, alarm.
     let the_alarm = Alarm {
         sequence: Some(Unsigned6Bit::ZERO),
         details: AlarmDetails::PSAL(22, "some PPSAL alarm".to_string()),
     };
     // raise the alarm, verify that it really fires.
     assert!(matches!(
-        alarm_unit.fire_if_not_masked(the_alarm),
+        alarm_unit.fire_if_not_masked(the_alarm, &diagnostics),
         Err(Alarm {
             sequence: _,
             details: AlarmDetails::PSAL(22, _),
-        })
+        },)
     ));
     // Verify that the alarm manager considers that an unmasked (in
     // this case because maskable but not actually masked) alarm is active.

cpu/src/bugreport.rs

@@ -0,0 +1,46 @@
+/// Generate bug-report URLs.
+use url::{ParseError, Url};
+
+// We have a version override for idna_adapter in the Cargo.toml file,
+// because GITHUB_URL is ASCII-only and we want to keep the size of
+// the binary down.
+use idna_adapter as _;
+
+const GITHUB_URL: &str = "https://github.com/";
+const ORG_NAME: &str = "TX-2";
+const REPO_NAME: &str = "TX-2-simulator";
+
+#[derive(Debug, Clone, Copy, PartialOrd, Ord, Hash, Eq, PartialEq)]
+pub enum IssueType {
+    Io,
+    Opcode,
+}
+
+pub fn bug_report_url(title: &str, issue_type: Option<IssueType>) -> Url {
+    bug_report_url_internal(title, issue_type).expect("bug-report URLs should always be valid")
+}
+
+fn bug_report_url_internal(title: &str, issue_type: Option<IssueType>) -> Result<Url, ParseError> {
+    let mut url = Url::parse(GITHUB_URL)?.join(&format!("{ORG_NAME}/{REPO_NAME}/issues/new"))?;
+    url.query_pairs_mut()
+        .append_pair("template", "bug_report.md")
+        .append_pair("title", title);
+    match issue_type {
+        Some(IssueType::Io) => {
+            url.query_pairs_mut().append_pair("labels", "I/O");
+        }
+        Some(IssueType::Opcode) => {
+            url.query_pairs_mut().append_pair("labels", "Opcode");
+        }
+        None => (), // add no labels.
+    }
+    Ok(url)
+}
+
+#[test]
+fn test_bug_report_url() {
+    assert_eq!(
+        bug_report_url("Eat some falafel", Some(IssueType::Io)).to_string(),
+        "https://github.com/TX-2/TX-2-simulator/issues/new?template=bug_report.md&title=Eat+some+falafel&labels=I%2FO"
+    );
+}