Closed Bug 1356150 Opened 8 years ago Closed 8 years ago

stylo: can't compile due to bad bindgen?

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
firefox55 --- affected

People

(Reporter: n.nethercote, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

compile errors
(deleted), text/plain
Details
generated structs_release.rs
(deleted), text/rust
Details
output for comment 15
(deleted), text/plain
Details
Attached file compile errors (deleted) — 
On my Ubuntu Linux64 box I get lots of compile errors from generated code. See the attached file.

I'm using clang 3.9. Here's my mozconfig:

> ac_add_options --with-ccache
> ac_add_options --enable-stylo
> ac_add_options --enable-dmd
> ac_add_options --enable-optimize='-O1'
> 
> mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/o64sty
> mk_add_options MOZ_MAKE_FLAGS="-j8"
> mk_add_options AUTOCLOBBER=1
> 
> export CC=clang-3.9
> export CXX=clang++-3.9
> export LLVM_CONFIG=llvm-config-3.9

I will attach the generated file in a moment.
Attached file generated structs_release.rs (deleted) — 
fitzgen, any ideas what's going wrong?
Flags: needinfo?(nfitzgerald)
Could you delete your obj folder and rebuild again?
I don't know anything concrete from looking at those logs, but I do know that Stylo is using an old bindgen version. At a glance, those errors all look vaguely template-y related, and template stuff received an overhaul in newer bindgen. In fact, we're now building bindings for stylo as a sanity/smoke test in bindgen's CI, so I suspect a bindgen upgrade will fix the issues you are seeing.

If you want, you can try with a local bindgen checkout. If you choose to go down this route, can you verify that stylo binding generation works with bindgen master? To use a local bindgen, add this to the bottom of Servo's top level Cargo.toml:

    [replace]
    "bindgen:0.22.0": { path = "/path/to/bindgen/checkout" }

Follow https://github.com/servo/servo/pull/16392 for Stylo's bindgen version bump.
Flags: needinfo?(nfitzgerald)
Blocks: stylo-tooling
Priority: -- → P2
> Could you delete your obj folder and rebuild again?

No change. Same errors.


> If you want, you can try with a local bindgen checkout. If you choose to go
> down this route, can you verify that stylo binding generation works with
> bindgen master? To use a local bindgen, add this to the bottom of Servo's
> top level Cargo.toml:
> 
>     [replace]
>     "bindgen:0.22.0": { path = "/path/to/bindgen/checkout" }

I tried that -- modifying servo/Cargo.toml within my mozilla-inbound clone -- and got the same error. I also tried "bindgen:0.23.0" and got the same error.

In both cases I got this line just before the errors:

> 4:13.67    Compiling bindgen v0.22.0

Is that expected? I'm not convinced my local copy of rust-bindgen is being used. How can I tell?
Flags: needinfo?(nfitzgerald)
I don't think your local copy is being used. Could you attach also the structs files?
Flags: needinfo?(n.nethercote)
I tried adding a deliberate syntax error to src/main.rs in rust-bindgen, and it had no effect. I then did likewise with servo/Cargo.toml, again to no effect. So I have no idea what's going on.

> Could you attach also the structs files?

Which files? I've already attached structs_release.rs.
Flags: needinfo?(n.nethercote)
Whoops, sorry, missed the second attachment.

Seems we're generating basic_string without using it anywhere, which seems dumb. First of all, does this patch fix your build?

diff --git a/servo/components/style/build_gecko.rs b/servo/components/style/build_gecko.rs
index 07ed63298084..4c10d7bd92bd 100644
--- a/servo/components/style/build_gecko.rs
+++ b/servo/components/style/build_gecko.rs
@@ -454,6 +454,8 @@ mod bindings {
             "std::namespace::atomic___base", "std::atomic__My_base",
             "std::atomic",
             "std::atomic___base",
+            "std::string",
+            "std::basic_string",
             "mozilla::gfx::.*",
             "FallibleTArray",
             "mozilla::dom::Sequence",


Second, the "replaces" thing will only work to debug bindgen if you are doing:

./mach build-geckolib --with-gecko /path/to/gecko/checkout/objdir/dist

From the servo dir. I'm not sure what the right path is for [replace] in a Gecko build.
> Seems we're generating basic_string without using it anywhere, which seems
> dumb. First of all, does this patch fix your build?

It's a lot closer to working. I'm down to two errors now:

>  error[E0091]: type parameter `T` is unused
>      --> /home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-3490280569f9f1de/out/gecko/structs_release.rs:5887:36
>       |
>  5887 |             pub type conditional_t<T, F> = ();
>       |                                    ^ unused type parameter
>  
>  error[E0091]: type parameter `F` is unused
>      --> /home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-3490280569f9f1de/out/gecko/structs_release.rs:5887:39
>       |
>  5887 |             pub type conditional_t<T, F> = ();
>       |                                       ^ unused type parameter

Seems like the kind of thing that could be a warning rather than an error?
(In reply to Nicholas Nethercote [:njn] from comment #8)
> > Seems we're generating basic_string without using it anywhere, which seems
> > dumb. First of all, does this patch fix your build?
> 
> It's a lot closer to working. I'm down to two errors now:

Nice! Adding mozilla::span_details::conditional_t to the same list should make it work. This should be fixed by the bindgen update, but that also fails for me and :heycam, so we need to work a bit more on that.

> Seems like the kind of thing that could be a warning rather than an error?

Ask that to the rustc devs, that's been a bindgen pain point for ages :)
I think Emilio responded to everything I was ni'd for. Will dig into the bindgen update issues from the servo PR.
Flags: needinfo?(nfitzgerald)
No longer blocks: 1356988
I now get *tons* of warnings like this:

> error: use of extern static requires unsafe function or block (error E0133)
>       --> /home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-3490280569f9f1de/out/properties.rs:115488:12
>        |
> 115488 |         if structs::root::mozilla::SERVO_PREF_ENABLED_background_color {
>        |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>        |
>        = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
>        = note: for more information, see issue #36247 <https://github.com/rust-lang/rust/issues/35112>
> note: lint level defined here
>       --> /home/njn/moz/mi2/servo/components/style/lib.rs:26:9
>        |
> 26     | #![deny(warnings)]
>        |         ^^^^^^^^
> 
> error: use of extern static requires unsafe function or block (error E0133)
>       --> /home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-3490280569f9f1de/out/properties.rs:115512:12
>        |
> 115512 |         if structs::root::mozilla::SERVO_PREF_ENABLED_background_image {
>        |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>        |
>        = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
>        = note: for more information, see issue #36247 <https://github.com/rust-lang/rust/issues/35112>

fitgzen, any ideas?
Flags: needinfo?(nfitzgerald)
Are you using clang 3.8? We used to support it, but people have started using the constant evaluation features of clang 3.9+.

If so, you need to update your libclang version to at least 3.9
Flags: needinfo?(nfitzgerald)
As per comment 0, I'm using clang 3.9. Not clang 4.0, not clang 3.8. clang 3.9.
Can you clone bindgen, run the following, and paste the result here? (Or, in particular, whether the output is different from https://github.com/servo/rust-bindgen/blob/master/tests/expectations/tests/constant-evaluate.rs).

 $ cargo build
 $ ./target/debug/bindgen tests/headers/constant-evaluate.h

If so, can you post here the output of:

 $ RUST_LOG=bindgen ./target/debug/bindgen tests/headers/constant-evaluate.h
Attached file output for comment 15 (deleted) — 
(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> Can you clone bindgen, run the following, and paste the result here? (Or, in
> particular, whether the output is different from
> https://github.com/servo/rust-bindgen/blob/master/tests/expectations/tests/
> constant-evaluate.rs).

It is different.
 
>  $ cargo build
>  $ ./target/debug/bindgen tests/headers/constant-evaluate.h
> 
> If so, can you post here the output of:
> 
>  $ RUST_LOG=bindgen ./target/debug/bindgen tests/headers/constant-evaluate.h

Output (both stdout and stderr) is attached.
> Output (both stdout and stderr) is attached.

Wait, that's using clang 3.8, which is my default |clang|. But when I build stylo I'm specifying |clang-3.9| via the mozconfig, as per comment 0.
Hmm, so it seems to be that bindgen is still using libclang 3.8 for the stylo build...

So, we call into clang_sys::load(), which ends up calling this function: https://github.com/KyleMayes/clang-sys/blob/master/build.rs#L126

That looks first into LIBCLANG_PATH, then runs llvm-config. I assume this isn't bug 1355464 because it's Linux, so the LIBCLANG_PATH environment variable should still be there. Does it happen to be the case that LIBCLANG_PATH is the same for libclang 3.8 and 3.9? That could explain it I guess.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> That looks first into LIBCLANG_PATH, then runs llvm-config. I assume this
> isn't bug 1355464 because it's Linux, so the LIBCLANG_PATH environment
> variable should still be there. Does it happen to be the case that
> LIBCLANG_PATH is the same for libclang 3.8 and 3.9?

Nick, is your LIBCLANG_PATH the same for libclang 3.8 and 3.9? configure should now require exactly 3.9 (bug 1357889) or 5.0+ (bug 1359508).
Flags: needinfo?(n.nethercote)
Nick says he is going to try making clang 3.9 the system default on his Linux machine. If bindgen works, then it seems like configure or bindgen is somehow not honoring his mozconfig settings to use clang 3.9 when 3.8 is his system default.

https://askubuntu.com/questions/907873/how-to-set-clang-3-9-as-the-default-in-zesty/907886#907886
(In reply to Chris Peterson [:cpeterson] from comment #19)
> Nick says he is going to try making clang 3.9 the system default on his
> Linux machine. If bindgen works, then it seems like configure or bindgen is
> somehow not honoring his mozconfig settings to use clang 3.9 when 3.8 is his
> system default.

It didn't work. I'm getting the same old errors.
Flags: needinfo?(n.nethercote)
Fix at https://github.com/KyleMayes/clang-sys/pull/51.
Finally got it working with help from Emilio.

The fix was to add a symlink from libclang.so to libclang.so.1 in /usr/lib/llvm-3.9/lib/. Emilio said he will land a fix to clang-sys to address this issue.
Emilio's fix has been merged upstream and released in clang-sys versions v0.14.1 and v0.15.2. I filed servo and rust-bindgen bugs about upgrading to clang-sys v0.14.1 and v0.15.2, respectively:

https://github.com/servo/servo/issues/16653

https://github.com/servo/rust-bindgen/issues/671
Thanks for the update, cpeterson. I think we can declare victory now.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: