From 63fd707bd342d614fa620415bb1062b3a40a2488 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Mon, 7 Jan 2019 07:54:46 +0000 Subject: [PATCH 1/6] style: Use atom handles in favour of atom pointers in style system code. Differential Revision: https://phabricator.services.mozilla.com/D15078 --- components/style/Cargo.toml | 1 - components/style/gecko/regen_atoms.py | 67 +------------ components/style/gecko_string_cache/mod.rs | 104 +++++++++++++++------ components/style/lib.rs | 4 - 4 files changed, 82 insertions(+), 94 deletions(-) diff --git a/components/style/Cargo.toml b/components/style/Cargo.toml index 9ec7b149430b..983b53c06e87 100644 --- a/components/style/Cargo.toml +++ b/components/style/Cargo.toml @@ -29,7 +29,6 @@ arrayvec = "0.4.6" atomic_refcell = "0.1" bitflags = "1.0" byteorder = "1.0" -cfg-if = "0.1.0" cssparser = "0.25" crossbeam-channel = { version = "0.3", optional = true } new_debug_unreachable = "1.0" diff --git a/components/style/gecko/regen_atoms.py b/components/style/gecko/regen_atoms.py index bf263a04032b..dfc2252aa899 100755 --- a/components/style/gecko/regen_atoms.py +++ b/components/style/gecko/regen_atoms.py @@ -122,51 +122,10 @@ def __exit__(self, type, value, traceback): // DO NOT EDIT DIRECTLY '''[1:] -IMPORTS = ''' -use gecko_bindings::structs::nsStaticAtom; -use string_cache::Atom; -''' - -UNSAFE_STATIC = ''' -#[inline(always)] -pub unsafe fn atom_from_static(ptr: *const nsStaticAtom) -> Atom { - Atom::from_static(ptr) -} -''' - -SATOMS_TEMPLATE = ''' - #[link_name = \"{link_name}\"] - pub static nsGkAtoms_sAtoms: *const nsStaticAtom; -'''[1:] - -CFG_IF_TEMPLATE = ''' -cfg_if! {{ - if #[cfg(not(target_env = "msvc"))] {{ - extern {{ -{gnu}\ - }} - }} else if #[cfg(target_pointer_width = "64")] {{ - extern {{ -{msvc64}\ - }} - }} else {{ - extern {{ -{msvc32}\ - }} - }} -}}\n -''' - -CONST_TEMPLATE = ''' -pub const k_{name}: isize = {index}; -'''[1:] - RULE_TEMPLATE = ''' -("{atom}") => - {{{{ - use $crate::string_cache::atom_macro; + ("{atom}") => {{{{ #[allow(unsafe_code)] #[allow(unused_unsafe)] - unsafe {{ atom_macro::atom_from_static(atom_macro::nsGkAtoms_sAtoms.offset(atom_macro::k_{name})) }} + unsafe {{ $crate::string_cache::Atom::from_index({index}) }} }}}}; '''[1:] @@ -180,26 +139,8 @@ def __exit__(self, type, value, traceback): def write_atom_macro(atoms, file_name): with FileAvoidWrite(file_name) as f: f.write(PRELUDE) - f.write(IMPORTS) - f.write(UNSAFE_STATIC) - - gnu_name='_ZN9nsGkAtoms6sAtomsE' - gnu_symbols = SATOMS_TEMPLATE.format(link_name=gnu_name) - - # Prepend "\x01" to avoid LLVM prefixing the mangled name with "_". - # See https://github.com/rust-lang/rust/issues/36097 - msvc32_name = '\\x01?sAtoms@nsGkAtoms@@0QBVnsStaticAtom@@B' - msvc32_symbols = SATOMS_TEMPLATE.format(link_name=msvc32_name) - - msvc64_name = '?sAtoms@nsGkAtoms@@0QEBVnsStaticAtom@@EB' - msvc64_symbols = SATOMS_TEMPLATE.format(link_name=msvc64_name) - - f.write(CFG_IF_TEMPLATE.format(gnu=gnu_symbols, msvc32=msvc32_symbols, msvc64=msvc64_symbols)) - - consts = [CONST_TEMPLATE.format(name=atom.ident, index=i) for (i, atom) in enumerate(atoms)] - f.write('{}'.format(''.join(consts))) - - macro_rules = [RULE_TEMPLATE.format(atom=atom.value, name=atom.ident) for atom in atoms] + macro_rules = [RULE_TEMPLATE.format(atom=atom.value, name=atom.ident, index=i) + for (i, atom) in enumerate(atoms)] f.write(MACRO_TEMPLATE.format(body=''.join(macro_rules))) diff --git a/components/style/gecko_string_cache/mod.rs b/components/style/gecko_string_cache/mod.rs index 5335c8c65aaf..d06091df80b7 100644 --- a/components/style/gecko_string_cache/mod.rs +++ b/components/style/gecko_string_cache/mod.rs @@ -15,6 +15,7 @@ use crate::gecko_bindings::bindings::Gecko_Atomize; use crate::gecko_bindings::bindings::Gecko_Atomize16; use crate::gecko_bindings::bindings::Gecko_ReleaseAtom; use crate::gecko_bindings::structs::{nsAtom, nsDynamicAtom, nsStaticAtom}; +use crate::gecko_bindings::structs::root::mozilla::detail::gGkAtoms; use nsstring::{nsAString, nsStr}; use precomputed_hash::PrecomputedHash; use std::borrow::{Borrow, Cow}; @@ -43,9 +44,12 @@ macro_rules! local_name { }; } -/// A strong reference to a Gecko atom. +/// A handle to a Gecko atom. +/// +/// This is either a strong reference to a dynamic atom (an nsAtom pointer), +/// or an offset from gGkAtoms to the nsStaticAtom object. #[derive(Eq, PartialEq)] -pub struct Atom(*mut WeakAtom); +pub struct Atom(usize); /// An atom *without* a strong reference. /// @@ -53,12 +57,31 @@ pub struct Atom(*mut WeakAtom); /// where `'a` is the lifetime of something that holds a strong reference to that atom. pub struct WeakAtom(nsAtom); +#[inline] +fn valid_static_atom_addr(addr: usize) -> bool { + unsafe { + let start = gGkAtoms.mAtoms.get_unchecked(0) as *const _; + let end = gGkAtoms.mAtoms.get_unchecked(gGkAtoms.mAtoms.len()) as *const _; + let in_range = addr >= start as usize && addr < end as usize; + let aligned = addr % mem::align_of::() == 0; + in_range && aligned + } +} + impl Deref for Atom { type Target = WeakAtom; #[inline] fn deref(&self) -> &WeakAtom { - unsafe { &*self.0 } + unsafe { + let addr = if self.is_static() { + (&gGkAtoms as *const _ as usize) + (self.0 >> 1) + } else { + self.0 + }; + debug_assert!(!self.is_static() || valid_static_atom_addr(addr)); + WeakAtom::new(addr as *const nsAtom) + } } } @@ -277,50 +300,73 @@ impl fmt::Display for WeakAtom { } } +#[inline] +unsafe fn make_handle(ptr: *const nsAtom) -> usize { + debug_assert!(!ptr.is_null()); + if !WeakAtom::new(ptr).is_static() { + ptr as usize + } else { + make_static_handle(ptr as *mut nsStaticAtom) + } +} + +#[inline] +unsafe fn make_static_handle(ptr: *const nsStaticAtom) -> usize { + // FIXME(heycam): Use offset_from once it's stabilized. + // https://github.com/rust-lang/rust/issues/41079 + debug_assert!(valid_static_atom_addr(ptr as usize)); + let base = &gGkAtoms as *const _; + let offset = ptr as usize - base as usize; + (offset << 1) | 1 +} + impl Atom { + #[inline] + fn is_static(&self) -> bool { + self.0 & 1 == 1 + } + /// Execute a callback with the atom represented by `ptr`. pub unsafe fn with(ptr: *const nsAtom, callback: F) -> R where F: FnOnce(&Atom) -> R, { - let atom = Atom(WeakAtom::new(ptr)); + let atom = Atom(make_handle(ptr as *mut nsAtom)); let ret = callback(&atom); mem::forget(atom); ret } - /// Creates an atom from an static atom pointer without checking in release - /// builds. - /// - /// Right now it's only used by the atom macro, and ideally it should keep - /// that way, now we have sugar for is_static, creating atoms using - /// Atom::from_raw should involve almost no overhead. + /// Creates a static atom from its index in the static atom table, without + /// checking in release builds. #[inline] - pub unsafe fn from_static(ptr: *const nsStaticAtom) -> Self { - let atom = Atom(ptr as *mut WeakAtom); - debug_assert!( - atom.is_static(), - "Called from_static for a non-static atom!" - ); + pub unsafe fn from_index(index: u16) -> Self { + let ptr = gGkAtoms.mAtoms.get_unchecked(index as usize) as *const _; + let handle = make_static_handle(ptr); + let atom = Atom(handle); + debug_assert!(valid_static_atom_addr(ptr as usize)); + debug_assert!(atom.is_static()); + debug_assert!((*atom).is_static()); + debug_assert!(handle == make_handle(atom.as_ptr())); atom } /// Creates an atom from an atom pointer. #[inline(always)] pub unsafe fn from_raw(ptr: *mut nsAtom) -> Self { - let atom = Atom(ptr as *mut WeakAtom); + let atom = Atom(make_handle(ptr)); if !atom.is_static() { Gecko_AddRefAtom(ptr); } atom } - /// Creates an atom from a dynamic atom pointer that has already had AddRef - /// called on it. + /// Creates an atom from an atom pointer that has already had AddRef + /// called on it. This may be a static or dynamic atom. #[inline] pub unsafe fn from_addrefed(ptr: *mut nsAtom) -> Self { assert!(!ptr.is_null()); - Atom(WeakAtom::new(ptr)) + Atom(make_handle(ptr)) } /// Convert this atom into an addrefed nsAtom pointer. @@ -353,7 +399,13 @@ impl Hash for WeakAtom { impl Clone for Atom { #[inline(always)] fn clone(&self) -> Atom { - unsafe { Atom::from_raw(self.as_ptr()) } + unsafe { + let atom = Atom(self.0); + if !atom.is_static() { + Gecko_AddRefAtom(atom.as_ptr()); + } + atom + } } } @@ -377,13 +429,13 @@ impl Default for Atom { impl fmt::Debug for Atom { fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result { - write!(w, "Gecko Atom({:p}, {})", self.0, self) + write!(w, "Atom(0x{:08x}, {})", self.0, self) } } impl fmt::Display for Atom { fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result { - unsafe { (&*self.0).fmt(w) } + self.deref().fmt(w) } } @@ -392,10 +444,10 @@ impl<'a> From<&'a str> for Atom { fn from(string: &str) -> Atom { debug_assert!(string.len() <= u32::max_value() as usize); unsafe { - Atom(WeakAtom::new(Gecko_Atomize( + Atom::from_addrefed(Gecko_Atomize( string.as_ptr() as *const _, string.len() as u32, - ))) + )) } } } @@ -410,7 +462,7 @@ impl<'a> From<&'a [u16]> for Atom { impl<'a> From<&'a nsAString> for Atom { #[inline] fn from(string: &nsAString) -> Atom { - unsafe { Atom(WeakAtom::new(Gecko_Atomize16(string))) } + unsafe { Atom::from_addrefed(Gecko_Atomize16(string)) } } } diff --git a/components/style/lib.rs b/components/style/lib.rs index 5bcc7a574b93..79829081b39f 100644 --- a/components/style/lib.rs +++ b/components/style/lib.rs @@ -32,10 +32,6 @@ extern crate atomic_refcell; extern crate bitflags; #[allow(unused_extern_crates)] extern crate byteorder; -#[cfg(feature = "gecko")] -#[macro_use] -#[no_link] -extern crate cfg_if; #[cfg(feature = "servo")] extern crate crossbeam_channel; #[macro_use] From 03507801cfb7b90bcfaee5da901d406cb94ebdb1 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Mon, 7 Jan 2019 09:50:25 +0000 Subject: [PATCH 2/6] style: Make GkAtoms opaque to avoid lld-link.exe errors. Differential Revision: https://phabricator.services.mozilla.com/D15800 --- components/style/gecko_string_cache/mod.rs | 29 +++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/components/style/gecko_string_cache/mod.rs b/components/style/gecko_string_cache/mod.rs index d06091df80b7..197eca1640c0 100644 --- a/components/style/gecko_string_cache/mod.rs +++ b/components/style/gecko_string_cache/mod.rs @@ -15,7 +15,9 @@ use crate::gecko_bindings::bindings::Gecko_Atomize; use crate::gecko_bindings::bindings::Gecko_Atomize16; use crate::gecko_bindings::bindings::Gecko_ReleaseAtom; use crate::gecko_bindings::structs::{nsAtom, nsDynamicAtom, nsStaticAtom}; +use crate::gecko_bindings::structs::root::mozilla::detail::GkAtoms_Atoms_AtomsCount; use crate::gecko_bindings::structs::root::mozilla::detail::gGkAtoms; +use crate::gecko_bindings::structs::root::mozilla::detail::kGkAtomsArrayOffset; use nsstring::{nsAString, nsStr}; use precomputed_hash::PrecomputedHash; use std::borrow::{Borrow, Cow}; @@ -57,11 +59,32 @@ pub struct Atom(usize); /// where `'a` is the lifetime of something that holds a strong reference to that atom. pub struct WeakAtom(nsAtom); +/// The number of static atoms we have. +const STATIC_ATOM_COUNT: usize = GkAtoms_Atoms_AtomsCount as usize; + +/// Returns the Gecko static atom array. +/// +/// We have this rather than use rust-bindgen to generate +/// mozilla::detail::gGkAtoms and then just reference gGkAtoms.mAtoms, so we +/// avoid a problem with lld-link.exe on Windows. +/// +/// https://bugzilla.mozilla.org/show_bug.cgi?id=1517685 +#[inline] +fn static_atoms() -> &'static [nsStaticAtom; STATIC_ATOM_COUNT] { + unsafe { + let addr = &gGkAtoms as *const _ as usize + kGkAtomsArrayOffset as usize; + &*(addr as *const _) + } +} + +/// Returns whether the specified address points to one of the nsStaticAtom +/// objects in the Gecko static atom array. #[inline] fn valid_static_atom_addr(addr: usize) -> bool { unsafe { - let start = gGkAtoms.mAtoms.get_unchecked(0) as *const _; - let end = gGkAtoms.mAtoms.get_unchecked(gGkAtoms.mAtoms.len()) as *const _; + let atoms = static_atoms(); + let start = atoms.get_unchecked(0) as *const _; + let end = atoms.get_unchecked(STATIC_ATOM_COUNT) as *const _; let in_range = addr >= start as usize && addr < end as usize; let aligned = addr % mem::align_of::() == 0; in_range && aligned @@ -341,7 +364,7 @@ impl Atom { /// checking in release builds. #[inline] pub unsafe fn from_index(index: u16) -> Self { - let ptr = gGkAtoms.mAtoms.get_unchecked(index as usize) as *const _; + let ptr = static_atoms().get_unchecked(index as usize) as *const _; let handle = make_static_handle(ptr); let atom = Atom(handle); debug_assert!(valid_static_atom_addr(ptr as usize)); From 28cc747a0d71b3a84934f2509a093c358d73fadc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 11 Jan 2019 00:55:09 +0100 Subject: [PATCH 3/6] style: Use a less-specific indexmap version. --- components/style/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/style/Cargo.toml b/components/style/Cargo.toml index 983b53c06e87..1940219ea749 100644 --- a/components/style/Cargo.toml +++ b/components/style/Cargo.toml @@ -38,7 +38,7 @@ fallible = { path = "../fallible" } fxhash = "0.2" hashglobe = { path = "../hashglobe" } html5ever = {version = "0.22", optional = true} -indexmap = "1.0.2" +indexmap = "1.0" itertools = "0.8" itoa = "0.4" lazy_static = "1" From 119b316885f4ceb584bf920e8b2c17ce8bfe68bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 11 Jan 2019 00:57:26 +0100 Subject: [PATCH 4/6] style: Rustfmt recent changes. --- components/style/gecko_string_cache/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/style/gecko_string_cache/mod.rs b/components/style/gecko_string_cache/mod.rs index 197eca1640c0..9c33f25d9b85 100644 --- a/components/style/gecko_string_cache/mod.rs +++ b/components/style/gecko_string_cache/mod.rs @@ -14,10 +14,10 @@ use crate::gecko_bindings::bindings::Gecko_AddRefAtom; use crate::gecko_bindings::bindings::Gecko_Atomize; use crate::gecko_bindings::bindings::Gecko_Atomize16; use crate::gecko_bindings::bindings::Gecko_ReleaseAtom; -use crate::gecko_bindings::structs::{nsAtom, nsDynamicAtom, nsStaticAtom}; -use crate::gecko_bindings::structs::root::mozilla::detail::GkAtoms_Atoms_AtomsCount; use crate::gecko_bindings::structs::root::mozilla::detail::gGkAtoms; use crate::gecko_bindings::structs::root::mozilla::detail::kGkAtomsArrayOffset; +use crate::gecko_bindings::structs::root::mozilla::detail::GkAtoms_Atoms_AtomsCount; +use crate::gecko_bindings::structs::{nsAtom, nsDynamicAtom, nsStaticAtom}; use nsstring::{nsAString, nsStr}; use precomputed_hash::PrecomputedHash; use std::borrow::{Borrow, Cow}; From 99f6d6f1b88d5f34c3092546626fa239e75d1c69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 11 Jan 2019 01:07:22 +0100 Subject: [PATCH 5/6] style: Three-value position syntax uses calc() as its computed value representation. This restores the previous behavior of using calc(). Note that background-position / object-position, which test this, weren't hitting the assertion because they use another codepath. I didn't add more extensive tests for this because it's well tested for those two properties, and because this is legacy anyway, see the comment in the test. I did add the assertion to the codepath those two properties hit. Differential Revision: https://phabricator.services.mozilla.com/D16176 --- components/style/gecko/conversions.rs | 7 ++++++- components/style/values/specified/position.rs | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/components/style/gecko/conversions.rs b/components/style/gecko/conversions.rs index 89cce99beb09..345e199a9a63 100644 --- a/components/style/gecko/conversions.rs +++ b/components/style/gecko/conversions.rs @@ -22,8 +22,8 @@ use crate::values::computed::transform::Matrix3D; use crate::values::computed::url::ComputedImageUrl; use crate::values::computed::{Angle, Gradient, Image}; use crate::values::computed::{Integer, LengthPercentage}; +use crate::values::computed::{Length, Percentage, TextAlign}; use crate::values::computed::{LengthPercentageOrAuto, NonNegativeLengthPercentageOrAuto}; -use crate::values::computed::{Percentage, TextAlign}; use crate::values::generics::box_::VerticalAlign; use crate::values::generics::grid::{TrackListValue, TrackSize}; use crate::values::generics::image::{CompatMode, GradientItem, Image as GenericImage}; @@ -35,6 +35,11 @@ use style_traits::values::specified::AllowedNumericType; impl From for nsStyleCoord_CalcValue { fn from(other: LengthPercentage) -> nsStyleCoord_CalcValue { + debug_assert!( + other.was_calc || + other.percentage.is_none() || + other.unclamped_length() == Length::zero() + ); let has_percentage = other.percentage.is_some(); nsStyleCoord_CalcValue { mLength: other.unclamped_length().to_i32_au(), diff --git a/components/style/values/specified/position.rs b/components/style/values/specified/position.rs index d13bc579f021..b9006ee799b3 100644 --- a/components/style/values/specified/position.rs +++ b/components/style/values/specified/position.rs @@ -263,11 +263,12 @@ impl ToComputedValue for PositionComponent { let length = length.to_computed_value(context); let p = Percentage(1. - length.percentage()); let l = -length.unclamped_length(); + // We represent ` ` as `calc(100% - )`. ComputedLengthPercentage::with_clamping_mode( l, Some(p), length.clamping_mode, - length.was_calc, + /* was_calc = */ true, ) }, PositionComponent::Side(_, Some(ref length)) | From cc1472ea2fd79b098d794ee5a73b4d7bbea96e15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 11 Jan 2019 01:41:59 +0100 Subject: [PATCH 6/6] Update lockfile. --- Cargo.lock | 1 - 1 file changed, 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 7dbe8242f340..aa8497b87f6a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3861,7 +3861,6 @@ dependencies = [ "bindgen 0.46.0 (registry+https://github.com/rust-lang/crates.io-index)", "bitflags 1.0.3 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", - "cfg-if 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)", "crossbeam-channel 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "cssparser 0.25.1 (registry+https://github.com/rust-lang/crates.io-index)", "encoding_rs 0.8.12 (registry+https://github.com/rust-lang/crates.io-index)",