Add support for 'unsafe fields' #20

Manishearth · 2016-07-20T14:59:19Z

Needed for the next step of servo/servo#12521

These are fields which are private but have unsafe accessor functions. Since we generate bindings in a separate module, we can't touch private fields from other parts of the module (an alternative is to inject a footer with these private impls). pub(restricted) exists, but is not stable.

r? @emilio

highfive · 2016-07-20T14:59:22Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

highfive · 2016-07-20T14:59:23Z

Warning

These commits modify unsafe code. Please review it carefully!

Manishearth · 2016-07-20T14:59:37Z

An example diff when run on Stylo:

diff --git a/ports/geckolib/gecko_bindings/structs_debug.rs b/ports/geckolib/gecko_bindings/structs_debug.rs
index e0763c1..f641604 100644
--- a/ports/geckolib/gecko_bindings/structs_debug.rs
+++ b/ports/geckolib/gecko_bindings/structs_debug.rs
@@ -4908,8 +4908,22 @@ fn bindgen_test_layout_nsStyleCoord() {
 #[repr(C)]
 #[derive(Debug)]
 pub struct nsStyleSides {
-    pub mUnits: [nsStyleUnit; 4usize],
-    pub mValues: [nsStyleUnion; 4usize],
+    mUnits: [nsStyleUnit; 4usize],
+    mValues: [nsStyleUnion; 4usize],
+}
+impl nsStyleSides {
+    #[inline]
+    pub unsafe fn get_mUnits(&self) -> &[nsStyleUnit; 4usize] { &self.mUnits }
+    pub unsafe fn get_mUnits_mut(&mut self) -> &mut [nsStyleUnit; 4usize] {
+        &mut self.mUnits
+    }
+    #[inline]
+    pub unsafe fn get_mValues(&self) -> &[nsStyleUnion; 4usize] {
+        &self.mValues
+    }
+    pub unsafe fn get_mValues_mut(&mut self) -> &mut [nsStyleUnion; 4usize] {
+        &mut self.mValues
+    }
 }
 #[test]
 fn bindgen_test_layout_nsStyleSides() {
@@ -4924,8 +4938,22 @@ fn bindgen_test_layout_nsStyleSides() {
 #[repr(C)]
 #[derive(Debug)]
 pub struct nsStyleCorners {
-    pub mUnits: [nsStyleUnit; 8usize],
-    pub mValues: [nsStyleUnion; 8usize],
+    mUnits: [nsStyleUnit; 8usize],
+    mValues: [nsStyleUnion; 8usize],
+}
+impl nsStyleCorners {
+    #[inline]
+    pub unsafe fn get_mUnits(&self) -> &[nsStyleUnit; 8usize] { &self.mUnits }
+    pub unsafe fn get_mUnits_mut(&mut self) -> &mut [nsStyleUnit; 8usize] {
+        &mut self.mUnits
+    }
+    #[inline]
+    pub unsafe fn get_mValues(&self) -> &[nsStyleUnion; 8usize] {
+        &self.mValues
+    }
+    pub unsafe fn get_mValues_mut(&mut self) -> &mut [nsStyleUnion; 8usize] {
+        &mut self.mValues
+    }
 }
 #[test]
 fn bindgen_test_layout_nsStyleCorners() {
diff --git a/ports/geckolib/gecko_bindings/structs_release.rs b/ports/geckolib/gecko_bindings/structs_release.rs
index 16d9046..e6c7af5 100644
--- a/ports/geckolib/gecko_bindings/structs_release.rs
+++ b/ports/geckolib/gecko_bindings/structs_release.rs
@@ -4887,8 +4887,22 @@ fn bindgen_test_layout_nsStyleCoord() {
 #[repr(C)]
 #[derive(Debug)]
 pub struct nsStyleSides {
-    pub mUnits: [nsStyleUnit; 4usize],
-    pub mValues: [nsStyleUnion; 4usize],
+    mUnits: [nsStyleUnit; 4usize],
+    mValues: [nsStyleUnion; 4usize],
+}
+impl nsStyleSides {
+    #[inline]
+    pub unsafe fn get_mUnits(&self) -> &[nsStyleUnit; 4usize] { &self.mUnits }
+    pub unsafe fn get_mUnits_mut(&mut self) -> &mut [nsStyleUnit; 4usize] {
+        &mut self.mUnits
+    }
+    #[inline]
+    pub unsafe fn get_mValues(&self) -> &[nsStyleUnion; 4usize] {
+        &self.mValues
+    }
+    pub unsafe fn get_mValues_mut(&mut self) -> &mut [nsStyleUnion; 4usize] {
+        &mut self.mValues
+    }
 }
 #[test]
 fn bindgen_test_layout_nsStyleSides() {
@@ -4903,8 +4917,22 @@ fn bindgen_test_layout_nsStyleSides() {
 #[repr(C)]
 #[derive(Debug)]
 pub struct nsStyleCorners {
-    pub mUnits: [nsStyleUnit; 8usize],
-    pub mValues: [nsStyleUnion; 8usize],
+    mUnits: [nsStyleUnit; 8usize],
+    mValues: [nsStyleUnion; 8usize],
+}
+impl nsStyleCorners {
+    #[inline]
+    pub unsafe fn get_mUnits(&self) -> &[nsStyleUnit; 8usize] { &self.mUnits }
+    pub unsafe fn get_mUnits_mut(&mut self) -> &mut [nsStyleUnit; 8usize] {
+        &mut self.mUnits
+    }
+    #[inline]
+    pub unsafe fn get_mValues(&self) -> &[nsStyleUnion; 8usize] {
+        &self.mValues
+    }
+    pub unsafe fn get_mValues_mut(&mut self) -> &mut [nsStyleUnion; 8usize] {
+        &mut self.mValues
+    }
 }
 #[test]
 fn bindgen_test_layout_nsStyleCorners() {
diff --git a/ports/geckolib/gecko_bindings/tools/regen.py b/ports/geckolib/gecko_bindings/tools/regen.py
index 4c3d4e5..c7fd581 100755
--- a/ports/geckolib/gecko_bindings/tools/regen.py
+++ b/ports/geckolib/gecko_bindings/tools/regen.py
@@ -94,7 +94,8 @@ COMPILATION_TARGETS = {
             "imgRequestProxy", "imgRequestProxyStatic", "CounterStyleManager",
             "ImageValue", "URLValue", "URLValueData", "nsIPrincipal",
             "nsDataHashtable", "imgIRequest"
-        ]
+        ],
+        "unsafe_field_types": ["nsStyleUnion", "nsStyleUnit"],
     },
     # Generation of the ffi bindings.
     "bindings": {
@@ -130,7 +131,7 @@ COMPILATION_TARGETS = {
         ],
         "void_types": [
             "nsINode", "nsIDocument", "nsIPrincipal", "nsIURI",
-        ]
+        ],
     }
 }

@@ -253,6 +254,11 @@ def build(objdir, target_name, kind_name=None,
             flags.append("-match")
             flags.append(header.format(objdir))

+    if "unsafe_field_types" in current_target:
+        for ty in current_target["unsafe_field_types"]:
+            flags.append("-unsafe-field-type")
+            flags.append(ty.format(objdir))
+
     if "blacklist" in current_target:
         for ty in current_target["blacklist"]:
             flags.append("-blacklist-type")

emilio · 2016-07-20T17:38:05Z

Hmm... I don't feel like there's a clear use case for this. I mean... This only changes those fields to have accesors, but I feel the approach is flaky in some sense (I mean, doing it type-based instead of on individual fields).

I... don't like that much this approach, but I'd love to know the opinion of other people... @nox?

Manishearth · 2016-07-20T17:51:00Z

Could do it on the basis of individual fields too. Usually unsafe fields are ones with additional invariants, and that is often a property of the field's type. But not always. Perhaps having it blacklist type,fieldname pairs makes more sense.

nox · 2016-07-21T09:38:23Z

Are these fields actually public on the Gecko's side to begin with?

Manishearth · 2016-07-21T10:05:16Z

Yes.

But C++ is very #yolo about safety 😄. And there's no safe way to encapsulate/represent a tagged union without using things like boost::variant or compromising performance, so it's not encapsulated. We have the ability to do this in Rust, so we should. The nsStyleUnion safety wrapper isn't so useful if you can directly modify the underlying union/unit without any safety checks.

Manishearth · 2016-07-22T07:39:35Z

I changed it so that you specify type/field name pairs instead of just types.

emilio · 2016-07-22T07:58:38Z

@nox, yes, all fields are public.

@Manishearth the intrinsic problem I see with this approach, is that you can still access them the same way (only with a getter instead of a direct field access). Making them definitely private is not feasible, because then you can't build on top of them, but making them private with a public accessor seems like a extra layer of complexity that I'm not sure it's worth it.

My point is, the only advantage of doing this is you are forced to write a unsafe block over the getter, but given that the code that has to deal with these structs is also mostly unsafe, I'm not sure it's a win in real benefit vs. maintainability of bindgen and the generated bindings (i.e., it's more code we have to migrate each time rustc breaks libsyntax, plus more flags to keep in sync in the bindgen scripts).

I don't really want to make a final decision without knowing the opinion of @nox, and also of @bholley, because my opinion on the utility of this could very well be wrong.

emilio · 2016-07-22T07:59:50Z

@nox, and with "all fields are public", I mean those fields are public within the geckolib crate, though it's not exposed anywhere else.

Manishearth · 2016-07-22T08:02:26Z

@emilio The point is that it lets us write completely-safe abstraction layers around these structs.

Right now, the whole CoordData layer can be completely bypassed by directly writing to mUnit and mValue. Indeed, we actually do that for z-index (which I will fix).

With this approach, the CoordData bindings will use the unsafe methods to get fields, and the rest of the stylo code will never directly touch these fields.

My point is, the only advantage of doing this is you are forced to write a unsafe block over the getter, but given that the code that has to deal with these structs is also mostly unsafe,

The benefit is that you can't accidentally modify these fields from non-sugar code.

emilio · 2016-07-22T08:18:09Z

Right now, the whole CoordData layer can be completely bypassed by directly writing to mUnit and mValue. Indeed, we actually do that for z-index (which I will fix).

FWIW, modifying mValue is an unsafe operation, since you need to use __BindgenUnionField (unless you modify the _bindgen_data field directly, that btw I would like to make private, though on the other hand might be useful to some people).

And don't take me wrong, I agree it's a somewhat useful feature, but I think it's would not pull its weight in terms of real benefit vs maintainability of the codebase in general.

Anyway, I really wouldn't like to make the final decision on this, because my preference is not that strong. Ideally, I'd like this to be at parse level, so we depend less on libsyntax, and not add a fixed cost to it, but (with my limited knowledge of the visitor API libsyntax exposes) looks good to me otherwise (except I don't see the field identifier check, only the type name).

Manishearth · 2016-07-22T08:19:51Z

FWIW, modifying mValue is an unsafe operation, since you need to use __BindgenUnionField (unless you modify the _bindgen_data field directly, that btw I would like to make private, though on the other hand might be useful to some people).

Not mUnit though. Nothing stops me from setting mUnit to Calc and watching the segfaults rain down upon us 😄

I removed the visitor thing btw, now it's a simple string comparison. The visitor was necessary because it used to be "all fields containing things which contain this type", now it's simply "tell me which type/field pairs to privatize"

emilio · 2016-07-22T08:38:02Z

IRC discussions always solve problems: http://logs.glob.uno/?c=mozilla%23servo&s=22+Jul+2016&e=22+Jul+2016#c486112

TLDR:

Manishearth emilio: I'm not sure what you mean in your comment there
08:03   Manishearth The code that deals with these structs is not mostly unsafe, that's the point of CoordData
08:04   jgraham Manishearth: Doesn't include the servo-specific tests, I think?
08:04   Manishearth jgraham: yeah
08:04   Manishearth sadly
08:11   Ms2ger  jgraham, and is broken with a lack of string formatting somewhere, iirc
08:18   emilio  Manishearth: yeah, I mean most of the functions that deal with them are
08:19   emilio  Manishearth: and that, fwiw, accessing the union is unsafe, the only *really* problematic thing IMO is changing the unit.
08:20   emilio  Manishearth: I still see the visitor in the PR btw
08:25   emilio  Manishearth: but what I mean is that while I don't necessarily dislike the idea, I don't think the advantages of having that feature are more that the disadvantages of maintaining it, over all taking into account more complex cases.
08:25   Manishearth emilio: gone now
08:25   Manishearth emilio: what complex cases
08:25   Manishearth ?
08:25   Manishearth emilio: and how else can we solve the problem of the holes in our safety?
08:26   emilio  Manishearth: anonymous structs/unions if you look at the struct name/field lookup, template parameters if you look at the typename-based lookup.
08:28   Manishearth emilio: ah
08:28   Manishearth hm
08:28   emilio  Manishearth: the ideal way would be something like wrapping every gecko struct in a safe container struct, and add methods on it. In fact, we sort-of can do that, though most of the code is going to be inside those structs so it doesn't really apply.
08:29   Manishearth emilio: in the case of anonymous inner strucs we can still use this support by making fields of that kind unsafe to access and writing a wrapper layer
08:29   Manishearth but I agree it isn't perfect
08:29   Manishearth wrapping every gecko struct works too i guess
08:29   emilio  Manishearth: other problem with the type-based lookup is a really simple use-case for this: what if you want to forbid access to a field that is a file descriptor, should we just make private all the int types?
08:30   emilio  Manishearth: Other thing we can do (there's already support for it), is marking those specific fields with the mutable c++ qualifier (if we can). That would wrap them in a Cell type when possible
08:31   emilio  Manishearth: or adding a new annotation /* <div rustbindgen private></div> */ ? that applies to field
08:31   emilio  fields
08:31   emilio  Manishearth: those are most-robust solutions IMHO
08:32   emilio  Manishearth: in fact if you *really* want this I can try to add support for the later tomorrow or over the weekind
08:32   emilio  *weekend
08:33   emilio  Manishearth: does that sound right? Even though it can be a bit noisy in the C++ side :/
08:33   Manishearth emilio: type based lookup is gone, anyway :)
08:33   Manishearth emilio: rust-bindgen private wfm
08:35   emilio  Manishearth: ok, I can add support for that
08:35   Manishearth emilio: and in that case you'd generate unsafe getters I suppose?
08:36   emilio  Manishearth: yeah, the gen.rs thing is basically the same code you've written, you only need to change the check for |if field.private| or whatever

Manishearth · 2016-07-22T09:17:28Z

Made it use a div. Haven't written a test yet.

bholley · 2016-07-22T17:14:41Z

I think it would also be useful to be able to specify that certain types and fields don't have getters and/or setters at all. For example, annotating a struct as having private members by default, and then annotating specific methods as having a getter and/or setter.

We could do this for nsIAtom, making the only accessor a getter for mHash. This would avoid the need for @SimonSapin to use a somewhat-confusing newtype there.

Manishearth · 2016-07-25T12:34:25Z

Updated. I also add an explicit passthrough of the DYLD_LIBRARY_PATH path in the python file, since make on my machine unsets it.

@SimonSapin does this look more in line with what you need? Privacy and accessors are now independent, and you can control both the safety of the accessors and whether or not the setter (mutable accessor) exists.

emilio · 2016-07-25T18:30:51Z

src/parser.rs

@@ -122,6 +122,7 @@ fn decl_name(ctx: &mut ClangParserCtx, cursor: &Cursor) -> Global {
            CXCursor_ClassTemplate |
            CXCursor_ClassDecl |
            CXCursor_StructDecl => {
+                let anno = Annotations::new(&cursor);


Hmm... This is for nested structs right? Can you add a test for this?

No, this is for annotating the entire struct with something. See the test.

Hmm... I'm pretty sure we parse that at another place. huh. I'll take a look.

That's visit_top, which AFAICT is only called for toplevel decls. decl_name otoh is called for all decls

The only thing visit_top shouldn't visit (for a class/struct/union decl), are nested structs, when we directly call visit_composite IIRC.

So I'm fine with moving it here, but there's no point in parsing it twice, that's what I wanted to say. Nontheless, I think that would bring a bit of churn here right? I'd be satisfied if you file an issue so I can refactor that when I have the time.

emilio · 2016-07-25T18:38:35Z

Seems like you didn't update all the expectations changed (the git diff-index --quiet HEAD line in the build failed), but the logic here seems sound. I'd like to take a last look when the issues are fixed before approving, but thanks for doing this! :)

Can you please add a test for the replace annotation and this one on a struct declaration? I think they should just work fine together, but worth double-checking.

Manishearth · 2016-07-26T09:20:26Z

Replace works.

The failures seem to be in unrelated tests and happen on master too.

Manishearth · 2016-07-26T09:34:18Z

Huh, seems like the tests results are different locally.

emilio · 2016-07-26T11:26:55Z

.travis.yml

@@ -42,6 +42,7 @@ script:
  - cargo build --verbose --features llvm_stable
  - make test
  - git add -A
+  - git diff @
  - git diff-index --quiet HEAD


You can do the same removing the --quiet here

No, git diff-index only gives an error on empty diffs if run with --quiet.

(It also outputs object ids, not diffs)

emilio · 2016-07-26T11:28:14Z

r=me if the difference between travis and your local computer isn't huge or related to this.

Manishearth · 2016-07-26T11:44:25Z

@bors-servo r+

It's unrelated.

bors-servo · 2016-07-26T11:44:26Z

📌 Commit 0fcf367 has been approved by Manishearth

bors-servo · 2016-07-26T11:44:27Z

⌛ Testing commit 0fcf367 with merge 5d7c4d7...

@emilio

Add support for 'unsafe fields' Needed for the next step of servo/servo#12521 These are fields which are private but have unsafe accessor functions. Since we generate bindings in a separate module, we can't touch private fields from other parts of the module (an alternative is to inject a footer with these private impls). `pub(restricted)` exists, but is not stable. r? @emilio

Manishearth · 2016-07-26T11:45:14Z

@bors-servo r=emilio

bors-servo · 2016-07-26T11:45:16Z

💡 This pull request was already approved, no need to approve it again.

This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

bors-servo · 2016-07-26T11:45:16Z

📌 Commit 0fcf367 has been approved by emilio

bors-servo · 2016-07-26T11:49:34Z

☀️ Test successful - travis

highfive assigned emilio Jul 20, 2016

highfive added the S-awaiting-review label Jul 20, 2016

Manishearth mentioned this pull request Jul 20, 2016

Introduce safer layer of sugar for nsStyleUnion servo/servo#12521

Merged

Manishearth force-pushed the unsafe-field branch from fb65a21 to ee9fec6 Compare July 22, 2016 07:39

Manishearth force-pushed the unsafe-field branch from ee9fec6 to f6b4c11 Compare July 22, 2016 08:25

Manishearth force-pushed the unsafe-field branch from f6b4c11 to 4c2f588 Compare July 22, 2016 09:14

bholley mentioned this pull request Jul 22, 2016

[Do not merge, partial change to discuss] Have gecko_string_cache use &Atom instead of BorrowedAtom servo/servo#12548

Closed

SimonSapin mentioned this pull request Jul 23, 2016

Update to selectors 0.8.2 (generic over atoms) servo/servo#12571

Merged

5 tasks

Manishearth force-pushed the unsafe-field branch 4 times, most recently from 57bb6df to a3ea659 Compare July 25, 2016 12:31

emilio reviewed Jul 25, 2016
View reviewed changes

Manishearth force-pushed the unsafe-field branch 2 times, most recently from d541afb to 33fa6ef Compare July 26, 2016 09:19

Make it possible to run tests on El Capitan

bee3117

Manishearth force-pushed the unsafe-field branch 2 times, most recently from a3dec17 to 0fcf367 Compare July 26, 2016 09:53

Manishearth added 2 commits July 26, 2016 15:24

Output diff from travis

7ab0e2c

Add support for 'unsafe fields'

0fcf367

Manishearth mentioned this pull request Jul 26, 2016

Share annotations between visit_top and decl_name #26

Closed

emilio reviewed Jul 26, 2016
View reviewed changes

highfive assigned Manishearth and unassigned emilio Jul 26, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 26, 2016

highfive assigned emilio and unassigned Manishearth Jul 26, 2016

bors-servo merged commit 0fcf367 into rust-lang:master Jul 26, 2016

highfive removed the S-awaiting-merge label Jul 26, 2016

Manishearth deleted the unsafe-field branch July 26, 2016 12:23

Add support for 'unsafe fields' #20

Add support for 'unsafe fields' #20

Conversation

Manishearth commented Jul 20, 2016

highfive commented Jul 20, 2016

highfive commented Jul 20, 2016

Manishearth commented Jul 20, 2016

emilio commented Jul 20, 2016

Manishearth commented Jul 20, 2016

nox commented Jul 21, 2016

Manishearth commented Jul 21, 2016

Manishearth commented Jul 22, 2016

emilio commented Jul 22, 2016

emilio commented Jul 22, 2016

Manishearth commented Jul 22, 2016

emilio commented Jul 22, 2016

Manishearth commented Jul 22, 2016

emilio commented Jul 22, 2016

Manishearth commented Jul 22, 2016

bholley commented Jul 22, 2016

Manishearth commented Jul 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Jul 25, 2016

Manishearth commented Jul 26, 2016

Manishearth commented Jul 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Jul 26, 2016

Manishearth commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016

Manishearth commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016