Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add "on bit flags" section #237

Merged
merged 3 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,23 @@ Pay attention to the "**Architecture Invariant**" sections.
library must be opt-in through a feature flag called `std` that is enabled by default. When the `alloc` crate is optional,
a feature flag called `alloc` must exist to enable its use.

**Architectural Invariant**: no platform-dependant code (`#[cfg(windows)]` and such).

**Architectural Invariant**: no non-essential dependency is allowed.

**Architectural Invariant**: no proc-macro dependency. Dependencies such as `syn` should be pushed
as far as possible from the foundational crates so it doesn’t become too much of a compilation
bottleneck. The paper [Developer Productivity For Humans, Part 4: Build Latency, Predictability,
bottleneck. [Compilation time is a multiplier for everything][why-care-about-build-time].
The paper [Developer Productivity For Humans, Part 4: Build Latency, Predictability,
and Developer Productivity][developer-productivity] by Ciera Jaspan and Collin Green, Google
researchers, elaborates on why it is important to keep build times low.
researchers, also elaborates on why it is important to keep build times low.

**Architectural Invariant**: unless the performance, usability or ergonomic gain is really worth
it, the amount of [monomorphization] incured in downstream user code should be minimal to avoid
binary bloating and to keep the compilation as parallel as possible. Large generic functions should
be avoided if possible.

[why-care-about-build-time]: https://matklad.github.io/2021/09/04/fast-rust-builds.html#Why-Care-About-Build-Times
[developer-productivity]: https://www.computer.org/csdl/magazine/so/2023/04/10176199/1OAJyfknInm
[monomorphization]: https://rustc-dev-guide.rust-lang.org/backend/monomorph.html

Expand All @@ -51,14 +55,14 @@ Meta crate re-exporting important crates.

**Architectural Invariant**: this crate re-exports other crates and does not provide anything else.

_TODO_: clean up the dependencies

#### [`crates/ironrdp-pdu`](./crates/ironrdp-pdu)

PDU encoding and decoding.

_TODO_: talk about important types and traits such as PduDecode, PduEncode…

_TODO_: clean up the dependencies

#### [`crates/ironrdp-graphics`](./crates/ironrdp-graphics)

Image processing primitives.
Expand Down
72 changes: 71 additions & 1 deletion crates/ironrdp-pdu/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,14 @@

RDP PDU encoding and decoding library.

## WIP: overview of encoding and decoding traits
- [Overview of encoding and decoding traits](#overview-of-encoding-and-decoding-traits)
- [Difference between `WriteBuf` and `WriteCursor`](#difference-between-writebuf-and-writecursor)
- [Difference between `WriteBuf` and `Vec<u8>`](#difference-between-writebuf-and-vecu8)
- [Most PDUs are "plain old data" structures with public fields](#most-pdus-are-plain-old-data-structures-with-public-fields)
- [Enumeration-like types should allow resilient parsing](#enumeration-like-types-should-allow-resilient-parsing)
- [On bit flags](#on-bit-flags)

## Overview of encoding and decoding traits

It’s important for `PduEncode` to be object-safe in order to enable patterns such as the one
found in `ironrdp-svc`:
Expand Down Expand Up @@ -400,3 +407,66 @@ MUST always be rejected. In such cases, an `enum` is deemed appropriate.
[RangedU32_get]: https://docs.rs/deranged/0.3.8/deranged/struct.RangedU32.html#method.get
[deranged]: https://docs.rs/deranged/0.3.8/
[http-status-code]: https://docs.rs/http/0.2.9/http/status/struct.StatusCode.html

## On bit flags

The **TL;DR** is:

- Use **both** `from_bits_retain` and `const _ = !0` when resilient parsing is required.
- `const _ = !0` ensures we don’t accidentally have non resilient or destructive parsing.
- `from_bits_retain` makes it clear at the call site that preserving all the bits is intentional.
- Use `from_bits` WITHOUT `const _ = !0` when strictness is required (almost never in IronRDP), and
document why with an in-source comment.

Bit flags are used quite pervasively in the RDP protocol.
IronRDP is relying on the [`bitflags` crate][bitflags] to generate well-defined flags structures,
freeing us from worrying about bitwise logic.

Three notable methods generated by the `bitflags` crate are:

- [`from_bits`][from_bits],
- [`from_bits_truncate`][from_bits_truncate], and
- [`from_bits_retain`][from_bits_retain].

Within IronRDP codebase, `from_bits_retain` should generally be used over `from_bits`, and
`from_bits_truncate` is likely wrong. `from_bits_retain` will simply ignore unknown bits, but will
not unset them (unlike `from_bits_truncate`), i.e.: the underlying `u32` is set exactly to the value
received from the network.

Rationale is:

- PDU decoding and encoding logic should uphold the round-tripping property (`m = encode(decode(m))`),
and for this property to hold, parsing must be non-destructive (i.e.: lossless),
but `from_bits_truncate` is destructive (unknown bits are discarded).
- Resilient parsing is generally preferred, ignoring unknown values as long as they are not needed and/or as
long as ignoring them is causing no harm, but `from_bits` is not lenient

Note that it’s okay to use `from_bits` if strictness is actually required somewhere, but such places must be
documented with a comment explaining why refusing unknown flags is better.

`bitflags` v2.4 also introduced a new syntax in the `bitflags!` macro (<https://github.com/bitflags/bitflags/pull/371>):

```rust
bitflags! {
pub struct Flags: u32 {
const A = 0b00000001;
const B = 0b00000010;
const C = 0b00000100;

// The source may set any bits
const _ = !0; // <- This
}
}
```

There is crate-level documentation for this: <https://docs.rs/bitflags/2.4.0/bitflags/#externally-defined-flags>

This addition makes `from_bits_truncate` behave exactly like `from_bits_retain`, because all values
are considered to be known and defined. In such cases, `from_bits` also never fails therefore works
precisely the same as `from_bits_retain`, except it’s less ergonomic because it returns a `Result`
which must be needlessly handled.

[bitflags]: https://crates.io/crates/bitflags
[from_bits]: https://docs.rs/bitflags/2.4.0/bitflags/example_generated/struct.Flags.html#method.from_bits
[from_bits_truncate]: https://docs.rs/bitflags/2.4.0/bitflags/example_generated/struct.Flags.html#method.from_bits_truncate
[from_bits_retain]: https://docs.rs/bitflags/2.4.0/bitflags/example_generated/struct.Flags.html#method.from_bits_retain