Closed Bug 1368083 Opened 7 years ago Closed 7 years ago

bindgen fails with undefined identifiers in clang/include/c++/v1/stdlib.h

Categories

(Firefox Build System :: General, defect)

x86_64
macOS
defect
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

Details

Attachments

(1 file, 2 obsolete files)

In integration builds with stylo or webrender enabled on macOS 10.7 with the fix from bug 1365993 applied fail in bindgen. It complains about a bunch of undefined identifiers in clang libstdc++ headers and few other files, then panics.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a92424ea8e26bb0da4230f66bacc9467d68e2b1a&selectedJob=102119107

> INFO -  error: failed to run custom build command for `style v0.0.1 (file:///home/worker/workspace/build/src/servo/components/style)`
> INFO -  process didn't exit successfully: `/home/worker/workspace/build/src/obj-firefox/toolkit/library/debug/build/style-233864485ecf600f/build-script-build` (exit code: 101)
> INFO -  --- stdout
> INFO -  cargo:rerun-if-changed=build.rs
> INFO -  cargo:out_dir=/home/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-apple-darwin/debug/build/style-6392e431471a656d/out
> INFO -  cargo:rerun-if-changed=properties/gecko.mako.rs
[...]
> INFO -  cargo:rerun-if-changed=/home/worker/workspace/build/src/obj-firefox/dist/include/nsIDOMMediaList.h
> INFO -  /home/worker/workspace/build/src/clang/include/c++/v1/stdlib.h:117:81: error: use of undeclared identifier 'llabs', err: true
> INFO -  /home/worker/workspace/build/src/clang/include/c++/v1/stdlib.h:122:34: error: unknown type name 'lldiv_t', err: true
> [...]
> INFO -  /home/worker/workspace/build/src/clang/include/c++/v1/iterator:417:10: fatal error: 'Availability.h' file not found
> INFO -  thread '<unnamed>' panicked at 'Failed to generate bindings, [...]
> INFO -  stack backtrace:
> 0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
> INFO -               at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
> INFO -     1: std::sys_common::backtrace::_print
> INFO -               at /checkout/src/libstd/sys_common/backtrace.rs:71
> INFO -     2: std::panicking::default_hook::{{closure}}
> INFO -               at /checkout/src/libstd/sys_common/backtrace.rs:60
> INFO -               at /checkout/src/libstd/panicking.rs:355
> INFO -     3: std::panicking::default_hook
> INFO -               at /checkout/src/libstd/panicking.rs:371
> INFO -     4: std::panicking::rust_panic_with_hook
> INFO -               at /checkout/src/libstd/panicking.rs:549
> INFO -     5: std::panicking::begin_panic
> INFO -               at /checkout/src/libstd/panicking.rs:511
> INFO -     6: std::panicking::begin_panic_fmt
> INFO -               at /checkout/src/libstd/panicking.rs:495
> INFO -     7: build_script_build::build_gecko::bindings::write_binding_file
> INFO -               at ./build_gecko.rs:275
> INFO -     8: build_script_build::build_gecko::bindings::generate_bindings
> INFO -               at ./build_gecko.rs:533
> INFO -     9: std::panicking::try::do_call
> INFO -               at /checkout/src/libstd/panicking.rs:454
> INFO -    10: __rust_maybe_catch_panic
> INFO -               at /checkout/src/libpanic_unwind/lib.rs:98
> INFO -    11: std::panicking::try
> INFO -               at /checkout/src/libstd/panicking.rs:433
> INFO -    12: std::panic::catch_unwind
> INFO -               at /checkout/src/libstd/panic.rs:361
> INFO -    13: std::thread::Builder::spawn::{{closure}}
> INFO -               at /checkout/src/libstd/thread/mod.rs:360
> INFO -    14: <F as alloc::boxed::FnBox<A>>::call_box
> INFO -               at /checkout/src/liballoc/boxed.rs:640
> INFO -    15: std::sys::imp::thread::Thread::new::thread_start
> INFO -               at /checkout/src/liballoc/boxed.rs:650
> INFO -               at /checkout/src/libstd/sys_common/thread.rs:21
> INFO -               at /checkout/src/libstd/sys/unix/thread.rs:84
> INFO -    16: start_thread
> INFO -    17: clone
> INFO -  thread '<unnamed>' panicked at 'Failed to generate bindings, [...] 
> INFO -  stack backtrace:
> INFO -     0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
> INFO -               at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
> INFO -     1: std::sys_common::backtrace::_print
> INFO -               at /checkout/src/libstd/sys_common/backtrace.rs:71
> INFO -     2: std::panicking::default_hook::{{closure}}
> INFO -               at /checkout/src/libstd/sys_common/backtrace.rs:60
> INFO -               at /checkout/src/libstd/panicking.rs:355
> INFO -     3: std::panicking::default_hook
> INFO -               at /checkout/src/libstd/panicking.rs:371
> INFO -     4: std::panicking::rust_panic_with_hook
> INFO -               at /checkout/src/libstd/panicking.rs:549
> INFO -     5: std::panicking::begin_panic
> INFO -               at /checkout/src/libstd/panicking.rs:511
> INFO -     6: std::panicking::begin_panic_fmt
> INFO -               at /checkout/src/libstd/panicking.rs:495
> INFO -     7: build_script_build::build_gecko::bindings::write_binding_file
> INFO -               at ./build_gecko.rs:275
> INFO -     8: build_script_build::build_gecko::bindings::generate_structs
> INFO -               at ./build_gecko.rs:421
> INFO -     9: std::panicking::try::do_call
> INFO -               at /checkout/src/libstd/panicking.rs:454
> INFO -    10: __rust_maybe_catch_panic
> INFO -               at /checkout/src/libpanic_unwind/lib.rs:98
> INFO -    11: std::panicking::try
> INFO -               at /checkout/src/libstd/panicking.rs:433
> INFO -    12: std::panic::catch_unwind
> INFO -               at /checkout/src/libstd/panic.rs:361
> INFO -    13: std::thread::Builder::spawn::{{closure}}
> INFO -               at /checkout/src/libstd/thread/mod.rs:360
> INFO -    14: <F as alloc::boxed::FnBox<A>>::call_box
> INFO -               at /checkout/src/liballoc/boxed.rs:640
> INFO -    15: std::sys::imp::thread::Thread::new::thread_start
> INFO -               at /checkout/src/liballoc/boxed.rs:650
> INFO -               at /checkout/src/libstd/sys_common/thread.rs:21
> INFO -               at /checkout/src/libstd/sys/unix/thread.rs:84
> INFO -    16: start_thread
> INFO -    17: clone
> [...]
> INFO -  gmake[5]: *** [force-cargo-library-build] Error 101
Not sure what's going on here. Is bindgen seeing different include directives than the rest of the build? I didn't see this doing a manual build on a ci loaner.
The flags passed there look mostly fine. We're giving bindgen |-stdlib=libc++ --target=x86_64-apple-darwin|, is that sane?
Some clarification: I think this is triggered on both the native (10.7) and cross automation builds, possibly shadowed by bug 1368163. This may also have the same root cause as bug 1366564 where missing stdlib.h is fixed by running `xcode-select --install`. I haven't had time to investigate further but will look later in the week.
Maybe we just need to change bindgen's include path to look for stdlib.h/etc in the right Xcode SDK directories instead of defaulting to /usr/include?
Assignee: nobody → giles
Applying the enable patch from bug 1373384 this issue reproduces on cross-opt builds as well. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8570ec40ad95f1142d32f3bd5917fd38dbdbf53&selectedJob=108308092
Updating to llvm/clang 4.0.1 doesn't fix this issue, so it looks like it's separate. https://treeherder.mozilla.org/#/jobs?repo=try&revision=509726c36849613a6444784d9e5a37759c4597c3
Maybe something we need to fix in the build script for stylo; building cross like we do is probably not something that's been tested at all by bindgen or clang-sys.
Blocks: 1375774
No longer blocks: stylo-nightly-build
Depends on: 1375798
(In reply to Nathan Froyd [:froydnj] from comment #7)
> Maybe something we need to fix in the build script for stylo; building cross
> like we do is probably not something that's been tested at all by bindgen or
> clang-sys.

At the very least we probably need to get bindgen's clang args to include the -isysroot we set in $FLAGS:
https://dxr.mozilla.org/mozilla-central/rev/b1b9129838ade91684574f42219b2010928d7db4/build/macosx/cross-mozconfig.common#25
so stylo's build script (https://dxr.mozilla.org/mozilla-central/source/servo/components/style/build_gecko.rs) reads a .toml config file to set a bunch of clang arguments:
https://dxr.mozilla.org/mozilla-central/source/layout/style/ServoBindings.toml

We could write out some additional arguments to a file in the objdir in configure, and then have the build script read that and add them to the commandline. We already define a `MOZ_TOPOBJDIR` environment variable while building Rust code, so it could locate the file relative to that.

As to what we'd put in the file, I guess that's tricky. We could punt initially and just explicitly define a variable like `STYLO_BINDINGS_CFLAGS`. Longer-term it would be nicer if bindgen used the same compiler flags we used for building Gecko when possible, but that's harder to get right (especially for builds where clang is not the C compiler).
Ted's hypothesis seems to be correct here. I was able to complete a local cross build simply by adding

> "-isysroot", "/home/giles/firefox/cross/MacOSX10.7.sdk",

to layout/style/ServoBindings.toml. I don't know if the -B option is also necessary; we seem to do ok without it. I'll work on the suggested extension.
Hard-coding -isysroot also fixed the issue on automation. Now I just have to figure out how to merge to BTreeSet structs...
Attachment #8883786 - Flags: review?(emilio+bugs)
Comment on attachment 8883786 [details]
Bug 1368083 - Read bindgen flags from a generated file.

https://reviewboard.mozilla.org/r/154736/#review159848

this looks good to me, assuming someone else has reviewed the generation of `bindgen.toml`.

Thanks!

::: servo/components/style/build_gecko.rs:93
(Diff revision 1)
> +    lazy_static! {
> +        static ref CONFIG: toml::Table = {
> +            // Load Gecko's binding generator config from the source tree.
> +            let path = PathBuf::from(env::var("MOZ_SRC").unwrap())
> +                .join("layout/style/ServoBindings.toml");
> +            let src_config = read_config(&path);

nit: Maybe just `read_config(&path)` (without the extra variable)?

Or is this working around (the lack of) non-lexical borrows?

Same below.
Attachment #8883786 - Flags: review?(emilio+bugs) → review+
> to layout/style/ServoBindings.toml. I don't know if the -B option is also necessary; we seem to do ok without it. I'll work on the suggested extension.

-B is just to tell the compiler where to locate other parts of the toolchain, like the linker, so it shouldn't be necessary for bindgen's usage.
Comment on attachment 8883786 [details]
Bug 1368083 - Read bindgen flags from a generated file.

https://reviewboard.mozilla.org/r/154736/#review159848

> nit: Maybe just `read_config(&path)` (without the extra variable)?
> 
> Or is this working around (the lack of) non-lexical borrows?
> 
> Same below.

This is left over from when I was trying to merge the two `toml::Table` structs at parse time. I'll remove them.
Comment on attachment 8884026 [details]
Bug 1368083 - Better bindgen error message when files are missing.

https://reviewboard.mozilla.org/r/154976/#review160272

r=me, thanks!
Attachment #8884026 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8884025 [details]
Bug 1368083 - Pass -isysroot to bindgen.

https://reviewboard.mozilla.org/r/154974/#review160342

::: build/moz.configure/toolchain.configure:906
(Diff revision 2)
> +@checking('bindgen flags')
> +def bindgen_flags(compiler):
> +    # Export relevant CXX flags so we can pass them to bindgen.
> +    args = compiler.flags
> +    try:
> +        index = args.index(u'-isysroot')

On IRC, ted said, "so hey, instead of trying to filter -isysroot out of CFLAGS, what would you think about just setting like a `BINDGEN_CFLAGS` var directly?"

I guess this means propagating BINDGEN_CFLAGS from the environment directly and adding another line to do that to the cross-mac mozconfigs.

I suppose that's fine, and would be a bit simpler, especially as the flags are already modular in mozconfig. I did it this way because I thought we were trying to converge the options bindgen uses with libclang with the ones we pass to clang for our C++ compilation.
Comment on attachment 8884025 [details]
Bug 1368083 - Pass -isysroot to bindgen.

https://reviewboard.mozilla.org/r/154974/#review160342

> On IRC, ted said, "so hey, instead of trying to filter -isysroot out of CFLAGS, what would you think about just setting like a `BINDGEN_CFLAGS` var directly?"
> 
> I guess this means propagating BINDGEN_CFLAGS from the environment directly and adding another line to do that to the cross-mac mozconfigs.
> 
> I suppose that's fine, and would be a bit simpler, especially as the flags are already modular in mozconfig. I did it this way because I thought we were trying to converge the options bindgen uses with libclang with the ones we pass to clang for our C++ compilation.

I think I have a shorter patch based on this suggestion, but it needs some cleanup. More later.
Comment on attachment 8884025 [details]
Bug 1368083 - Pass -isysroot to bindgen.

https://reviewboard.mozilla.org/r/154974/#review160342

> I think I have a shorter patch based on this suggestion, but it needs some cleanup. More later.

I turned out to not really be any shorter, since we still need to convert the flags string into a toml sequence, but this is at least more explicit. Passing review to gps while ted is away.
Blocks: 1379339
Comment on attachment 8884025 [details]
Bug 1368083 - Pass -isysroot to bindgen.

https://reviewboard.mozilla.org/r/154974/#review160468

The part about normalizing the value so it can fit in a TOML list feels a bit hacky. But, hey, a) this is the build system b) I can't think of a much better while simple solution c) this in support of Stylo. Therefore "meh" applies.

I'm also not super confident that the moz.configure bits are ideal because I haven't been reviewing too much of that code. But it looks right enough to me. So r+ :)
Attachment #8884025 - Flags: review?(gps) → review+
Ok, thanks for the after-hours review. It's definitely hacky, but the only other thing I could think of was to write a toml generator, and that felt like over-engineering.
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 6 changes to 5 files
remote: (8d24ab361c92 modifies servo/components/style/build_gecko.rs from Servo; not enforcing peer review)
remote: (c2c0aa01b6d2 modifies servo/components/style/build_gecko.rs from Servo; not enforcing peer review)
remote: (2 changesets contain changes to protected servo/ directory: c2c0aa01b6d2, 8d24ab361c92)
remote: ************************************************************************
remote: you do not have permissions to modify files under servo/
remote: 
remote: the servo/ directory is kept in sync with the canonical upstream
remote: repository at https://github.com/servo/servo
remote: 
remote: changes to servo/ are only allowed by the syncing tool and by sheriffs
remote: performing cross-repository "merges"
remote: 
remote: to make changes to servo/, submit a Pull Request against the servo/servo
remote: GitHub project
remote: ************************************************************************
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.e_prevent_vendored hook failed
abort: push failed on remote
Apparently we don't have two-way sync. I filed https://github.com/servo/servo/pull/17638 to get the `build_gecko.rs` changes merged, and will land the gecko changes first on this bug.
Attachment #8883786 - Attachment is obsolete: true
Attachment #8884026 - Attachment is obsolete: true
Comment on attachment 8884025 [details]
Bug 1368083 - Pass -isysroot to bindgen.

https://reviewboard.mozilla.org/r/154974/#review160486

::: build/moz.configure/toolchain.configure:910
(Diff revision 4)
> +@checking('bindgen cflags', lambda s: s if s and s.strip() else 'no')
> +def bindgen_cflags(value):
> +    if value and len(value):
> +        # Reformat the env value for substitution into a toml list.
> +        flags = value[0].split()
> +        return ', '.join('"' + flag + '"' for flag in flags)

would probably be less bad to dump the list as json.
https://hg.mozilla.org/mozilla-central/rev/7e4599280174
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.