From 9817637edfeb93fb8dca7b5249503d61134bfab6 Mon Sep 17 00:00:00 2001
From: Alex Crawford <poe@code.acrawford.com>
Date: Mon, 29 May 2023 22:25:47 -0700
Subject: [PATCH] log: cleanup and address potential UB

This was spawned by the following error message, introduced in 1.77.0:

    warning: creating a shared reference to mutable static is discouraged
      --> src/log/mod.rs:36:27
       |
    36 |     let logger = unsafe { &LOGGER };
       |                           ^^^^^^^ shared reference to mutable static
       |
       = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>

This changes the implementation slightly so that it no longer returns
a reference to the logger itself; instead returning a marker type that
is called in the same way as the original. It also removes the use of
`cortex_m::singleton!`, since the singleton value wasn't actually
being used.

Before this implementation, I tried removing the builder pattern
entirely and just taking the ITM and RTT loggers in the init function.
That ended up being fine, but it prevented any logging (RTT or
otherwise) before the GPIOs were configured.
---
 firmware/src/log/mod.rs | 52 ++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/firmware/src/log/mod.rs b/firmware/src/log/mod.rs
index 7712a7b..21a87e7 100644
--- a/firmware/src/log/mod.rs
+++ b/firmware/src/log/mod.rs
@@ -13,36 +13,41 @@
 // You should have received a copy of the GNU General Public License
 // along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
+use core::mem::MaybeUninit;
+
 pub mod itm;
 pub mod rtt;
 
-static mut LOGGER: Logger = Logger {
-    #[cfg(feature = "itm")]
-    itm: None,
+static mut LOGGER: MaybeUninit<Logger> = MaybeUninit::uninit();
 
-    #[cfg(feature = "rtt")]
-    rtt: None,
-};
+pub fn init() -> InitializedLogger {
+    static mut INITIALIZED: bool = false;
+    assert!(unsafe { !INITIALIZED }, "logger already initialized");
+    unsafe { INITIALIZED = true };
 
-pub fn init() -> &'static Logger {
-    let logger = unsafe { &LOGGER };
-    log::set_logger(logger).expect("set_logger");
-    logger
-}
+    log::set_logger(unsafe {
+        LOGGER.write(Logger {
+            #[cfg(feature = "itm")]
+            itm: None,
 
-pub struct Logger {
-    #[cfg(feature = "itm")]
-    itm: Option<itm::Logger>,
+            #[cfg(feature = "rtt")]
+            rtt: None,
+        })
+    })
+    .expect("set_logger");
 
-    #[cfg(feature = "rtt")]
-    rtt: Option<rtt::Logger>,
+    InitializedLogger {}
 }
 
-impl Logger {
+#[non_exhaustive]
+pub struct InitializedLogger {}
+
+impl InitializedLogger {
     #[cfg(feature = "itm")]
     pub fn add_itm(&self, logger: itm::Logger) -> &Self {
         log::set_max_level(log::max_level().max(logger.level));
-        unsafe { LOGGER.itm = Some(logger) };
+        unsafe { LOGGER.assume_init_mut().itm = Some(logger) };
+
         log::info!("ITM logging online!");
         self
     }
@@ -50,12 +55,21 @@ impl Logger {
     #[cfg(feature = "rtt")]
     pub fn add_rtt(&self, logger: rtt::Logger) -> &Self {
         log::set_max_level(log::max_level().max(logger.level));
-        unsafe { LOGGER.rtt = Some(logger) };
+        unsafe { LOGGER.assume_init_mut().rtt = Some(logger) };
+
         log::info!("RTT logging online!");
         self
     }
 }
 
+struct Logger {
+    #[cfg(feature = "itm")]
+    itm: Option<itm::Logger>,
+
+    #[cfg(feature = "rtt")]
+    rtt: Option<rtt::Logger>,
+}
+
 impl log::Log for Logger {
     fn enabled(&self, metadata: &log::Metadata) -> bool {
         #[cfg(feature = "itm")]