From 6fbddc193161d177af680778ad55fe193c4c878b Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 25 Aug 2021 22:15:24 -0500 Subject: [PATCH] Replace some cfg(debug) with cfg(debug_assertions) (#3242) * Replace some cfg(debug) with cfg(debug_assertions) Cargo nor rustc ever sets `cfg(debug)` automatically, so it's expected that these usages were intended to be `cfg(debug_assertions)`. * Fix MachBuffer debug-assertion invariant checks. We should only check invariants when we expect them to be true -- specifically, before the branch-simplification algorithm runs. At other times, they may be temporarily violated: e.g., after `add_{cond,uncond}_branch()` but before emitting the branch bytes. This is the expected sequence, and the rest of the code is consistent with that. Some of the checks also were not quite right (w.r.t. the written invariants); specifically, we should not check validity of a label's offset when the label has been aliased to another label. It seems that this is an unfortunate consequence of leftover debug-assertions that weren't actually being run, so weren't kept up-to-date. Should no longer happen now that we actually check these! Co-authored-by: Chris Fallin --- cranelift/codegen/src/machinst/buffer.rs | 28 ++++++++++--------- .../src/debug/transform/address_transform.rs | 2 +- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index 03debd9454..be9bf468e1 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -321,8 +321,15 @@ impl MachBuffer { /// Debug-only: check invariants of labels and branch-records described /// under "Branch-optimization Correctness" above. - #[cfg(debug)] + /// + /// These invariants are checked at branch-simplification + /// time. Note that they may be temporarily violated at other + /// times, e.g. after calling `add_{cond,uncond}_branch()` and + /// before emitting branch bytes. fn check_label_branch_invariants(&self) { + if !cfg!(debug_assertions) { + return; + } let cur_off = self.cur_offset(); // Check that every entry in latest_branches has *correct* // labels_at_this_branch lists. We do not check completeness because @@ -340,10 +347,15 @@ impl MachBuffer { } } - // Check that every label is unresolved, or resolved at or before - // cur_offset. If at cur_offset, must be in `labels_at_tail`. + // Check that every label is unresolved, or resolved at or + // before cur_offset. If at cur_offset, must be in + // `labels_at_tail`. We skip labels that are aliased to + // others already. for (i, &off) in self.label_offsets.iter().enumerate() { let label = MachLabel(i as u32); + if self.label_aliases[i] != UNKNOWN_LABEL { + continue; + } debug_assert!(off == UNKNOWN_LABEL_OFFSET || off <= cur_off); if off == cur_off { debug_assert!( @@ -363,11 +375,6 @@ impl MachBuffer { } } - #[cfg(not(debug))] - fn check_label_branch_invariants(&self) { - // Nothing. - } - /// Current offset from start of buffer. pub fn cur_offset(&self) -> CodeOffset { self.data.len() as CodeOffset @@ -527,7 +534,6 @@ impl MachBuffer { self.optimize_branches(); // Post-invariant: by `optimize_branches()` (see argument there). - self.check_label_branch_invariants(); } /// Lazily clear `labels_at_tail` if the tail offset has moved beyond the @@ -594,7 +600,6 @@ impl MachBuffer { } // Post-invariant: no mutations to branches/labels data structures. - self.check_label_branch_invariants(); } /// Inform the buffer of an unconditional branch at the given offset, @@ -628,7 +633,6 @@ impl MachBuffer { // Post-invariant: we asserted branch start is current tail; the list of // labels at branch is cloned from list of labels at current tail. - self.check_label_branch_invariants(); } /// Inform the buffer of a conditional branch at the given offset, @@ -662,7 +666,6 @@ impl MachBuffer { // Post-invariant: we asserted branch start is current tail; labels at // branch list is cloned from list of labels at current tail. - self.check_label_branch_invariants(); } fn truncate_last_branch(&mut self) { @@ -757,7 +760,6 @@ impl MachBuffer { // (see comments to `add_{cond,uncond}_branch()`). // - The buffer is truncated to just before the last branch, and the // fixup record referring to that last branch is removed. - self.check_label_branch_invariants(); } fn optimize_branches(&mut self) { diff --git a/crates/cranelift/src/debug/transform/address_transform.rs b/crates/cranelift/src/debug/transform/address_transform.rs index 4f3ee2fe04..aac8b103b3 100644 --- a/crates/cranelift/src/debug/transform/address_transform.rs +++ b/crates/cranelift/src/debug/transform/address_transform.rs @@ -212,7 +212,7 @@ fn build_function_addr_map( }); } - if cfg!(debug) { + if cfg!(debug_assertions) { // fn_map is sorted by the generated field -- see FunctionAddressMap::instructions. for i in 1..fn_map.len() { assert_le!(fn_map[i - 1].generated, fn_map[i].generated);