Pleco icon indicating copy to clipboard operation
Pleco copied to clipboard

Reduce unsafe code

Open terrorfisch opened this issue 3 years ago • 0 comments

There are mutliple uses of unsafe that can be either replaced with safe code or with an external crate. I propose using this issue to discuss these cases and then document in the code why each unsafe is fine. This makes code review much easier.

  • Transmutes to enums. Totally fine although the compiler generates the same assembly for safe match statements.
  • Custom Arc without weak counter from servo_arc. The discussion to include this in std died here. Why not use the crate? The currently published version has possible UB https://github.com/servo/servo/issues/26358
  • Custom TranspositionTable that allows to trigger undefined behaviour from safe code (if I understand the code correctly):
let tt = TranspositionTable::new_num_entries(40000);
let prng = PRNG::init(932445561);
let key: u64 = prng.rand();
let (found1, entry1): (bool, &mut Entry) = tt.probe(key);
let (found2, entry2) = tt.probe(key); // second mutable reference to the same object -> UB

I did not look into the usecases yet. There are probably alternatives available in the ecosystem.

  • Custom TimeManager: Internals can be replaced with atomics and Ordering::Relaxed load/store (although the Instance might be problematic). Also: do not create mutable references from UnsafeCell in unguarded code but use ptr::write to avoid UB.
  • Unchecked array access with enums like this: https://github.com/sfleischman105/Pleco/blob/292d38e78dd7d82112aef79a379e8329707e3659/pleco_engine/src/tables/counter_move.rs#L24 I am quite sure that the compiler will elide the bound checks. Another possibility is to use enum_map or static asserts.
  • Remove static mut and replace it either with lazy_static or with const fn initialization. https://github.com/sfleischman105/Pleco/blob/292d38e78dd7d82112aef79a379e8329707e3659/pleco_engine/src/tables/pawn_table.rs#L82
  • Use arrayvec or tinyvec for MoveList.
  • mem::uninitialized to gether with an enum that encodes if a field is used or not. Is there a reason that this is a repr(u8) enum instead of carrying the data itself? https://github.com/sfleischman105/Pleco/blob/292d38e78dd7d82112aef79a379e8329707e3659/pleco_engine/src/movepick/mod.rs#L64

terrorfisch avatar Jan 24 '21 14:01 terrorfisch