macro: ensure that the same extension should be included only once

Signed-off-by: Zhouqi Jiang <[email protected]>

macros/src/lib.rs

@@ -40,7 +40,7 @@ pub fn derive_rustsbi(input: TokenStream) -> TokenStream {
         let mut skipped = false;
         for attr in &field.attrs {
             if !attr.path().is_ident("rustsbi") {
-                continue
+                continue;
             }
             let parsed = attr.parse_nested_meta(|meta| {
                 if meta.path.is_ident("skip") {
@@ -49,9 +49,7 @@ pub fn derive_rustsbi(input: TokenStream) -> TokenStream {
                     Ok(())
                 } else {
                     let path = meta.path.to_token_stream().to_string().replace(' ', "");
-                    Err(
-                        meta.error(format_args!("unknown rustsbi variant attribute `{}`", path))
-                    )
+                    Err(meta.error(format_args!("unknown RustSBI variant attribute `{}`", path)))
                 }
             });
             if let Err(err) = parsed {
@@ -59,23 +57,36 @@ pub fn derive_rustsbi(input: TokenStream) -> TokenStream {
             }
         }
         if skipped {
-            continue
+            continue;
         }
         if let Some(name) = &field.ident {
-            match name.to_string().as_str() {
-                "rfnc" | "fence" => imp.fence = Some(name),
-                "hsm" => imp.hsm = Some(name),
-                "spi" | "ipi" => imp.ipi = Some(name),
-                "srst" | "reset" => imp.reset = Some(name),
-                "time" | "timer" => imp.timer = Some(name),
-                "pmu" => imp.pmu = Some(name),
-                "dbcn" | "console" => imp.console = Some(name),
-                "susp" => imp.susp = Some(name),
-                "cppc" => imp.cppc = Some(name),
-                "nacl" => imp.nacl = Some(name),
-                "sta" => imp.sta = Some(name),
-                "info" | "env_info" => imp.env_info = Some(name),
-                _ => {}
+            let origin = match name.to_string().as_str() {
+                "rfnc" | "fence" => imp.fence.replace(name),
+                "hsm" => imp.hsm.replace(name),
+                "spi" | "ipi" => imp.ipi.replace(name),
+                "srst" | "reset" => imp.reset.replace(name),
+                "time" | "timer" => imp.timer.replace(name),
+                "pmu" => imp.pmu.replace(name),
+                "dbcn" | "console" => imp.console.replace(name),
+                "susp" => imp.susp.replace(name),
+                "cppc" => imp.cppc.replace(name),
+                "nacl" => imp.nacl.replace(name),
+                "sta" => imp.sta.replace(name),
+                "info" | "env_info" => imp.env_info.replace(name),
+                _ => continue,
+            };
+            if let Some(_origin) = origin {
+                // TODO: provide more detailed proc macro error hinting that previous
+                // definition of this extension resides in `origin` once RFC 1566
+                // (Procedural Macro Diagnostics) is stablized.
+                // Link: https://github.com/rust-lang/rust/issues/54140
+                let error = syn::Error::new_spanned(
+                    &field,
+                    format!("more than one field defined SBI extension '{}'. \
+                    At most one fields should define the same SBI extension; consider using \
+                    #[rustsbi(skip)] to ignore fields that shouldn't be treated as an extension.", name),
+                );
+                ans.extend(TokenStream::from(error.to_compile_error()));
             }
         }
     }

src/lib.rs

@@ -801,6 +801,32 @@ pub extern crate sbi_spec as spec;
 /// # }
 /// ```
 ///
+/// Fields indicating the same extension (SBI extension or `EnvInfo`) shouldn't be included
+/// more than once.
+///
+/// ```compile_fail
+/// #[derive(RustSBI)]
+/// struct MySBI {
+///     fence: MyFence,
+///     rfnc: MyFence, // <-- Second field providing `rustsbi::Fence` implementation
+///     info: MyEnvInfo,
+/// }
+/// # use rustsbi::{RustSBI, HartMask};
+/// # use sbi_spec::binary::SbiRet;
+/// # struct MyFence;
+/// # impl rustsbi::Fence for MyFence {
+/// #     fn remote_fence_i(&self, _: HartMask) -> SbiRet { unimplemented!() }
+/// #     fn remote_sfence_vma(&self, _: HartMask, _: usize, _: usize) -> SbiRet { unimplemented!() }
+/// #     fn remote_sfence_vma_asid(&self, _: HartMask, _: usize, _: usize, _: usize) -> SbiRet { unimplemented!() }
+/// # }
+/// # struct MyEnvInfo;
+/// # impl rustsbi::EnvInfo for MyEnvInfo {
+/// #     fn mvendorid(&self) -> usize { 1 }
+/// #     fn marchid(&self) -> usize { 2 }
+/// #     fn mimpid(&self) -> usize { 3 }
+/// # }
+/// ```
+///
 /// The struct as derive input may include generics, specifically type generics, lifetimes,
 /// constant generics and where clauses.
 ///
@@ -834,7 +860,7 @@ pub extern crate sbi_spec as spec;
 ///
 /// Inner attribute `#[rustsbi(skip)]` informs the macro to skip a certain field when
 /// generating a RustSBI implementation.
-/// 
+///
 /// ```rust
 /// #[derive(RustSBI)]
 /// struct MySBI {
@@ -849,7 +875,7 @@ pub extern crate sbi_spec as spec;
 /// // Notably, a `#[warn(unused)]` would be raised if `fence` is not furtherly used
 /// // by following code; `console` and `info` fields are not warned because they are
 /// // internally used by the trait implementation derived in the RustSBI macro.
-/// # use rustsbi::RustSBI;
+/// # use rustsbi::{HartMask, RustSBI};
 /// # use sbi_spec::binary::{SbiRet, Physical};
 /// # struct MyConsole;
 /// # impl rustsbi::Console for MyConsole {
@@ -858,6 +884,11 @@ pub extern crate sbi_spec as spec;
 /// #     fn write_byte(&self, _: u8) -> SbiRet { unimplemented!() }
 /// # }
 /// # struct MyFence;
+/// # impl rustsbi::Fence for MyFence {
+/// #     fn remote_fence_i(&self, _: HartMask) -> SbiRet { unimplemented!() }
+/// #     fn remote_sfence_vma(&self, _: HartMask, _: usize, _: usize) -> SbiRet { unimplemented!() }
+/// #     fn remote_sfence_vma_asid(&self, _: HartMask, _: usize, _: usize, _: usize) -> SbiRet { unimplemented!() }
+/// # }
 /// # struct MyEnvInfo;
 /// # impl rustsbi::EnvInfo for MyEnvInfo {
 /// #     fn mvendorid(&self) -> usize { 1 }

tests/build-skip.rs

@@ -1,5 +1,9 @@
-use sbi_spec::binary::{Physical, SbiRet};
 use rustsbi::RustSBI;
+use sbi_spec::{
+    binary::{Physical, SbiRet},
+    dbcn::EID_DBCN,
+    rfnc::EID_RFNC,
+};
 
 #[derive(RustSBI)]
 struct SkipExtension {
@@ -26,19 +30,34 @@ fn rustsbi_skip_extension() {
         info: DummyEnvInfo,
     };
     // 1. Skipped fields are neither used during RustSBI macro generation, ...
-    assert_eq!(sbi.handle_ecall(0x4442434E, 0x0, [0; 6]), SbiRet::success(1));
-    assert_eq!(sbi.handle_ecall(0x4442434E, 0x1, [0; 6]), SbiRet::success(2));
-    assert_eq!(sbi.handle_ecall(0x4442434E, 0x2, [0; 6]), SbiRet::success(3));
-    assert_eq!(sbi.handle_ecall(0x52464E43, 0x0, [0; 6]), SbiRet::not_supported());
-    assert_eq!(sbi.handle_ecall(0x52464E43, 0x1, [0; 6]), SbiRet::not_supported());
-    assert_eq!(sbi.handle_ecall(0x52464E43, 0x2, [0; 6]), SbiRet::not_supported());
+    assert_eq!(sbi.handle_ecall(EID_DBCN, 0x0, [0; 6]), SbiRet::success(1));
+    assert_eq!(sbi.handle_ecall(EID_DBCN, 0x1, [0; 6]), SbiRet::success(2));
+    assert_eq!(sbi.handle_ecall(EID_DBCN, 0x2, [0; 6]), SbiRet::success(3));
+    assert_eq!(
+        sbi.handle_ecall(EID_RFNC, 0x0, [0; 6]),
+        SbiRet::not_supported()
+    );
+    assert_eq!(
+        sbi.handle_ecall(EID_RFNC, 0x1, [0; 6]),
+        SbiRet::not_supported()
+    );
+    assert_eq!(
+        sbi.handle_ecall(EID_RFNC, 0x2, [0; 6]),
+        SbiRet::not_supported()
+    );
     // 2. ... nor do they appear in extension detection in Base extension.
-    // note that it's `assert_ne` - the handle_ecall should not return a success detection with 
+    // note that it's `assert_ne` - the handle_ecall should not return a success detection with
     // value 0 indicating this feature is not supported, ...
-    assert_ne!(sbi.handle_ecall(0x10, 0x3, [0x4442434E, 0, 0, 0, 0, 0]), SbiRet::success(0));
+    assert_ne!(
+        sbi.handle_ecall(0x10, 0x3, [EID_DBCN, 0, 0, 0, 0, 0]),
+        SbiRet::success(0)
+    );
     // ... and the `assert_eq` here means extension detection detected successfully that this
     // extension is not supported in SBI implementation `SkipExtension`.
-    assert_eq!(sbi.handle_ecall(0x10, 0x3, [0x52464E43, 0, 0, 0, 0, 0]), SbiRet::success(0));
+    assert_eq!(
+        sbi.handle_ecall(0x10, 0x3, [EID_RFNC, 0, 0, 0, 0, 0]),
+        SbiRet::success(0)
+    );
     // Additionally, we illustrate here that the skipped fields may be used elsewhere.
     let _ = sbi.fence;
 }
@@ -53,9 +72,18 @@ fn rustsbi_skip_env_info() {
     };
     // The `env_info` instead of `info` field would be used by RustSBI macro; struct
     // `RealEnvInfo` would return 7, 8 and 9 for mvendorid, marchid and mimpid.
-    assert_eq!(sbi.handle_ecall(0x10, 0x4, [0x4442434E, 0, 0, 0, 0, 0]), SbiRet::success(7));
-    assert_eq!(sbi.handle_ecall(0x10, 0x5, [0x4442434E, 0, 0, 0, 0, 0]), SbiRet::success(8));
-    assert_eq!(sbi.handle_ecall(0x10, 0x6, [0x4442434E, 0, 0, 0, 0, 0]), SbiRet::success(9));
+    assert_eq!(
+        sbi.handle_ecall(0x10, 0x4, [EID_DBCN, 0, 0, 0, 0, 0]),
+        SbiRet::success(7)
+    );
+    assert_eq!(
+        sbi.handle_ecall(0x10, 0x5, [EID_DBCN, 0, 0, 0, 0, 0]),
+        SbiRet::success(8)
+    );
+    assert_eq!(
+        sbi.handle_ecall(0x10, 0x6, [EID_DBCN, 0, 0, 0, 0, 0]),
+        SbiRet::success(9)
+    );
     let _ = sbi.info;
 }