[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [tor/master] docs: Document coding standards, build instructions, etc. for Rust code.



commit fe66d06a45a4714141eba992fe87ec3dd5fa1c22
Author: Isis Lovecruft <isis@xxxxxxxxxxxxxx>
Date:   Tue Aug 29 23:25:02 2017 +0000

    docs: Document coding standards, build instructions, etc. for Rust code.
    
     * FIXES #22818
---
 doc/HACKING/CodingStandardsRust.md | 252 +++++++++++++++++++++++++++++++++++++
 doc/HACKING/GettingStartedRust.md  | 158 +++++++++++++++++++++++
 2 files changed, 410 insertions(+)

diff --git a/doc/HACKING/CodingStandardsRust.md b/doc/HACKING/CodingStandardsRust.md
new file mode 100644
index 000000000..2efa8a177
--- /dev/null
+++ b/doc/HACKING/CodingStandardsRust.md
@@ -0,0 +1,252 @@
+
+ Rust Coding Standards
+=======================
+
+You MUST follow the standards laid out in `.../doc/HACKING/CodingStandards.md`,
+where applicable.
+
+ Module/Crate Declarations
+---------------------------
+
+Each Tor C module which is being rewritten MUST be in its own crate.
+See the structure of `.../src/rust` for examples.
+
+In your crate, you MUST use `lib.rs` ONLY for pulling in external
+crates (e.g. `extern crate libc;`) and exporting public objects from
+other Rust modules (e.g. `pub use mymodule::foo;`).  For example, if
+you create a crate in `.../src/rust/yourcrate`, your Rust code should
+live in `.../src/rust/yourcrate/yourcode.rs` and the public interface
+to it should be exported in `.../src/rust/yourcrate/lib.rs`.
+
+If your code is to be called from Tor C code, you MUST define a safe
+`ffi.rs` which ONLY copies `&[u8]`s (i.e. byte arrays) across the FFI
+boundary.
+
+For example, in a hypothetical `tor_addition` Rust module:
+
+In `.../src/rust/tor_addition/addition.rs`:
+
+    pub fn get_sum(a: i32, b: i32) -> i32 {
+        a + b
+    }
+
+In `.../src/rust/tor_addition/lib.rs`:
+
+    pub use addition::*;
+
+In `.../src/rust/tor_addition/ffi.rs`:
+
+    #[no_mangle]
+    pub extern "C" fn tor_get_sum(a: c_int, b: c_int) -> c_int {
+        get_sum(a, b)
+    }
+
+If your Rust code must call out to parts of Tor's C code, you must
+declare the functions you are calling in the `external` crate, located
+at `.../src/rust/external`.
+
+XXX get better examples of how to declare these externs, when/how they
+XXX are unsafe, what they are expected to do â??isis
+
+Modules should strive to be below 500 lines (tests excluded). Single
+responsibility and limited dependencies should be a guiding standard.
+
+If you have any external modules as dependencies (e.g. `extern crate
+libc;`), you MUST declare them in your crate's `lib.rs` and NOT in any
+other module.
+
+ Dependencies
+--------------
+
+In general, we use modules from only the Rust standard library
+whenever possible. We will review including external crates on a
+case-by-case basis.
+
+ Documentation
+---------------
+
+You MUST include `#[deny(missing_docs)]` in your crate.
+
+For example, a one-sentence, "first person" description of function
+behaviour (see requirements for documentation as described in
+`.../src/HACKING/CodingStandards.md`), then an `# Inputs` section for
+inputs or initialisation values, a `# Returns` section for return
+values/types, a `# Warning` section containing warnings for unsafe
+behaviours or panics that could happen.  For publicly accessible
+types/constants/objects/functions/methods, you SHOULD also include an
+`# Examples` section with runnable doctests.
+
+You MUST document your module with _module docstring_ comments,
+i.e. `//!` at the beginning of each line.
+
+ Testing
+---------
+
+All code MUST be unittested and integration tested.
+
+Public functions/objects exported from a crate SHOULD include doctests
+describing how the function/object is expected to be used.
+
+Integration tests SHOULD go into a `tests/` directory inside your
+crate.  Unittests SHOULD go into their own module inside the module
+they are testing, e.g. in `.../src/rust/tor_addition/addition.rs` you
+should put:
+
+    #[cfg(test)]
+    mod test {
+        use super::*;
+
+        #[test]
+        fn addition_with_zero() {
+            let sum: i32 = get_sum(5i32, 0i32);
+            assert_eq!(sum, 5);
+        }
+    }
+
+ Benchmarking
+--------------
+
+If you wish to benchmark some of your Rust code, you MUST put the
+following in the `[features]` section of your crate's `Cargo.toml`:
+
+    [features]
+    bench = []
+
+Next, in your crate's `lib.rs` you MUST put:
+
+    #[cfg(all(test, feature = "bench"))]
+    extern crate test;
+
+This ensures that the external crate `test`, which contains utilities
+for basic benchmarks, is only used when running benchmarks via `cargo
+bench --features bench`.  (This is due to the `test` module requiring
+nightly Rust, and since we may want to switch to a more stable Rust
+compiler eventually we don't want to break builds for stable compilers
+by always requiring the `test` crate.)
+
+Finally, to write your benchmark code, in
+`.../src/rust/tor_addition/addition.rs` you SHOULD put:
+
+    #[cfg(all(test, features = "bench"))]
+    mod bench {
+        use test::Bencher;
+        use super::*;
+
+        #[bench]
+        fn addition_small_integers(b: &mut Bencher) {
+            b.iter(| | get_sum(5i32, 0i32));
+        }
+    }
+
+ Safety
+--------
+
+You SHOULD read [the nomicon](https://doc.rust-lang.org/nomicon/) before writing
+Rust FFI code.  It is *highly advised* that you read and write normal Rust code
+before attempting to write FFI or any other unsafe code.
+
+Here are some additional bits of advice and rules:
+
+1. `unwrap()`
+
+   If you call `unwrap()`, anywhere, even in a test, you MUST include
+   an inline comment stating how the unwrap will either 1) never fail,
+   or 2) should fail (i.e. in a unittest).
+
+2. `unsafe`
+
+   If you use `unsafe`, you MUST describe a contract in your
+   documentation which describes how and when the unsafe code may
+   fail, and what expectations are made w.r.t. the interfaces to
+   unsafe code.  This is also REQUIRED for major pieces of FFI between
+   C and Rust.
+
+   When creating an FFI in Rust for C code to call, it is NOT REQUIRED
+   to declare the entire function `unsafe`.  For example, rather than doing:
+
+        #[no_mangle]
+        pub unsafe extern "C" fn increment_and_combine_numbers(mut numbers: [u8; 4]) -> u32 {
+            for index in 0..numbers.len() {
+                numbers[index] += 1;
+            }
+            std::mem::transmute::<[u8; 4], u32>(numbers)
+        }
+
+   You SHOULD instead do:
+
+        #[no_mangle]
+        pub extern "C" fn increment_and_combine_numbers(mut numbers: [u8; 4]) -> u32 {
+            for index in 0..numbers.len() {
+                numbers[index] += 1;
+            }
+            unsafe {
+                std::mem::transmute::<[u8; 4], u32>(numbers)
+            }
+        }
+
+3. Pass only integer types and bytes over the boundary
+
+   The only non-integer type which may cross the FFI boundary is
+   bytes, e.g. `&[u8]`.  This SHOULD be done on the Rust side by
+   passing a pointer (`*mut libc::c_char`) and a length
+   (`libc::size_t`).
+
+   One might be tempted to do this via doing
+   `CString::new("blah").unwrap().into_raw()`. This has several problems:
+
+   a) If you do `CString::new("bl\x00ah")` then the unwrap() will fail
+      due to the additional NULL terminator, causing a dangling
+      pointer to be returned (as well as a potential use-after-free).
+
+   b) Returning the raw pointer will cause the CString to run its deallocator,
+      which causes any C code which tries to access the contents to dereference a
+      NULL pointer.
+
+   c) If we were to do `as_raw()` this would result in a potential double-free
+      since the Rust deallocator would run and possibly Tor's deallocator.
+
+   d) Calling `into_raw()` without later using the same pointer in Rust to call
+      `from_raw()` and then deallocate in Rust can result in a
+      [memory leak](https://doc.rust-lang.org/std/ffi/struct.CString.html#method.into_raw).
+
+     [It was determined](https://github.com/rust-lang/rust/pull/41074) that this
+     is safe to do if you use the same allocator in C and Rust and also specify
+     the memory alignment for CString (except that there is no way to specify
+     the alignment for CString).  It is believed that the alignment is always 1,
+     which would mean it's safe to dealloc the resulting `*mut c_char` in Tor's
+     C code.  However, the Rust developers are not willing to guarantee the
+     stability of, or a contract for, this behaviour, citing concerns that this
+     is potentially extremely and subtly unsafe.
+
+
+4. Perform an allocation on the other side of the boundary
+
+   After crossing the boundary, the other side MUST perform an
+   allocation to copy the data and is therefore responsible for
+   freeing that memory later.
+
+5. No touching other language's enums
+
+   Rust enums should never be touched from C (nor can they be safely
+   `#[repr(C)]`) nor vice versa:
+
+   >  "The chosen size is the default enum size for the target platform's C
+   >  ABI. Note that enum representation in C is implementation defined, so this is
+   >  really a "best guess". In particular, this may be incorrect when the C code
+   >  of interest is compiled with certain flags."
+
+   (from https://gankro.github.io/nomicon/other-reprs.html)
+
+6. Type safety
+
+   Wherever possible and sensical, you SHOULD create new types, either as tuple
+   structs (e.g. `struct MyInteger(pub u32)`) or as type aliases (e.g. `pub type
+   MyInteger = u32`).
+
+ Whitespace & Formatting
+-------------------------
+
+You MUST run `rustfmt` (https://github.com/rust-lang-nursery/rustfmt)
+on your code before your code will be merged.  You can install rustfmt
+by doing `cargo install rustfmt-nightly` and then run it with `cargo
+fmt`.
diff --git a/doc/HACKING/GettingStartedRust.md b/doc/HACKING/GettingStartedRust.md
new file mode 100644
index 000000000..cfb5196aa
--- /dev/null
+++ b/doc/HACKING/GettingStartedRust.md
@@ -0,0 +1,158 @@
+
+ Hacking on Rust in Tor
+========================
+
+ Getting Started
+-----------------
+
+Please read or review our documentation on Rust coding standards
+(`.../doc/HACKING/CodingStandardsRust.md`) before doing anything.
+
+Please also read
+[the Rust Code of Conduct](https://www.rust-lang.org/en-US/conduct.html). We aim
+to follow the good example set by the Rust community and be excellent to one
+another.  Let's be careful with each other, so we can be memory-safe together!
+
+Next, please contact us before rewriting anything!  Rust in Tor is still an
+experiment.  It is an experiment that we very much want to see succeed, so we're
+going slowly and carefully.  For the moment, it's also a completely
+volunteer-driven effort: while many, if not most, of us are paid to work on Tor,
+we are not yet funded to write Rust code for Tor.  Please be patient with the
+other people who are working on getting more Rust code into Tor, because they
+are graciously donating their free time to contribute to this effort.
+
+ Resources for learning Rust
+-----------------------------
+
+**Beginning resources**
+
+The primary resource for learning Rust is
+[The Book](https://doc.rust-lang.org/book/).  If you'd like to start writing
+Rust immediately, without waiting for anything to install, there is
+[an interactive browser-based playground](https://play.rust-lang.org/).
+
+**Advanced resources**
+
+If you're interested in playing with various Rust compilers and viewing a very
+nicely displayed output of the generated assembly, there is
+[the Godbolt compiler explorer](https://rust.godbolt.org/)
+
+For learning how to write unsafe Rust, read
+[The Rustonomicon](https://doc.rust-lang.org/nomicon/).
+
+For learning everything you ever wanted to know about Rust macros, there is
+[The Little Book of Rust Macros](https://danielkeep.github.io/tlborm/book/index.html).
+
+ Compiling Tor with Rust enabled
+---------------------------------
+
+You will need to run the `configure` script with the `--enable-rust` flag to
+explicitly build with Rust. Additionally, you will need to specify where to
+fetch Rust dependencies, as we allow for either fetching dependencies from Cargo
+or specifying a local directory.
+
+**Fetch dependencies from Cargo**
+
+    ./configure --enable-rust --enable-cargo-online-mode
+
+**Using a local dependency cache**
+
+**NOTE**: local dependency caches which were not *originally* created via
+  `--enable-cargo-online-mode` are broken. See https://bugs.torproject.org/22907
+
+To specify a local directory:
+
+    RUST_DEPENDENCIES='path_to_dependencies_directory' ./configure --enable-rust
+
+(Note that RUST_DEPENDENCIES must be the full path to the directory; it cannot
+be relative.)
+
+You'll need the following Rust dependencies (as of this writing):
+
+    libc==0.2.22
+
+To get them, do:
+
+    mkdir path_to_dependencies_directory
+    cd path_to_dependencies_directory
+    git clone https://github.com/rust-lang/libc
+    cd libc
+    git checkout 0.2.22
+    cargo package
+    cd ..
+    ln -s libc/target/package/libc-0.2.22 libc-0.2.22
+
+
+ Identifying which modules to rewrite
+======================================
+
+The places in the Tor codebase that are good candidates for porting to Rust are:
+
+1. loosely coupled to other Tor submodules,
+2. have high test coverage, and
+3. would benefit from being implemented in a memory safe language.
+
+Help in either identifying places such as this, or working to improve existing
+areas of the C codebase by adding regression tests and simplifying dependencies,
+would be really helpful.
+
+Furthermore, as submodules in C are implemented in Rust, this is a good
+opportunity to refactor, add more tests, and split modules into smaller areas of
+responsibility.
+
+A good first step is to build a module-level callgraph to understand how
+interconnected your target module is.
+
+    git clone https://git.torproject.org/user/nickm/calltool.git
+    cd tor
+    CFLAGS=0 ./configure
+    ../calltool/src/main.py module_callgraph
+
+The output will tell you each module name, along with a set of every module that
+the module calls.  Modules which call fewer other modules are better targets.
+
+ Writing your Rust module
+==========================
+
+Strive to change the C API as little as possible.
+
+We are currently targetting Rust nightly, *for now*. We expect this to change
+moving forward, as we understand more about which nightly features we need. It
+is on our TODO list to try to cultivate good standing with various distro
+maintainers of `rustc` and `cargo`, in order to ensure that whatever version we
+solidify on is readily available.
+
+ Adding your Rust module to Tor's build system
+-----------------------------------------------
+
+0. Your translation of the C module should live in its own crate(s)
+   in the `.../tor/src/rust/` directory.
+1. Add your crate to `.../tor/src/rust/Cargo.toml`, in the
+   `[workspace.members]` section.
+2. Append your crate's static library to the `rust_ldadd` definition
+   (underneath `if USE_RUST`) in `.../tor/Makefile.am`.
+
+ How to test your Rust code
+----------------------------
+
+Everything should be tested full stop.  Even non-public functionality.
+
+Be sure to edit `.../tor/src/test/test_rust.sh` to add the name of your crate to
+the `crates` variable! This will ensure that `cargo test` is run on your crate.
+
+Configure Tor's build system to build with Rust enabled:
+
+    ./configure --enable-fatal-warnings --enable-rust --enable-cargo-online-mode
+
+Tor's test should be run by doing:
+
+    make check
+
+Tor's integration tests should also pass:
+
+    make test-stem
+
+ Submitting a patch
+=====================
+
+Please follow the instructions in `.../doc/HACKING/GettingStarted.md`.



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits