1295762 - Implement rust bindings to XPCOM's string types

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Implement rust bindings to XPCOM's string types

This patch could definitely use more tests, but I ran out of time to write them today.

Looking at the tests I've added so far should give you an idea of what this patch acts like.

Manish, any chance you could take a look and see if there are any glaring safety holes?

Comment 2

[Manish Goregaokar [:manishearth]]

Comment on attachment 8781736 [details] [diff] [review]
Implement rust bindings to XPCOM's string types

Review of attachment 8781736 [details] [diff] [review]:
-----------------------------------------------------------------

Safety wise looks good. Bit concerned about the lifetime on String; we need to be careful to not dangle that in APIs. Usually elision bails out when this happens so it's usually ok.

I feel like we need a *lot* of doc comments here, so that folks not aware of Gecko types can still use these types safely.

::: xpcom/rust/nsstring/src/lib.rs
@@ +53,5 @@
> +    pub const F_CLASS_FIXED: u32 = 1 << 16;
> +}
> +
> +/// This type is zero-sized and uninstantiable. It is used in the definition of
> +/// AString and ACString to 

nit: missing something

@@ +71,5 @@
> +        StringRepr = $StringRepr: ident;
> +        FixedStringRepr = $FixedStringRepr: ident;
> +        AutoStringRepr = $AutoStringRepr: ident;
> +    } => {
> +        /// The FFI safe type for ns[C]String inside of structs. Use this inside

Can these doc comments actually mention what the type is, with details on how it's used? I'm unfamiliar with the AString stuff and I had to look it all up in dxr. And there are no comments on these types in dxr. Great.

I presume that a portion of pure-Stylo/etc contributors will have similar issues.

@@ +75,5 @@
> +        /// The FFI safe type for ns[C]String inside of structs. Use this inside
> +        /// `#[repr(C)]` structs, containing the C++ ns[C]String type as it has
> +        /// the correct shape.
> +        ///
> +        /// This struct will leak its data if dropped from rust.

Perhaps include a destructor bomb in debug mode?

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8781736 [details] [diff] [review]
Implement rust bindings to XPCOM's string types

Review of attachment 8781736 [details] [diff] [review]:
-----------------------------------------------------------------

Seems mostly reasonable.

::: xpcom/rust/nsstring/gtest/Test.cpp
@@ +9,5 @@
> +
> +  nsAutoString bar;
> +  bar.AssignASCII("Hello, World!");
> +
> +  Rust_StringFromCpp(&foo, &bar);

It's sort of odd that the Rust code is doing the checks here (and other places below).  Can we move the asserts into C++ and have the Rust cool return bools?  (This will require separate tests for CStrings and Strings here.)

::: xpcom/rust/nsstring/src/lib.rs
@@ +11,5 @@
> +//! `string::nsFixed[C]String` implement `Drop`, which means that they have
> +//! implicit drop flags, which changes their layout compared to the string types
> +//! found in Gecko. The types which should be used in FFI are defined in
> +//! `string::ffi`, and should be easy to convert to and from the helper types
> +//! `ns[C]String` and `nsFixed[C]String`.

This caveat sounds like a recipe for disaster, if somebody's using the wrong types--and to be explicit, we should say "in FFI functions and #[repr(C)] structs", correct?  How easy is it for that to happen?  Is `Drop` at least guaranteed to add implicit members after all the declared ones, so we can at least fake layout compatibility?

@@ +17,5 @@
> +//! NOTE: ns[C]String stores length as u32 instead of usize. This should not
> +//! be a soundness problem, as if the string is larger than the max size
> +//! of a u32, it will just be truncated, however it may have unexpected
> +//! effects on very long strings. No attempt is made to check these
> +//! bounds for performance reasons.

This comment is saying that if we convert Rust strings into ns*Strings, we might produce "unexpected" results?  That doesn't sound good at all.  Security first, then performance.  I think we should add the checks...

@@ +160,5 @@
> +        /// this type, which provides the useful operations on strings.
> +        ///
> +        /// NOTE: Rust thinks this type has a size of 0, because the data
> +        /// associated with it is not necessarially safe to move. It is not saf
> +        /// eto construct a nsAString yourself, unless it is received by

Looks like comment wrapping did something wrong here.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8781736 [details] [diff] [review]
Implement rust bindings to XPCOM's string types

Review of attachment 8781736 [details] [diff] [review]:
-----------------------------------------------------------------

Sigh, bugzilla.

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8781736 [details] [diff] [review]
Implement rust bindings to XPCOM's string types

Review of attachment 8781736 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/rust/nsstring/src/lib.rs
@@ +11,5 @@
> +//! `string::nsFixed[C]String` implement `Drop`, which means that they have
> +//! implicit drop flags, which changes their layout compared to the string types
> +//! found in Gecko. The types which should be used in FFI are defined in
> +//! `string::ffi`, and should be easy to convert to and from the helper types
> +//! `ns[C]String` and `nsFixed[C]String`.

For Drop we need to either add unsafe_no_drop_flag or wait a few releases for the drop flag to go away. There's no guarantee otherwise.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8781736 [details] [diff] [review]
Implement rust bindings to XPCOM's string types

Review of attachment 8781736 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/rust/nsstring/src/lib.rs
@@ +3,5 @@
> +//! NOTE: This doesn't yet have the full support for arbitrary xpcom string
> +//! types embedded in C++ structs. Currently, the only types supported are
> +//! `ffi::ns[C]String`, `ffi::nsFixed[C]String`, and `ffi::nsAuto[C]String`.
> +//! Other types can be added on a case-by-case basis, or bindings for them can
> +//! be autogenerated once we get support for rust-bindgen in tree

Nit: Needs trailing period.

@@ +11,5 @@
> +//! `string::nsFixed[C]String` implement `Drop`, which means that they have
> +//! implicit drop flags, which changes their layout compared to the string types
> +//! found in Gecko. The types which should be used in FFI are defined in
> +//! `string::ffi`, and should be easy to convert to and from the helper types
> +//! `ns[C]String` and `nsFixed[C]String`.

Presumably we can just make the structs with the drop flags not coerce to *nsString, no? I don't see why this needs to be a footgun.

@@ +17,5 @@
> +//! NOTE: ns[C]String stores length as u32 instead of usize. This should not
> +//! be a soundness problem, as if the string is larger than the max size
> +//! of a u32, it will just be truncated, however it may have unexpected
> +//! effects on very long strings. No attempt is made to check these
> +//! bounds for performance reasons.

Yeah can we just panic if this happens?

@@ +49,5 @@
> +    pub const F_LITERAL: u32 = 1 << 5;
> +
> +    // class flags are in the upper 16-bits
> +    // indicates that |this| is of type nsTFixedString
> +    pub const F_CLASS_FIXED: u32 = 1 << 16;

We need to somehow assert that these match so we don't get screwed if they get rearranged.

One approach, would would probably have a lot of general utility for stylo, would be to hook things up so that we can preprocess the gtests for rust modules with rust. This would allow us to spit out a bunch of static_asserts that we could then compile into the gtests. Something like this would be helpful for our style struct bindings as well.

Long-term it would be great to just access these over generated-at-builtime rust-bindgen bindings. But we don't have the capability to do that yet.

@@ +81,5 @@
> +        pub struct $StringRepr {
> +            data: *const $char_t,
> +            length: u32,
> +            flags: u32,
> +        }

This needs some static asserts as well.

@@ +125,5 @@
> +                &mut self.base
> +            }
> +        }
> +
> +        /// The FFI safe type for nsAuto[C]String inside of structs. Use this

Do we ever actually have fixed or auto strings inside of structs? I'm skeptical this this is ever a good thing, and that the extra binding glue here is worth it.

::: xpcom/string/nsReadableUtils.cpp
@@ +1245,5 @@
>  }
> +
> +extern "C" {
> +
> +void Gecko_AppendUtf16ToCString(nsACString* aThis, const nsAString* aOther)

Please use the same capitalization convention as our other APIs (Gecko_AppendUTF16toCString)

Comment 7

[Nika Layzell [:nika] (ni? for response)]

I'll add more clarification as to what the deal is with FFI-safe vs non-FFI safe types. It's pretty straightforward, and if you try to pass a non-FFI safe type across the FFI boundary (which you usually won't try to do), you should get an improper_ctypes warning from rust.

I'll add much clearer documentation in the next version I put up. I think that the API has remarkably few footguns, and that I just haven't explained it well enough yet :).

Comment 8

[Manish Goregaokar [:manishearth]]

Inline drop flags will die in 1.13 https://github.com/rust-lang/rust/pull/35764

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Manish Goregaokar [:manishearth] from comment #8)
> Inline drop flags will die in 1.13
> https://github.com/rust-lang/rust/pull/35764

*celebration*

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6) 
> @@ +49,5 @@
> > +    pub const F_LITERAL: u32 = 1 << 5;
> > +
> > +    // class flags are in the upper 16-bits
> > +    // indicates that |this| is of type nsTFixedString
> > +    pub const F_CLASS_FIXED: u32 = 1 << 16;
> 
> We need to somehow assert that these match so we don't get screwed if they
> get rearranged.
> 
> One approach, would would probably have a lot of general utility for stylo,
> would be to hook things up so that we can preprocess the gtests for rust
> modules with rust. This would allow us to spit out a bunch of static_asserts
> that we could then compile into the gtests. Something like this would be
> helpful for our style struct bindings as well.
> 
> Long-term it would be great to just access these over generated-at-builtime
> rust-bindgen bindings. But we don't have the capability to do that yet.
> 
> @@ +81,5 @@
> > +        pub struct $StringRepr {
> > +            data: *const $char_t,
> > +            length: u32,
> > +            flags: u32,
> > +        }
> 
> This needs some static asserts as well.

I don't know of a nice way to implement these asserts yet. For now, I think we have to just assert that they are equivalent, and test to make sure that the code interop works by using these interfaces in GTests. I'm not sure of a better way to handle this.

> 
> @@ +125,5 @@
> > +                &mut self.base
> > +            }
> > +        }
> > +
> > +        /// The FFI safe type for nsAuto[C]String inside of structs. Use this
> 
> Do we ever actually have fixed or auto strings inside of structs? I'm
> skeptical this this is ever a good thing, and that the extra binding glue
> here is worth it.

Removed in the next version

(In reply to Manish Goregaokar [:manishearth] from comment #2)
> Safety wise looks good. Bit concerned about the lifetime on String; we need
> to be careful to not dangle that in APIs. Usually elision bails out when
> this happens so it's usually ok.

The string lifetimes will probably not appear often in C++ APIs. It is mostly
to ensure the safety of strings within rust code (and to allow for allocation-free
conversion of rust string slices into borrowed nsA[C]Strings which can be passed to C++)

> I feel like we need a *lot* of doc comments here, so that folks not aware of
> Gecko types can still use these types safely.

> @@ +71,5 @@
> > +        StringRepr = $StringRepr: ident;
> > +        FixedStringRepr = $FixedStringRepr: ident;
> > +        AutoStringRepr = $AutoStringRepr: ident;
> > +    } => {
> > +        /// The FFI safe type for ns[C]String inside of structs. Use this inside
> 
> Can these doc comments actually mention what the type is, with details on
> how it's used? I'm unfamiliar with the AString stuff and I had to look it
> all up in dxr. And there are no comments on these types in dxr. Great.
> 
> I presume that a portion of pure-Stylo/etc contributors will have similar
> issues.

I've added lots of comments to the module documentation trying to explain what each of these types do, along with a quick summary of when to use which type.

> @@ +75,5 @@
> > +        /// The FFI safe type for ns[C]String inside of structs. Use this inside
> > +        /// `#[repr(C)]` structs, containing the C++ ns[C]String type as it has
> > +        /// the correct shape.
> > +        ///
> > +        /// This struct will leak its data if dropped from rust.
> 
> Perhaps include a destructor bomb in debug mode?

Can't do that, as it will break the layout of the structs which this type is in.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Implement rust bindings to XPCOM's string types

I've been really busy over the last while working on some pop-up regressions so I haven't been able to work on this. I'll try to get more work done tomorrow, but I've tried to address your comments and add a lot of docs.

I'll add more tests tomorrow including potentially some variant on the validation for constants and struct layout.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Michael Layzell [:mystor] from comment #9)
> (In reply to Manish Goregaokar [:manishearth] from comment #8)
> > @@ +81,5 @@
> > > +        pub struct $StringRepr {
> > > +            data: *const $char_t,
> > > +            length: u32,
> > > +            flags: u32,
> > > +        }
> > 
> > This needs some static asserts as well.
> 
> I don't know of a nice way to implement these asserts yet. For now, I think
> we have to just assert that they are equivalent, and test to make sure that
> the code interop works by using these interfaces in GTests. I'm not sure of
> a better way to handle this.

Surely we can implement the offsetof macro in Rust somehow (cf https://github.com/Diggsey/rust-field-offset/blob/master/src/lib.rs) and compare that to the values in C++?  Not perfect, but similar to things we do elsewhere when we want to assert layout equivalency of structs.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #9)
> > This needs some static asserts as well.
> 
> I don't know of a nice way to implement these asserts yet. For now, I think
> we have to just assert that they are equivalent, and test to make sure that
> the code interop works by using these interfaces in GTests. I'm not sure of
> a better way to handle this.

What's wrong with my proposal to generate static_asserts with a build-time script and then #include them in the gtest?

Comment 13

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12)
> (In reply to Michael Layzell [:mystor] from comment #9)
> > > This needs some static asserts as well.
> > 
> > I don't know of a nice way to implement these asserts yet. For now, I think
> > we have to just assert that they are equivalent, and test to make sure that
> > the code interop works by using these interfaces in GTests. I'm not sure of
> > a better way to handle this.
> 
> What's wrong with my proposal to generate static_asserts with a build-time
> script and then #include them in the gtest?

I'll try to do something similar to this, potentially verifying the layouts dynamically. I'll add the layout verification logic into a separate patch on this bug, because I feel like it might make the original patch more complex to review.

Comment 14

[Manish Goregaokar [:manishearth]]

Comment on attachment 8782643 [details] [diff] [review]
Implement rust bindings to XPCOM's string types

Review of attachment 8782643 [details] [diff] [review]:
-----------------------------------------------------------------

Documentation makes it much clearer, thanks.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Implement rust bindings to XPCOM's string types

MozReview-Commit-ID: 7fnWSc3AzlR

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add tests to ensure that the layout of rust's ns[C]String matches C++'s

Sorry for how long it took to get these patches out! I had them written 5 days ago, and got distracted with fixing a crash before I could post the patches.

Bholley, is this an OK way to test the layouts?

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Fast branch for ASCII

It seems this binding would still involve lots of UTF-8 to UTF-16 conversion (in impl fmt::Write for nsAString). I guess it may be worth adding a branch to take the benefit of the fast 8to16 conversion, so I wrote this patch.

Not sure whether it's really worth, though.

Comment 18

[Nika Layzell [:nika] (ni? for response)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> Created attachment 8784624 [details] [diff] [review]
> Fast branch for ASCII
> 
> It seems this binding would still involve lots of UTF-8 to UTF-16 conversion
> (in impl fmt::Write for nsAString). I guess it may be worth adding a branch
> to take the benefit of the fast 8to16 conversion, so I wrote this patch.
> 
> Not sure whether it's really worth, though.

I'm not sure how much faster that will be than just directly converting.

We could also have a method on `nsAString` which appends a `&[u8]` quickly and asserts that each of the fields is a valid ASCII character. This method could be passed a `b"string"`, and be faster in that situation. I'm not sure whether or not this is worthwhile immediately. I'd probably hold off on adding this until we profile stylo using the new string APIs and discover that a lot of time is spent in the UTF8->16 conversions, and that optimizations need to be made.

In the future, when we get procedural macros in stable rust, we should be able to add features like `utf16!("string")` to get `&[u16]` string literals, and potentially even a `writeutf16!()` macro which acts like write! except that it works directly on utf16 strings. This would likely greatly reduce our dependence on these UTF-8 to UTF-16 conversions. This is quite a bit further down the line though.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8784095 [details] [diff] [review]
Part 2: Add tests to ensure that the layout of rust's ns[C]String matches C++'s

Review of attachment 8784095 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I missed this, it got buried.

It seems like there's a lot of repeated code across the different tests - is there any reason this can't be replaced with some macros?

It really seems like a shame to do this dynamically. I would much prefer for us to be generating a file with static_asserts during the export phase that gets #included in the gtests. But I guess that means we need to figure out how to have an export phase cargo build step, which is something we'll probably sort out later with build-time bindgen.

Comment 20

[Nathan Froyd [:froydnj]]

Comment on attachment 8784095 [details] [diff] [review]
Part 2: Add tests to ensure that the layout of rust's ns[C]String matches C++'s

Review of attachment 8784095 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/rust/nsstring/gtest/Test.cpp
@@ +17,5 @@
> +                                                size_t* lengthOff,
> +                                                size_t* lengthSize,
> +                                                size_t* flagsOff,
> +                                                size_t* flagsSize);
> +TEST(RustNsString, StringRepr) {

I'm sure there's got to be a better, more gtest-y way to write these tests.  But as these are ideally getting replaced with something else that doesn't involve so much code duplication...I guess we can let this stand.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8784093 [details] [diff] [review]
Part 1: Implement rust bindings to XPCOM's string types

Review of attachment 8784093 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the question below answered.

::: xpcom/rust/nsstring/src/lib.rs
@@ +1,1 @@
> +//! This module provides rust bindings for the XPCOM string types.

Thank you for this detailed comment.

@@ +4,5 @@
> +//!
> +//! Use `&{mut,} nsA[C]String` for functions in rust which wish to take or
> +//! mutate XPCOM strings. The other string types `Deref` to this type.
> +//!
> +//! Use `ns[C]String<'a>` for string struct members which don't leave C++, and

Is this supposed to be "...which don't leave Rust"?  The wording doesn't make sense to me otherwise.

@@ +14,5 @@
> +//! Use `nsFixed[C]String` or `ns_auto_[c]string!` for dynamic stack allocated
> +//! strings which are expected to hold short string values.
> +//!
> +//! Use `*{const,mut} nsA[C]String` (`{const,} nsA[C]String*` in C++) for
> +//! function arguments passed across the rust/c++ language boundary.

Nit: let's make this C++, since we capitalize it everywhere else.

Comment 22

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nathan Froyd [:froydnj] from comment #21)
> Comment on attachment 8784093 [details] [diff] [review]
> @@ +4,5 @@
> > +//!
> > +//! Use `&{mut,} nsA[C]String` for functions in rust which wish to take or
> > +//! mutate XPCOM strings. The other string types `Deref` to this type.
> > +//!
> > +//! Use `ns[C]String<'a>` for string struct members which don't leave C++, and
> 
> Is this supposed to be "...which don't leave Rust"?  The wording doesn't
> make sense to me otherwise.

You are correct, my bad.

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to Michael Layzell [:mystor] from comment #22)
> (In reply to Nathan Froyd [:froydnj] from comment #21)
> > Comment on attachment 8784093 [details] [diff] [review]
> > @@ +4,5 @@
> > > +//!
> > > +//! Use `&{mut,} nsA[C]String` for functions in rust which wish to take or
> > > +//! mutate XPCOM strings. The other string types `Deref` to this type.
> > > +//!
> > > +//! Use `ns[C]String<'a>` for string struct members which don't leave C++, and
> > 
> > Is this supposed to be "...which don't leave Rust"?  The wording doesn't
> > make sense to me otherwise.
> 
> You are correct, my bad.

\o/

Is it possible to write some sort of style checker that Rust-only types are used in Rust-only structures, and so forth?  Or do we think strong typing will just take care of that for us?

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Rust will already complain if you do that with an improper_ctypes warning (https://is.gd/rm2YRl)

    warning: found non-foreign-function-safe member in struct marked #[repr(C)]: found struct without foreign-function-safe representation annotation in foreign module, consider adding a #[repr(C)] attribute to the type, #[warn(improper_ctypes)] on by default
      --> <anon>:13:35
       |>
    13 |>     pub fn takes_c_layout_type(x: *const CLayoutType);
       |>                                   ^^^^^^^^^^^^^^^^^^

Custom Rust style checkers (lints) are unstable, so I don't think we will have any in gecko for a while.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30bb58fbbeb8
Part 1: Implement rust bindings to XPCOM's string types, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b5fa96e8ee
Part 2: Add tests to ensure that the layout of rust's ns[C]String matches C++'s, r=froydnj

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=34994590&repo=mozilla-inbound#L21866

https://hg.mozilla.org/integration/mozilla-inbound/rev/41d8277af192

Comment 27

[Nika Layzell [:nika] (ni? for response)]

(In reply to Wes Kocher (:KWierso) from comment #26)
> I had to back these out for build bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=34994590&repo=mozilla-
> inbound#L21866
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/41d8277af192

I'm surprised this didn't fire up locally. It seems like because the crate is used by both xul and the testing infra, it is actually included once.

I'm curious as to why we have a separate rust library for gtest. Could we have this be a cargo feature instead? That way we would avoid having 2x copies of rust code in the build.

Comment 28

[Nathan Froyd [:froydnj]]

(In reply to Michael Layzell [:mystor] from comment #27)
> (In reply to Wes Kocher (:KWierso) from comment #26)
> > I had to back these out for build bustage like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=34994590&repo=mozilla-
> > inbound#L21866
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/41d8277af192
> 
> I'm surprised this didn't fire up locally. It seems like because the crate
> is used by both xul and the testing infra, it is actually included once.

Local builds don't build xul-gtest unless you explicitly do |make -C toolkit/library/ gtestxul|.  I think this is kind of suboptimal, but faster builds and all...

> I'm curious as to why we have a separate rust library for gtest. Could we
> have this be a cargo feature instead? That way we would avoid having 2x
> copies of rust code in the build.

How would it be a cargo feature?

We have run into a problem, though: people are going to want to write gtests such that they include crates in both the regular libxul and libxul-gtest.  Our current linking methodology doesn't support that: we link all the gkrust crates into libgkrust.a, and then stuff that into a libxul.a or equivalent.  But the way we build libxul-gtest is that we start with libxul.a and add in all the gtest-specific stuff, which winds up linking relevant Rust things twice.  Our current Rust-y gtests don't run into this problem, because they have virtually no Rust in them, but these tests do, because they have significant amounts of Rust, even if some of the heavy lifting is farmed out to C++.  And there's no good way to get around them having significant amounts of Rust, AFAICT.

Comment 29

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Feature gate the test_helpers module behind the gtest feature, so that they are only included in the libgkrust-gtest

I think that this should fix the opt-only linking errors on infrastructure

Comment 30

[Nika Layzell [:nika] (ni? for response)]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a0ee43139a8

Comment 31

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8786942 [details] [diff] [review]
Part 3: Feature gate the test_helpers module behind the gtest feature, so that they are only included in the libgkrust-gtest

Scratch that, seems like it only fixed things locally.

Comment 32

[Nika Layzell [:nika] (ni? for response)]

(In reply to Michael Layzell [:mystor] from comment #31)
> Comment on attachment 8786942 [details] [diff] [review]
> Part 3: Feature gate the test_helpers module behind the gtest feature, so
> that they are only included in the libgkrust-gtest
> 
> Scratch that, seems like it only fixed things locally.

The failures remaining seem to me like failures due to panic infrastructure. Manish, is panic=abort supported on the current stable version? It's turned on in our Cargo.tomls, but I believe infra uses stable rustc and unstable cargo atm.

Comment 33

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #32)
> (In reply to Michael Layzell [:mystor] from comment #31)
> > Comment on attachment 8786942 [details] [diff] [review]
> > Part 3: Feature gate the test_helpers module behind the gtest feature, so
> > that they are only included in the libgkrust-gtest
> > 
> > Scratch that, seems like it only fixed things locally.
> 
> The failures remaining seem to me like failures due to panic infrastructure.
> Manish, is panic=abort supported on the current stable version? It's turned
> on in our Cargo.tomls, but I believe infra uses stable rustc and unstable
> cargo atm.

I believe m-c cargo builds with panic=abort.

Comment 34

[Manish Goregaokar [:manishearth]]

Yes, since 1.10: https://blog.rust-lang.org/2016/07/07/Rust-1.10.html

Comment 35

[Nika Layzell [:nika] (ni? for response)]

It seems like we will need to make gtest xul builds not include libgkrust and only include libgkrust-gtest (which would depend on libgkrust inside of cargo perhaps?)

I can't think of another way to prevent us from having 2 copies of the universe inside of our binary. I don't know enough about the build system to fix this in any reasonable amount of time. I was talking with Nathan yesterday and he seems to agree that I will have a hard time with it.

Nathan, do you know who would be able to solve this problem the most quickly / do you have any idea how I would go about making libxul-gtest depend on `static:xul - libgkrust` instead of just `static:xul`? My first thought would be to split out a libxul-cpp which is built statically and then linked into libxul with libgkrust, and libxul-gtest with libgkrust-gtest, but I have no idea how that would be implemented on top of mozbuild.

Comment 36

[Nika Layzell [:nika] (ni? for response)]

I actually have a patch which seems to fix the problem (locally at least). Doing a try push then I'll ask for review.

Comment 37

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Merge libgkrust and libgkrust-gtest into a single rust library

And use dynamic symbol resolution in gtest code to not require pulling in gtest C++ code.
This fixes the linker issues which were present.

It's pretty gross that we build testing code into libxul. I'm hoping that this won't inflate binary size too much because the linker will notice the code is dead and prune it.

This uses dynamic symbol resolution to call from rust gtest code into c++ gtest code, so that the c++ gtest code doesn't need to be present.

Comment 38

[Nathan Froyd [:froydnj]]

Comment on attachment 8787401 [details] [diff] [review]
Part 3: Merge libgkrust and libgkrust-gtest into a single rust library

Review of attachment 8787401 [details] [diff] [review]:
-----------------------------------------------------------------

This is an inspired hack.  Can you verify that the code is present in libxul-gtest, but not in libxul itself?  (You'll have to enable dead code stripping yourself or run the patch through try and whatever, because we don't normally turn it on for local builds; you can pass --enable-release to turn on said option.)

I'm not a huge fan of the patch because it's not a very general solution; anybody else who comes along will run into the same sort of problems you've had, and then we'll have to repeat this.  I'd rather get this issue fixed in the general case so we can not think about it later.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a222c243ea01
Part 1: Implement rust bindings to XPCOM's string types, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/25d7613115f7
Part 2: Add tests to ensure that the layout of rust's ns[C]String matches C++'s, r=froydnj

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a222c243ea01
https://hg.mozilla.org/mozilla-central/rev/25d7613115f7

Comment 41

[(not currently active) Ted Mielczarek]

rillian tried to land a rust update on OS X in bug 1304815 (to fix bug 1301751) and it got backed out for failures like:
https://treeherder.mozilla.org/logviewer.html#?job_id=3922451&repo=autoland#L30237

 19:42:39     INFO -  libxul-gtest-real-rs-prelink.a
 19:42:39     INFO -  /builds/slave/autoland-m64-00000000000000000/build/src/rustc/bin/rustc -o ../../../toolkit/library/gtest/rust/libxul-gtest-real-rs-prelink.a --crate-type staticlib --target x86_64-apple-darwin  --extern gkrust=../../../toolkit/library/rust/x86_64-apple-darwin/release/libgkrust.rlib -L ../../../toolkit/library/rust/x86_64-apple-darwin/release/deps --extern gkrust-gtest=../../../toolkit/library/gtest/rust/x86_64-apple-darwin/release/libgkrust_gtest.rlib -L ../../../toolkit/library/gtest/rust/x86_64-apple-darwin/release/deps -g -C panic=abort -C opt-level=2 -C lto /builds/slave/autoland-m64-00000000000000000/build/src/obj-firefox/x86_64/toolkit/library/gtest/xul-gtest-real-rs-prelink.rs
 19:42:41     INFO -  error[E0523]: found two different crates with name `nsstring` that are not distinguished by differing `-C metadata`. This will result in symbol conflicts between the two.
 19:42:41     INFO -   --> /builds/slave/autoland-m64-00000000000000000/build/src/obj-firefox/x86_64/toolkit/library/gtest/xul-gtest-real-rs-prelink.rs:4:1
 19:42:41     INFO -    |
 19:42:41     INFO -  4 | extern crate gkrust_gtest;
 19:42:41     INFO -    | ^^^^^^^^^^^^^^^^^^^^^^^^^^
 19:42:42     INFO -  make[8]: *** [../../../toolkit/library/gtest/rust/libxul-gtest-real-rs-prelink.a] Error 101
 19:42:42     INFO -  make[7]: *** [toolkit/library/gtest/target] Error 2
 19:42:42     INFO -  make[6]: *** [gtestxul] Error 2
 19:42:42     INFO -  make[5]: *** [gtest] Error 2
 19:42:42 INFO - make[4]: *** [stage-gtest] Error 2

I'm guessing Rust 1.12 doesn't like your shenanigans here somehow? I'm not sure, but we really need to get that re-landed because we don't currently have symbols for OS X builds and that is making a mess of crash reporting.

Comment 42

[(not currently active) Ted Mielczarek]

...that comment came out grumpier than planned. It was early and I am frustrated with the ever-expanding set of issues we keep hitting here. Sorry!

Comment 43

[Nathan Froyd [:froydnj]]

Alex, we're running into a problem in comment 41; Rust 1.11 permits us to link together two rlibs:

libgkrust.rlib
- nsstring
- ...other crates...

libgkrust_gtest.rlib
- nsstring
- ...other crates...

into a staticlib, and it apparently folds the duplicate nsstring crates together--or at least doesn't complain about it.  Rust 1.12 now forbids this behavior; the error message apparently requests that we compile with `-C metadata` for each crate.  What was the problem with the previous behavior, and is it sufficient to simply compile each rlib (and all crates beneath it) with differing `-C metadata` flags?

Comment 44

[Nathan Froyd [:froydnj]]

glandium pointed out on #build that this is more likely that one nsstring crate was recompiled with the new compiler, while the other was not. :(

Comment 45

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #44)
> glandium pointed out on #build that this is more likely that one nsstring
> crate was recompiled with the new compiler, while the other was not. :(

This seems kind of unlikely, as:

https://treeherder.mozilla.org/logviewer.html#?job_id=3922451&repo=autoland

shows that nsstring is getting recompiled for both gkrust and gkrust_gtest.  In fact, due to what I think is our busted build system, we actually compile nsstring *four* times:
  12403:19:08:17     INFO -     Compiling nsstring v0.1.0 (file:///builds/slave/autoland-m64-00000000000000000/build/src/xpcom/rust/nsstring)
  12405:19:08:17     INFO -       Running `/builds/slave/autoland-m64-00000000000000000/build/src/rustc/bin/rustc /builds/slave/autoland-m64-00000000000000000/build/src/xpcom/rust/nsstring/src/lib.rs --crate-name nsstring --crate-type lib -C opt-level=2 -C panic=abort -g -C metadata=07e716181e70f839 --out-dir /builds/slave/autoland-m64-00000000000000000/build/src/obj-firefox/x86_64/toolkit/library/rust/./x86_64-apple-darwin/release/deps --emit=dep-info,link --target x86_64-apple-darwin -L dependency=/builds/slave/autoland-m64-00000000000000000/build/src/obj-firefox/x86_64/toolkit/library/rust/./x86_64-apple-darwin/release/deps`
  12883:19:10:16     INFO -     Compiling nsstring v0.1.0 (file:///builds/slave/autoland-m64-00000000000000000/build/src/xpcom/rust/nsstring)
  12885:19:10:16     INFO -       Running `/builds/slave/autoland-m64-00000000000000000/build/src/rustc/bin/rustc /builds/slave/autoland-m64-00000000000000000/build/src/xpcom/rust/nsstring/src/lib.rs --crate-name nsstring --crate-type lib -C opt-level=2 -C panic=abort -C codegen-units=1 -g -C metadata=07e716181e70f839 --out-dir /builds/slave/autoland-m64-00000000000000000/build/src/obj-firefox/x86_64/toolkit/library/gtest/rust/./x86_64-apple-darwin/release/deps --emit=dep-info,link --target x86_64-apple-darwin -L dependency=/builds/slave/autoland-m64-00000000000000000/build/src/obj-firefox/x86_64/toolkit/library/gtest/rust/./x86_64-apple-darwin/release/deps`
  26325:19:35:30     INFO -     Compiling nsstring v0.1.0 (file:///builds/slave/autoland-m64-00000000000000000/build/src/xpcom/rust/nsstring)
  26327:19:35:30     INFO -       Running `/builds/slave/autoland-m64-00000000000000000/build/src/rustc/bin/rustc /builds/slave/autoland-m64-00000000000000000/build/src/xpcom/rust/nsstring/src/lib.rs --crate-name nsstring --crate-type lib -C opt-level=2 -C panic=abort -g -C metadata=07e716181e70f839 --out-dir /builds/slave/autoland-m64-00000000000000000/build/src/obj-firefox/i386/toolkit/library/rust/./i686-apple-darwin/release/deps --emit=dep-info,link --target i686-apple-darwin -L dependency=/builds/slave/autoland-m64-00000000000000000/build/src/obj-firefox/i386/toolkit/library/rust/./i686-apple-darwin/release/deps`
  26789:19:37:32     INFO -     Compiling nsstring v0.1.0 (file:///builds/slave/autoland-m64-00000000000000000/build/src/xpcom/rust/nsstring)
  26791:19:37:32     INFO -       Running `/builds/slave/autoland-m64-00000000000000000/build/src/rustc/bin/rustc /builds/slave/autoland-m64-00000000000000000/build/src/xpcom/rust/nsstring/src/lib.rs --crate-name nsstring --crate-type lib -C opt-level=2 -C panic=abort -C codegen-units=1 -g -C metadata=07e716181e70f839 --out-dir /builds/slave/autoland-m64-00000000000000000/build/src/obj-firefox/i386/toolkit/library/gtest/rust/./i686-apple-darwin/release/deps --emit=dep-info,link --target i686-apple-darwin -L dependency=/builds/slave/autoland-m64-00000000000000000/build/src/obj-firefox/i386/toolkit/library/gtest/rust/./i686-apple-darwin/release/deps`

You'll note that:

- The `-C metadata` option is the same for all of the compiler invocations.
- We recompile twice for gkrust and twice for gkrust_gtest.

So, this is not a build system problem--at least from the perspective of not recompiling dependencies.  I think the ball is back to Alex to explain what Rust 1.12 broke in this scenario (comment 41, comment 43).

Comment 46

[Nathan Froyd [:froydnj]]

mystor helped me realize that I had my head screwed on backwards: the compilations in comment 45 *do* have identical metadata, which suggests they must *not* have had identical metadata before.  We think this is because compilation was made more determinstic for workspaces, incremental compilation, or just good software engineering.

If so, that would explain why this scheme worked before: we had duplicate nsstring crates and duplicate nsstring.o objects between gkrust and gkrust_gtest, but the actual symbols in those crates and objects presumably didn't have identical names.  And so the linker didn't complain, because it never saw any conflicts.  But now the symbol names would be identical, and rust is helpfully warning us before the linker produces inscrutable errors.

But now it doesn't, and we need to figure out what to do...again.

Comment 47

[Nika Layzell [:nika] (ni? for response)]

I should note that I designed the patch, and have been locally testing it, using the nightly build of rustc, and a nightly cargo. This makes me confused as to what would have changed.

(I also have never had a failure occur during a local build)

Comment 48

[Alex Crichton [:acrichto]]

This is interesting! I think froydnj's analysis in comment 46 is right where this was "silently allowed" before but is now rejected as it was basically just lucky it didn't cause problems before.

The four compilations seem interesting here, but it looks like half are x86_64 and half are i686, so we've actually only got two compilations of the nsstring crate. One has `codegen-units=1` and one doesn't. I wonder, are two rlibs being linked into one staticlib where the rlibs are produced by two different invocations of Cargo?

Failing that, though, could you describe the build process here for what's happening to link everything together? Is this just a top-level Cargo.toml with a ["staticlib"], or perhaps something more flavorful?

Comment 49

[Nathan Froyd [:froydnj]]

(In reply to Alex Crichton [:acrichto] from comment #48)
> The four compilations seem interesting here, but it looks like half are
> x86_64 and half are i686, so we've actually only got two compilations of the
> nsstring crate. One has `codegen-units=1` and one doesn't. I wonder, are two
> rlibs being linked into one staticlib where the rlibs are produced by two
> different invocations of Cargo?
> 
> Failing that, though, could you describe the build process here for what's
> happening to link everything together? Is this just a top-level Cargo.toml
> with a ["staticlib"], or perhaps something more flavorful?

Ah, that's a good point that we're doing one compilation for x86-64 and one compilation for i686.  I did not see that before.  So we're in the clear as to problems in the build system there.

The build process here is as follows:

* We have a crate, gkrust, with type "rlib", built by Cargo
* We have a crate, gkrust_gtest, with type "rlib", built by Cargo
* Both of the above crates use the same crate, nsstring
* When we go to link libxul.so, which contains gkrust, we compile a fake Rust staticlib which includes gkrust.  This library is compiled via manual rustc invocation.  Call this libgkrust-fake.a.
* We link libxul.so with the equivalent of:

  $(CC) -o libxul.so $LIBXUL_OBJECTS libgkrust-fake.a ...other arguments...

* When we go to link libxul-gtest, which contains gkrust and gkrust_gtest, we compile a fake Rust staticlib which includes gkrust and gkrust_gtest.  This library is compiled via manual rustc invocation.  Call this libgkrust_gtest-fake.a.
* We link libxul-gtest with the equivalent of:

  $(CC) -o libxul-gtest.so $LIBXUL_OBJECTS libgkrust-fake.a libgkrust_gtest-fake.a ...other arguments...

We used to do things like so:

* We have a crate, gkrust, with type "staticlib", built by Cargo
* We have a crate, gkrust_gtest, with type "staticlib", built by Cargo
* When we go to link libxul, we pass libgkrust.a on the command line
* When we go to link libxul-gtest, we passed libgkrust.a and libgkrust_gtest.a on the command line

This model broke when including the same crate in both gkrust and gkrust_gtest, however.  We switched to the current model, which works with our current Rust compilers (1.11) and nightlies (according to comment 47), but apparently not with beta.

What it seems Rust really wants us to do is something like:

* We have a crate, gkrust, that gets linked as libgkrust.a into libxul (but gets compiled as an rlib?), built by Cargo.
* We have a crate, gkrust_gtest, that depends on gkrust and is built by Cargo.
* When we go to link libxul, we only link in libgkrust.a
* When we go to link libxul-gtest, we only link in libgkrust_gtest.a

(Hacking -C metadata to try and force the crates to have different symbol names results in rustc internal errors, so that's out.)

Having a setup like this presumably enables rustc to see that nsstring (and any other duplicated crates between gkrust and gkrust_gtest) only needs to be included once in any resulting staticlib; it's not clever enough to see that otherwise.  The only problem is that it's a little tricky to keep libgkrust.a out of the link line for libxul-gtest, for reasons too boring to go into here.

Alex, does that explanation make sense?  Do you have insight into why our setup would work with current Rust and nightly Rust, but not beta Rust?

Comment 50

[Nika Layzell [:nika] (ni? for response)]

(In reply to Alex Crichton [:acrichto] from comment #48)
> This is interesting! I think froydnj's analysis in comment 46 is right where
> this was "silently allowed" before but is now rejected as it was basically
> just lucky it didn't cause problems before.
> 
> The four compilations seem interesting here, but it looks like half are
> x86_64 and half are i686, so we've actually only got two compilations of the
> nsstring crate. One has `codegen-units=1` and one doesn't. I wonder, are two
> rlibs being linked into one staticlib where the rlibs are produced by two
> different invocations of Cargo?

Yup, unfortunately they are. We invoke Cargo to create rlibs, and then invoke rustc directly immediately before the final link in order to merge all of our "top-level" cargo crates which are used within the target executable, passing --extern and -L for each of the crates which we are consuming as dependencies. I believe that the problem is caused by both of the invocations of cargo having their own deps/ directory which contains a nsstring.rlib library, and rustc being unable to decide which one to choose. I believe that this particular problem would go away with workspaces, as they would share the nsstring dependency.

The reason why this is needed is simple: When building gtests we create a custom libxul.so which contains the extra code which is required for testing purposes. We also need to link extra rust code into that .so file. Right now we have a gkrust-gtest cargo crate which we also build, and when doing the link for the custom libxul.so, we link in a custom .a file which links in both gkrust and gkrust-gtest.

This will also be useful if/when we start using rust code in spidermonkey. Spidermonkey needs to be able to both build into a consumable linkable library, as well as link into xul. The easiest way to do that is to allow our build system to control the linking of atomic pieces of rust code into the final libraries. This way we can use mozbuild's global knowledge to make sure we only link in the rust standard library once per final library.

I realize that this is not the intended use of cargo, but I'm not sure what other options we have. Do you have any ideas here?

Comment 51

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nathan Froyd [:froydnj] from comment #49)
> (In reply to Alex Crichton [:acrichto] from comment #48)
> * We link libxul-gtest with the equivalent of:
> 
>   $(CC) -o libxul-gtest.so $LIBXUL_OBJECTS libgkrust-fake.a
> libgkrust_gtest-fake.a ...other arguments...

Correction: We don't link libgkrust-fake.a into libxul-gtest.so, only libgkrust-gtest-fake.a which contains both gkrust.rlib and gkrust-gtest.rlib.

Comment 52

[Alex Crichton [:acrichto]]

Thanks for the info! I think I can see what's going on here as well now, and I think I can also see a possible solution.

> Do you have insight into why our setup would work with current Rust
> and nightly Rust, but not beta Rust?

I believe the crux of the error happens in this spot mystor mentioned:

> and then invoke rustc directly immediately before the final link in order 
> to merge all of our "top-level" cargo crates which are used within the 
> target executable, passing --extern and -L for each of the crates which 
> we are consuming as dependencies

The problem is that the same crate, presumably `nsstring`, is in both the dependency graph for gkrust and gkrust-test. It's then compiled the same `-C metadata` in each dependency graph, but it apparently has a different SVH (strict version hash) according to the compiler. This can happen due to the crate structure changing, such a features, etc. A new check was added to the compiler in 1.12.0 that if crates have the same name and `-C metadata` they must have equal SVH as well. This isn't true, however, as evidenced by the error here:

> error[E0523]: found two different crates with name `nsstring` that are not distinguished by differing `-C metadata`. This will result in symbol conflicts between the two.

So to answer the question of why this works on stable and not beta is that it *may* work on stable and may also hit symbol errors. Right now though there's no symbol errors (for whatever reason). The beta compiler is yielding the error, however. As to why nightly works... I have no idea :(. Are you sure that something like today's nightly works, whereas the beta does not? This may have been a bug fix, but I'm not entirely certain

---

As for a fix, though, I think this isn't quite the right architecture that you'll want. When using Cargo you'll want to stick to the Cargo world as much as possible to have everything "just work" and avoid these kinds of surprises. 

Along those lines, would it be possible to have two Cargo projects, one for gkrust and one for gkrust-test? Both projects would generate a staticlib, and libxul.so would link to libgkrust.a where libxul-test.so would link to libgkrust-test.a.

I may not understand this step though:

> * When we go to link libxul.so, which contains gkrust, we compile a fake Rust staticlib which includes gkrust.  This library is compiled via manual rustc invocation.  Call this libgkrust-fake.a.

so maybe this isn't possible? I'm not sure what this means by a "fake staticlib" which also includes the relevant library.

My thinking though is that the DAG for gkrust and gkrust-test would be very similar (e.g. the Cargo.toml would be very similar).

Does that make sense? Or has that already been ruled out?

Comment 53

[Nathan Froyd [:froydnj]]

(In reply to Alex Crichton [:acrichto] from comment #52)

> The problem is that the same crate, presumably `nsstring`, is in both the
> dependency graph for gkrust and gkrust-test. It's then compiled the same `-C
> metadata` in each dependency graph, but it apparently has a different SVH
> (strict version hash) according to the compiler. This can happen due to the
> crate structure changing, such a features, etc. A new check was added to the
> compiler in 1.12.0 that if crates have the same name and `-C metadata` they
> must have equal SVH as well. This isn't true, however, as evidenced by the
> error here:
> 
> > error[E0523]: found two different crates with name `nsstring` that are not distinguished by differing `-C metadata`. This will result in symbol conflicts between the two.

My question here would be why aren't the SVH equal here, then?  The code has not changed between compiling the two crate versions, so if SVH is supposed to reflect the "version" of the code that has been compiled, different SVH for the same code seems like a bug.  Or am I misunderstanding the purpose of SVH?

> As for a fix, though, I think this isn't quite the right architecture that
> you'll want. When using Cargo you'll want to stick to the Cargo world as
> much as possible to have everything "just work" and avoid these kinds of
> surprises. 
> 
> Along those lines, would it be possible to have two Cargo projects, one for
> gkrust and one for gkrust-test? Both projects would generate a staticlib,
> and libxul.so would link to libgkrust.a where libxul-test.so would link to
> libgkrust-test.a.

We do have two Cargo projects currently.  The wrinkle is that we assume that everything going into libxul.so also goes into libxul-gtest.so, as libxul-gtest.so is supposed to be libxul + test code.

We can try to fix this according to what you suggest, but then when people add Rust code to libxul, then they also have to add that code to gkrust-gtest, or things will not link correctly.  I don't like having to special-case Rust libraries in the build system like this, and force poor ergonomics on people, but perhaps we don't have much of a choice.

ni? to Alex to clear up my question about SVH.

Comment 54

[Nika Layzell [:nika] (ni? for response)]

Here's my work-in-progress idea about how to make having to maintain a separate dependency tree for code which depends on rust code as ergonomic as possible:

If you had rust code in your library, you would write the following in your moz.build:

    RUST_CRATE = path/to/rust/library

A library can, quite explicitly, only contain one rust crate at most because you had to write RUST_CRATE explicitly. You would not put a moz.build file in the directory containing the Cargo.toml. 

The RUST_CRATE will be built as a staticlib through cargo, and linked like normal into the library... unless the library is a static library. Normal static libraries will have the code linked in as normal, but the .a.desc style libraries will not include the staticlib in the .desc file.

Any library which depends on a static library which has RUST_CRATE set will implicitly have the same RUST_CRATE as the base library. If multiple dependencies have RUST_CRATEs, then the library must specify its own RUST_CRATE.

If RUST_CRATE is specified, and there are any dependencies which specify RUST_CRATE, the build system will verify that the dependencies are listed as dependencies in the RUST_CRATE/Cargo.toml. Unfortunately, we probably cannot ensure that these dependencies are actually linked into the final library (as that requires writing `extern crate lib;` in the actual source, which we don't really want to parse. We could potentially scan the lib.rs file and check for the string "libname" and complain if it isn't present?).

It's pretty gross, but might work? What do you think nathan?

Comment 55

[Alex Crichton [:acrichto]]

(In reply to Nathan Froyd [:froydnj] from comment #53)
> My question here would be why aren't the SVH equal here, then?  The code has
> not changed between compiling the two crate versions, so if SVH is supposed
> to reflect the "version" of the code that has been compiled, different SVH
> for the same code seems like a bug.  Or am I misunderstanding the purpose of
> SVH?

Hm, that is indeed interesting! The SVH has been known to be somewhat unstable in the past, but that should be smoothed out now by this point for incremental compilation. All that's basically a fancy way of saying I'm not sure :(. If it's exactly the same code (feature sets, dependencies, everything), then they should in theory be equal.

> We do have two Cargo projects currently.  The wrinkle is that we assume that
> everything going into libxul.so also goes into libxul-gtest.so, as
> libxul-gtest.so is supposed to be libxul + test code.

Does libxul-gtest.so link to libxul.so?

If not, could the gkrust-test project just link to the gkrust project? That'd slurp up everything automatically I think?

> We can try to fix this according to what you suggest, but then when people
> add Rust code to libxul, then they also have to add that code to
> gkrust-gtest, or things will not link correctly. 

Agreed this does indeed sound bad! I'd love to help explore solutions to prevent this.

Comment 56

[Nika Layzell [:nika] (ni? for response)]

(In reply to Alex Crichton [:acrichto] from comment #55)
> (In reply to Nathan Froyd [:froydnj] from comment #53)
> > My question here would be why aren't the SVH equal here, then?  The code has
> > not changed between compiling the two crate versions, so if SVH is supposed
> > to reflect the "version" of the code that has been compiled, different SVH
> > for the same code seems like a bug.  Or am I misunderstanding the purpose of
> > SVH?
> 
> Hm, that is indeed interesting! The SVH has been known to be somewhat
> unstable in the past, but that should be smoothed out now by this point for
> incremental compilation. All that's basically a fancy way of saying I'm not
> sure :(. If it's exactly the same code (feature sets, dependencies,
> everything), then they should in theory be equal.

They _should_ be exactly the same. We do pass the RUSTFLAGS environment variable, but it should be the same for both invocations.

> > We do have two Cargo projects currently.  The wrinkle is that we assume that
> > everything going into libxul.so also goes into libxul-gtest.so, as
> > libxul-gtest.so is supposed to be libxul + test code.
> 
> Does libxul-gtest.so link to libxul.so?

No, it replaces libxul.so, as in it contains all of libxul.so + extra code.

> If not, could the gkrust-test project just link to the gkrust project?
> That'd slurp up everything automatically I think?

That's what I'm suggesting we do in my above comment. It will work but the ergonomics aren't great.

> > We can try to fix this according to what you suggest, but then when people
> > add Rust code to libxul, then they also have to add that code to
> > gkrust-gtest, or things will not link correctly. 
> 
> Agreed this does indeed sound bad! I'd love to help explore solutions to
> prevent this.

Comment 57

[Alex Crichton [:acrichto]]

I think the same strategy was suggested by Ted here as well, right? https://bugzilla.mozilla.org/show_bug.cgi?id=1304815#c16

Comment 58

[(not currently active) Ted Mielczarek]

Yeah, that sounds basically the same. I get the point about the ergonomics--you have this weird situation where one library is linking in another, and they both have Rust code, and now you have to also make sure that the Rust crate in the former pulls in the crate from the latter.

Comment 59

[Nathan Froyd [:froydnj]]

(In reply to Michael Layzell [:mystor] from comment #54)
> Here's my work-in-progress idea about how to make having to maintain a
> separate dependency tree for code which depends on rust code as ergonomic as
> possible:
> 
> If you had rust code in your library, you would write the following in your
> moz.build:
> 
>     RUST_CRATE = path/to/rust/library
> 
> A library can, quite explicitly, only contain one rust crate at most because
> you had to write RUST_CRATE explicitly. You would not put a moz.build file
> in the directory containing the Cargo.toml. 
> 
> The RUST_CRATE will be built as a staticlib through cargo, and linked like
> normal into the library... unless the library is a static library. Normal
> static libraries will have the code linked in as normal, but the .a.desc
> style libraries will not include the staticlib in the .desc file.
> 
> Any library which depends on a static library which has RUST_CRATE set will
> implicitly have the same RUST_CRATE as the base library. If multiple
> dependencies have RUST_CRATEs, then the library must specify its own
> RUST_CRATE.
> 
> If RUST_CRATE is specified, and there are any dependencies which specify
> RUST_CRATE, the build system will verify that the dependencies are listed as
> dependencies in the RUST_CRATE/Cargo.toml. Unfortunately, we probably cannot
> ensure that these dependencies are actually linked into the final library
> (as that requires writing `extern crate lib;` in the actual source, which we
> don't really want to parse. We could potentially scan the lib.rs file and
> check for the string "libname" and complain if it isn't present?).

I think this plan is reasonable, except that you should be able to derive all of this in the mozbuild Python code and you shouldn't need to add any special variables to moz.build files.